Skip to content

Commit

Permalink
CMS: make cms->selected_digest an index (again)
Browse files Browse the repository at this point in the history 
In 926782c, we switched
cms->selected_digest to be a pointer to the entry in cms->digests.

Because cms->digests is lazily allocated, setting the selected_digest
pointer has to be done at the right part of the CMS context life cycle,
and in some cases it clearly is not:

==334217== Command: ./src/pesign -n tmp -s --pinfile tmp/pinfile -t OpenSC\ Card\ (testcard) -c kernel-signer -i tmp/unsigned.efi -o tmp/signed.efi --force
==334217==
==334217== Invalid read of size 8
==334217==    at 0x115E7D: digest_get_digest_oid (cms_common.c:59)
==334217==    by 0x11CF41: generate_algorithm_id_list (signed_data.c:33)
==334217==    by 0x11D348: generate_spc_signed_data (signed_data.c:279)
==334217==    by 0x11EDFD: calculate_signature_space (wincert.c:297)
==334217==    by 0x11467D: pe_handle_action (file_pe.c:298)
==334217==    by 0x10F962: main (pesign.c:585)
==334217==  Address 0x10 is not stack'd, malloc'd or (recently) free'd
==334217==
==334217==
==334217== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==334217==  Access not within mapped region at address 0x10
==334217==    at 0x115E7D: digest_get_digest_oid (cms_common.c:59)
==334217==    by 0x11CF41: generate_algorithm_id_list (signed_data.c:33)
==334217==    by 0x11D348: generate_spc_signed_data (signed_data.c:279)
==334217==    by 0x11EDFD: calculate_signature_space (wincert.c:297)
==334217==    by 0x11467D: pe_handle_action (file_pe.c:298)
==334217==    by 0x10F962: main (pesign.c:585)
==334217==  If you believe this happened as a result of a stack
==334217==  overflow in your program's main thread (unlikely but
==334217==  possible), you can try to increase the size of the
==334217==  main thread stack using the --main-stacksize= flag.
==334217==  The main thread stack size used in this run was 8388608.
==334217==
==334217== HEAP SUMMARY:
==334217==     in use at exit: 588,544 bytes in 4,388 blocks
==334217==   total heap usage: 8,568 allocs, 4,180 frees, 2,077,115 bytes allocated
==334217==
==334217== LEAK SUMMARY:
==334217==    definitely lost: 25 bytes in 1 blocks
==334217==    indirectly lost: 0 bytes in 0 blocks
==334217==      possibly lost: 51,378 bytes in 166 blocks
==334217==    still reachable: 537,141 bytes in 4,221 blocks
==334217==                       of which reachable via heuristic:
==334217==                         length64           : 321,312 bytes in 590 blocks
==334217==         suppressed: 0 bytes in 0 blocks
==334217== Rerun with --leak-check=full to see details of leaked memory
==334217==
==334217== For lists of detected and suppressed errors, rerun with: -s
==334217== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

There is also a similar issue in the daemon code, and how to fix it
there is not immediately clear to me.

Currently, we realistically only support using sha256 digests, so for
now I've chosen to paper over the issue by switching back to
cms->selected_digest be an index into both ctx->digests and
digest_params, but switching the default value from -1 to 0, aka
DIGEST_PARAM_SHA256.  We can revisit this issue later whenever we add
sha384 support (or whichever other digest).

Signed-off-by: Peter Jones <pjones@redhat.com>
  • Loading branch information
