From 1ed941c0b0ff89980dd421d720ec7d9b00c71d57 Mon Sep 17 00:00:00 2001 From: Jimmy Brisson Date: Wed, 20 Jan 2021 15:34:51 -0600 Subject: [PATCH] cert-tool: avoid duplicates in extension stack This bug manifests itself as a segfault triggered by a double-free. I noticed that right before the double-free, the sk list contained 2 elements with the same address. (gdb) p sk_X509_EXTENSION_value(sk, 1) $34 = (X509_EXTENSION *) 0x431ad0 (gdb) p sk_X509_EXTENSION_value(sk, 0) $35 = (X509_EXTENSION *) 0x431ad0 (gdb) p sk_X509_EXTENSION_num(sk) $36 = 2 This caused confusion; this should never happen. I figured that this was caused by a ext_new_xxxx function freeing something before it is added to the list, so I put a breakpoint on each of them to step through. I was suprised to find that none of my breakpoints triggered for the second element of the iteration through the outer loop just before the double-free. Looking through the code, I noticed that it's possible to avoid doing a ext_new_xxxx, when either: * ext->type == NVCOUNTER and ext->arg == NULL * ext->type == HASH and ext->arg == NULL and ext->optional == false So I put a breakpoint on both. It turns out that it was the HASH version, but I added a fix for both. The fix for the Hash case is simple, as it was a mistake. The fix for the NVCOUNTER case, however, is a bit more subtle. The NVCOUNTER may be optional, and when it's optional we can skip it. The other case, when the NVCOUNTER is required (not optinal), the `check_cmd_params` function has already verified that the `ext->arg` must be non-NULL. We assert that before processing it to covert any possible segfaults into more descriptive errors. This should no longer cause double-frees by adding the same ext twice. Change-Id: Idae2a24ecd964b0a3929e6193c7f85ec769f6470 Signed-off-by: Jimmy Brisson --- tools/cert_create/src/main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/cert_create/src/main.c b/tools/cert_create/src/main.c index 2ba110132..1489cb06e 100644 --- a/tools/cert_create/src/main.c +++ b/tools/cert_create/src/main.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2020, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2021, ARM Limited and Contributors. All rights reserved. * * SPDX-License-Identifier: BSD-3-Clause */ @@ -492,7 +492,12 @@ int main(int argc, char *argv[]) */ switch (ext->type) { case EXT_TYPE_NVCOUNTER: - if (ext->arg) { + if (ext->optional && ext->arg == NULL) { + /* Skip this NVCounter */ + continue; + } else { + /* Checked by `check_cmd_params` */ + assert(ext->arg != NULL); nvctr = atoi(ext->arg); CHECK_NULL(cert_ext, ext_new_nvcounter(ext_nid, EXT_CRIT, nvctr)); @@ -505,7 +510,7 @@ int main(int argc, char *argv[]) memset(md, 0x0, SHA512_DIGEST_LENGTH); } else { /* Do not include this hash in the certificate */ - break; + continue; } } else { /* Calculate the hash of the file */ -- GitLab