From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Mon, 29 Aug 2022 16:22:18 -0400 Subject: [PATCH] Revert "cms: store digest as pointer instead of index" In 926782c216532a83f9ff864dee39d2349d61fd23, we switched cms->selected_digest to be a pointer to the member of the digests array rather than an index. Unfortunately this is just as bad, because the bugs that come up wind up setting pointers to NULL+(selected*offset), i.e. 0x10, and that doesn't get us any closer to actually finding any problem. For now, the new approach is going to be to make it an index again, but to default it to 0 (sha256) rather than -1, so if it isn't set at the correct part of the lifecycle it'll just default to the (nearly always) correct choice. This reverts commit 926782c216532a83f9ff864dee39d2349d61fd23. Signed-off-by: Peter Jones --- 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, 42 insertions(+), 39 deletions(-) diff --git a/src/certdb.c b/src/certdb.c index f512824..69d5daf 100644 --- a/src/certdb.c +++ b/src/certdb.c @@ -263,19 +263,18 @@ 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_data; - struct digest *digests = ctx->cms_ctx->digests; + void *digest; if (memcmp(sigtype, &efi_sha256, sizeof(efi_guid_t)) == 0) { - digest_data = digests[0].pe_digest->data; - if (memcmp (digest_data, sig->data, 32) == 0) { - ctx->cms_ctx->selected_digest = &digests[0]; + digest = ctx->cms_ctx->digests[0].pe_digest->data; + if (memcmp (digest, sig->data, 32) == 0) { + ctx->cms_ctx->selected_digest = 0; return FOUND; } } else if (memcmp(sigtype, &efi_sha1, sizeof(efi_guid_t)) == 0) { - digest_data = digests[1].pe_digest->data; - if (memcmp (digest_data, sig->data, 20) == 0) { - ctx->cms_ctx->selected_digest = &digests[1]; + digest = ctx->cms_ctx->digests[1].pe_digest->data; + if (memcmp (digest, sig->data, 20) == 0) { + ctx->cms_ctx->selected_digest = 1; return FOUND; } } diff --git a/src/cms_common.c b/src/cms_common.c index 2275f67..86341ca 100644 --- a/src/cms_common.c +++ b/src/cms_common.c @@ -33,6 +33,15 @@ #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, @@ -56,25 +65,29 @@ static int n_digest_params = sizeof (digest_params) / sizeof (digest_params[0]); SECOidTag digest_get_digest_oid(cms_context *cms) { - return cms->selected_digest->digest_params->digest_tag; + int i = cms->selected_digest; + return digest_params[i].digest_tag; } SECOidTag digest_get_encryption_oid(cms_context *cms) { - return cms->selected_digest->digest_params->digest_encryption_tag; + int i = cms->selected_digest; + return digest_params[i].digest_encryption_tag; } SECOidTag digest_get_signature_oid(cms_context *cms) { - return cms->selected_digest->digest_params->signature_tag; + int i = cms->selected_digest; + return digest_params[i].signature_tag; } int digest_get_digest_size(cms_context *cms) { - return cms->selected_digest->digest_params->size; + int i = cms->selected_digest; + return digest_params[i].size; } void @@ -129,6 +142,8 @@ 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; @@ -211,7 +226,7 @@ cms_context_fini(cms_context *cms) memset(&cms->newsig, '\0', sizeof (cms->newsig)); } - cms->selected_digest = NULL; + cms->selected_digest = -1; if (cms->ci_digest) { free_poison(cms->ci_digest->data, cms->ci_digest->len); @@ -336,7 +351,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 = &cms->digests[i]; + cms->selected_digest = i; return 0; } } @@ -1264,7 +1279,6 @@ 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; @@ -1337,11 +1351,11 @@ generate_signature(cms_context *cms) { int rc = 0; - if (cms->selected_digest->pe_digest == NULL) + if (cms->digests[cms->selected_digest].pe_digest == NULL) cnreterr(-1, cms, "PE digest has not been allocated"); - if (content_is_empty(cms->selected_digest->pe_digest->data, - cms->selected_digest->pe_digest->len)) + if (content_is_empty(cms->digests[cms->selected_digest].pe_digest->data, + cms->digests[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 777aa28..9684850 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; - memcpy(&di.digest, cms->selected_digest->pe_digest, - sizeof(di.digest)); + int i = cms->selected_digest; + memcpy(&di.digest, cms->digests[i].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 c8875fc..6880cda 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->selected_digest->pe_digest; + SECItem *digest = cms->digests[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 c22b2af..805e614 100644 --- a/src/file_pe.c +++ b/src/file_pe.c @@ -114,8 +114,6 @@ check_inputs(pesign_context *ctx) static void print_digest(pesign_context *pctx) { - unsigned int i; - if (!pctx) return; @@ -123,9 +121,10 @@ print_digest(pesign_context *pctx) if (!ctx) return; - unsigned char *ddata = ctx->selected_digest->pe_digest->data; - for (i = 0; i < ctx->selected_digest->pe_digest->len; i++) - printf("%02x", ddata[i]); + 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]); printf(" %s\n", pctx->infile); } diff --git a/src/pesigcheck.c b/src/pesigcheck.c index ebb404d..6dc67f7 100644 --- a/src/pesigcheck.c +++ b/src/pesigcheck.c @@ -221,7 +221,9 @@ static void get_digest(pesigcheck_context *ctx, SECItem *digest) { struct cms_context *cms = ctx->cms_ctx; - memcpy(digest, cms->selected_digest->pe_digest, sizeof(*digest)); + struct digest *cms_digest = &cms->digests[cms->selected_digest]; + + memcpy(digest, cms_digest->pe_digest, sizeof (*digest)); } static int diff --git a/src/cms_common.h b/src/cms_common.h index c7d4f69..c7acbcf 100644 --- a/src/cms_common.h +++ b/src/cms_common.h @@ -12,7 +12,6 @@ #include #include -#include #include #include #include @@ -58,19 +57,9 @@ 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 { @@ -144,7 +133,7 @@ typedef struct cms_context { int db_out, dbx_out, dbt_out; struct digest *digests; - struct digest *selected_digest; + int selected_digest; int omit_vendor_cert; SECItem newsig;