From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Robbie Harwood Date: Fri, 10 Jun 2022 14:40:33 -0400 Subject: [PATCH] cms: store digest as pointer instead of index Storage as an index is problematic because the sentinel value -1 was used, but accesses were unchecked, leading to crashes like that in 3b1031a6b779cb80c11b34eec84c5a0cc215efed ("pesigcheck: Fix crash on digest match"). By storing a pointer, we get an explicit NULL dereference: still a crash, but preferred since it's clearer. Since the index was previously also used for retrieving digest parameters, include a pointer to the relevant struct digest_param in the struct digest. Signed-off-by: Robbie Harwood --- src/certdb.c | 15 ++++++++------- src/cms_common.c | 34 ++++++++++------------------------ src/content_info.c | 4 ++-- src/file_kmod.c | 2 +- src/file_pe.c | 9 +++++---- src/pesigcheck.c | 4 +--- src/cms_common.h | 13 ++++++++++++- 7 files changed, 39 insertions(+), 42 deletions(-) diff --git a/src/certdb.c b/src/certdb.c index 69d5daf..f512824 100644 --- a/src/certdb.c +++ b/src/certdb.c @@ -263,18 +263,19 @@ check_hash(pesigcheck_context *ctx, SECItem *sig, efi_guid_t *sigtype, { efi_guid_t efi_sha256 = efi_guid_sha256; efi_guid_t efi_sha1 = efi_guid_sha1; - void *digest; + void *digest_data; + struct digest *digests = ctx->cms_ctx->digests; if (memcmp(sigtype, &efi_sha256, sizeof(efi_guid_t)) == 0) { - digest = ctx->cms_ctx->digests[0].pe_digest->data; - if (memcmp (digest, sig->data, 32) == 0) { - ctx->cms_ctx->selected_digest = 0; + digest_data = digests[0].pe_digest->data; + if (memcmp (digest_data, sig->data, 32) == 0) { + ctx->cms_ctx->selected_digest = &digests[0]; return FOUND; } } else if (memcmp(sigtype, &efi_sha1, sizeof(efi_guid_t)) == 0) { - digest = ctx->cms_ctx->digests[1].pe_digest->data; - if (memcmp (digest, sig->data, 20) == 0) { - ctx->cms_ctx->selected_digest = 1; + digest_data = digests[1].pe_digest->data; + if (memcmp (digest_data, sig->data, 20) == 0) { + ctx->cms_ctx->selected_digest = &digests[1]; return FOUND; } } diff --git a/src/cms_common.c b/src/cms_common.c index 86341ca..2275f67 100644 --- a/src/cms_common.c +++ b/src/cms_common.c @@ -33,15 +33,6 @@ #include "hex.h" -struct digest_param { - char *name; - SECOidTag digest_tag; - SECOidTag signature_tag; - SECOidTag digest_encryption_tag; - const efi_guid_t *efi_guid; - int size; -}; - static struct digest_param digest_params[] = { {.name = "sha256", .digest_tag = SEC_OID_SHA256, @@ -65,29 +56,25 @@ static int n_digest_params = sizeof (digest_params) / sizeof (digest_params[0]); SECOidTag digest_get_digest_oid(cms_context *cms) { - int i = cms->selected_digest; - return digest_params[i].digest_tag; + return cms->selected_digest->digest_params->digest_tag; } SECOidTag digest_get_encryption_oid(cms_context *cms) { - int i = cms->selected_digest; - return digest_params[i].digest_encryption_tag; + return cms->selected_digest->digest_params->digest_encryption_tag; } SECOidTag digest_get_signature_oid(cms_context *cms) { - int i = cms->selected_digest; - return digest_params[i].signature_tag; + return cms->selected_digest->digest_params->signature_tag; } int digest_get_digest_size(cms_context *cms) { - int i = cms->selected_digest; - return digest_params[i].size; + return cms->selected_digest->digest_params->size; } void @@ -142,8 +129,6 @@ cms_context_init(cms_context *cms) if (!cms->arena) cnreterr(-1, cms, "could not create cryptographic arena"); - cms->selected_digest = -1; - INIT_LIST_HEAD(&cms->pk12_ins); cms->pk12_out.fd = -1; cms->db_out = cms->dbx_out = cms->dbt_out = -1; @@ -226,7 +211,7 @@ cms_context_fini(cms_context *cms) memset(&cms->newsig, '\0', sizeof (cms->newsig)); } - cms->selected_digest = -1; + cms->selected_digest = NULL; if (cms->ci_digest) { free_poison(cms->ci_digest->data, cms->ci_digest->len); @@ -351,7 +336,7 @@ set_digest_parameters(cms_context *cms, char *name) if (strcmp(name, "help")) { for (int i = 0; i < n_digest_params; i++) { if (!strcmp(name, digest_params[i].name)) { - cms->selected_digest = i; + cms->selected_digest = &cms->digests[i]; return 0; } } @@ -1279,6 +1264,7 @@ generate_digest_begin(cms_context *cms) cngotoerr(err, cms, "could not create digest context"); PK11_DigestBegin(digests[i].pk11ctx); + digests[i].digest_params = &digest_params[i]; } cms->digests = digests; @@ -1351,11 +1337,11 @@ generate_signature(cms_context *cms) { int rc = 0; - if (cms->digests[cms->selected_digest].pe_digest == NULL) + if (cms->selected_digest->pe_digest == NULL) cnreterr(-1, cms, "PE digest has not been allocated"); - if (content_is_empty(cms->digests[cms->selected_digest].pe_digest->data, - cms->digests[cms->selected_digest].pe_digest->len)) + if (content_is_empty(cms->selected_digest->pe_digest->data, + cms->selected_digest->pe_digest->len)) cnreterr(-1, cms, "PE binary has not been digested"); SECItem sd_der; diff --git a/src/content_info.c b/src/content_info.c index 9684850..777aa28 100644 --- a/src/content_info.c +++ b/src/content_info.c @@ -181,8 +181,8 @@ generate_spc_digest_info(cms_context *cms, SECItem *dip) if (generate_algorithm_id(cms, &di.digestAlgorithm, digest_get_digest_oid(cms)) < 0) return -1; - int i = cms->selected_digest; - memcpy(&di.digest, cms->digests[i].pe_digest, sizeof (di.digest)); + memcpy(&di.digest, cms->selected_digest->pe_digest, + sizeof(di.digest)); if (content_is_empty(di.digest.data, di.digest.len)) { cms->log(cms, LOG_ERR, "got empty digest"); diff --git a/src/file_kmod.c b/src/file_kmod.c index 6880cda..c8875fc 100644 --- a/src/file_kmod.c +++ b/src/file_kmod.c @@ -60,7 +60,7 @@ ssize_t kmod_write_signature(cms_context *cms, int outfd) { SEC_PKCS7ContentInfo *cinfo; - SECItem *digest = cms->digests[cms->selected_digest].pe_digest; + SECItem *digest = cms->selected_digest->pe_digest; SECStatus rv; struct write_sig_info info = { .outfd = outfd, diff --git a/src/file_pe.c b/src/file_pe.c index 805e614..c22b2af 100644 --- a/src/file_pe.c +++ b/src/file_pe.c @@ -114,6 +114,8 @@ check_inputs(pesign_context *ctx) static void print_digest(pesign_context *pctx) { + unsigned int i; + if (!pctx) return; @@ -121,10 +123,9 @@ print_digest(pesign_context *pctx) if (!ctx) return; - int j = ctx->selected_digest; - for (unsigned int i = 0; i < ctx->digests[j].pe_digest->len; i++) - printf("%02x", - (unsigned char)ctx->digests[j].pe_digest->data[i]); + unsigned char *ddata = ctx->selected_digest->pe_digest->data; + for (i = 0; i < ctx->selected_digest->pe_digest->len; i++) + printf("%02x", ddata[i]); printf(" %s\n", pctx->infile); } diff --git a/src/pesigcheck.c b/src/pesigcheck.c index 6dc67f7..ebb404d 100644 --- a/src/pesigcheck.c +++ b/src/pesigcheck.c @@ -221,9 +221,7 @@ static void get_digest(pesigcheck_context *ctx, SECItem *digest) { struct cms_context *cms = ctx->cms_ctx; - struct digest *cms_digest = &cms->digests[cms->selected_digest]; - - memcpy(digest, cms_digest->pe_digest, sizeof (*digest)); + memcpy(digest, cms->selected_digest->pe_digest, sizeof(*digest)); } static int diff --git a/src/cms_common.h b/src/cms_common.h index c7acbcf..c7d4f69 100644 --- a/src/cms_common.h +++ b/src/cms_common.h @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -57,9 +58,19 @@ goto errlabel; \ }) +struct digest_param { + char *name; + SECOidTag digest_tag; + SECOidTag signature_tag; + SECOidTag digest_encryption_tag; + const efi_guid_t *efi_guid; + int size; +}; + struct digest { PK11Context *pk11ctx; SECItem *pe_digest; + struct digest_param *digest_params; }; typedef struct pk12_file { @@ -133,7 +144,7 @@ typedef struct cms_context { int db_out, dbx_out, dbt_out; struct digest *digests; - int selected_digest; + struct digest *selected_digest; int omit_vendor_cert; SECItem newsig;