Skip to content

Commit

Permalink
cms: store digest as pointer instead of index
Browse files Browse the repository at this point in the history
Storage as an index is problematic because the sentinel value -1 was
used, but accesses were unchecked, leading to crashes like that in
3b1031a ("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 <[email protected]>
frozencemetery committed Jun 10, 2022
1 parent 3b1031a commit 926782c
Showing 7 changed files with 39 additions and 42 deletions.
15 changes: 8 additions & 7 deletions src/certdb.c
Original file line number Diff line number Diff line change
@@ -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;
}
}
34 changes: 10 additions & 24 deletions src/cms_common.c
Original file line number Diff line number Diff line change
@@ -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;
13 changes: 12 additions & 1 deletion src/cms_common.h
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@
#include <secpkcs7.h>

#include <errno.h>
#include <efivar.h>
#include <signal.h>
#include <stdarg.h>
#include <sys/types.h>
@@ -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;
4 changes: 2 additions & 2 deletions src/content_info.c
Original file line number Diff line number Diff line change
@@ -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");
2 changes: 1 addition & 1 deletion src/file_kmod.c
Original file line number Diff line number Diff line change
@@ -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,
9 changes: 5 additions & 4 deletions src/file_pe.c
Original file line number Diff line number Diff line change
@@ -114,17 +114,18 @@ check_inputs(pesign_context *ctx)
static void
print_digest(pesign_context *pctx)
{
unsigned int i;

if (!pctx)
return;

cms_context *ctx = pctx->cms_ctx;
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);
}

4 changes: 1 addition & 3 deletions src/pesigcheck.c
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 926782c

Please sign in to comment.