@vathpela
vathpela committed Aug 31, 2022
1 parent 40f221e commit 45d6cb7
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 22 deletions.
2 changes: 1 addition & 1 deletion src/certdb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ check_hash(pesigcheck_context *ctx, SECItem *sig, efi_guid_t *sigtype,
efi_guid_t efi_sha1 = efi_guid_sha1;
void *digest_data;
struct digest *digests = ctx->cms_ctx->digests;
int selected_digest = -1;
unsigned int selected_digest;
size_t size;

if (memcmp(sigtype, &efi_sha256, sizeof(efi_guid_t)) == 0) {
Expand Down
41 changes: 23 additions & 18 deletions src/cms_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@

#include "hex.h"

/*
* Note that cms->selected_digest defaults to 0, which means the first
* entry of this array is the default digest.
*/
const struct digest_param digest_params[] = {
[DIGEST_PARAM_SHA256] = {
.name = "sha256",
Expand All @@ -53,33 +57,33 @@ const struct digest_param digest_params[] = {
},
#endif
};
const int n_digest_params = sizeof (digest_params) / sizeof (digest_params[0]);
const unsigned int n_digest_params = sizeof (digest_params) / sizeof (digest_params[0]);

SECOidTag
digest_get_digest_oid(cms_context *cms)
{
int i = cms->selected_digest;
unsigned int i = cms->selected_digest;
return digest_params[i].digest_tag;
}

SECOidTag
digest_get_encryption_oid(cms_context *cms)
{
int i = cms->selected_digest;
unsigned int i = cms->selected_digest;
return digest_params[i].digest_encryption_tag;
}

SECOidTag
digest_get_signature_oid(cms_context *cms)
{
int i = cms->selected_digest;
unsigned int i = cms->selected_digest;
return digest_params[i].signature_tag;
}

int
digest_get_digest_size(cms_context *cms)
{
int i = cms->selected_digest;
unsigned int i = cms->selected_digest;
return digest_params[i].size;
}

Expand All @@ -91,7 +95,7 @@ teardown_digests(cms_context *ctx)
if (!digests)
return;

for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
if (digests[i].pk11ctx) {
PK11_Finalize(digests[i].pk11ctx);
PK11_DestroyContext(digests[i].pk11ctx, PR_TRUE);
Expand Down Expand Up @@ -135,7 +139,7 @@ cms_context_init(cms_context *cms)
if (!cms->arena)
cnreterr(-1, cms, "could not create cryptographic arena");

cms->selected_digest = -1;
cms->selected_digest = DEFAULT_DIGEST_PARAM;

INIT_LIST_HEAD(&cms->pk12_ins);
cms->pk12_out.fd = -1;
Expand Down Expand Up @@ -219,7 +223,7 @@ cms_context_fini(cms_context *cms)
memset(&cms->newsig, '\0', sizeof (cms->newsig));
}

cms->selected_digest = -1;
cms->selected_digest = DEFAULT_DIGEST_PARAM;

if (cms->ci_digest) {
free_poison(cms->ci_digest->data, cms->ci_digest->len);
Expand Down Expand Up @@ -342,15 +346,15 @@ int
set_digest_parameters(cms_context *cms, char *name)
{
if (strcmp(name, "help")) {
for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
if (!strcmp(name, digest_params[i].name)) {
cms->selected_digest = i;
return 0;
}
}
} else {
printf("Supported digests: ");
for (int i = 0; digest_params[i].name != NULL; i++) {
for (unsigned int i = 0; digest_params[i].name != NULL; i++) {
printf("%s ", digest_params[i].name);
}
printf("\n");
Expand Down Expand Up @@ -1265,7 +1269,7 @@ generate_digest_begin(cms_context *cms)
cnreterr(-1, cms, "could not allocate digest context");
}

for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
digests[i].pk11ctx = PK11_CreateDigestContext(
digest_params[i].digest_tag);
if (!digests[i].pk11ctx)
Expand All @@ -1278,7 +1282,7 @@ generate_digest_begin(cms_context *cms)
return 0;

err:
for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
if (digests[i].pk11ctx)
PK11_DestroyContext(digests[i].pk11ctx, PR_TRUE);
}
Expand All @@ -1290,7 +1294,7 @@ generate_digest_begin(cms_context *cms)
void
generate_digest_step(cms_context *cms, void *data, size_t len)
{
for (int i = 0; i < n_digest_params; i++)
for (unsigned int i = 0; i < n_digest_params; i++)
PK11_DigestOp(cms->digests[i].pk11ctx, data, len);
}

Expand All @@ -1299,7 +1303,7 @@ generate_digest_finish(cms_context *cms)
{
void *mark = PORT_ArenaMark(cms->arena);

for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
SECItem *digest = PORT_ArenaZAlloc(cms->arena,sizeof (SECItem));
if (digest == NULL)
cngotoerr(err, cms, "could not allocate memory");
Expand All @@ -1326,7 +1330,7 @@ generate_digest_finish(cms_context *cms)
PORT_ArenaUnmark(cms->arena, mark);
return 0;
err:
for (int i = 0; i < n_digest_params; i++) {
for (unsigned int i = 0; i < n_digest_params; i++) {
if (cms->digests[i].pk11ctx)
PK11_DestroyContext(cms->digests[i].pk11ctx, PR_TRUE);
}
Expand All @@ -1343,12 +1347,13 @@ int
generate_signature(cms_context *cms)
{
int rc = 0;
int i = cms->selected_digest;

if (cms->digests[cms->selected_digest].pe_digest == NULL)
if (cms->digests[i].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->digests[i].pe_digest->data,
cms->digests[i].pe_digest->len))
cnreterr(-1, cms, "PE binary has not been digested");

SECItem sd_der;
Expand Down
5 changes: 3 additions & 2 deletions src/cms_common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ struct digest {

#define DIGEST_PARAM_SHA256 0
#define DIGEST_PARAM_SHA1 1
#define DEFAULT_DIGEST_PARAM DIGEST_PARAM_SHA256

struct digest_param {
char *name;
Expand All @@ -76,7 +77,7 @@ struct digest_param {
};

extern const struct digest_param digest_params[2];
extern const int n_digest_params;
extern const unsigned int n_digest_params;

typedef struct pk12_file {
char *path;
Expand Down Expand Up @@ -149,7 +150,7 @@ typedef struct cms_context {
int db_out, dbx_out, dbt_out;

struct digest *digests;
int selected_digest;
unsigned int selected_digest;
int omit_vendor_cert;

SECItem newsig;
Expand Down
2 changes: 1 addition & 1 deletion src/content_info.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ 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;
unsigned 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)) {
Expand Down

0 comments on commit 45d6cb7

Please sign in to comment.