Djam/arm-trusted-firmware
1
0
Fork 0
mirror of https://github.com/ARM-software/arm-trusted-firmware.git synced 2025-04-16 09:34:18 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

cert-tool: avoid duplicates in extension stack

Browse source 
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 <jimmy.brisson@arm.com>
This commit is contained in:
Jimmy Brisson 2021-01-20 15:34:51 -06:00
parent 337e493306
commit 1ed941c0b0
1 changed files with 8 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
tools/cert_create/src/main.c
View file

@ -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 */