Coverity fixes from last run (though still not all of them) #982

Jakuje · 2017-03-01T07:54:11Z

card-cac.c

CLANG_WARNING: The left operand of '<' is a garbage value

card-coolkey.c

CLANG_WARNING: overwriting variable
CPPCHECK_WARNING: memory leak / overwrite variable
CLANG_WARNING: null pointer dereference
UNUSED_VALUE: unused return value

card-gids.c

CLANG_WARNING: Branch condition evaluates to a garbage value
SIZEOF_MISMATCH: suspicious_sizeof
card-myeid.c
RESOURCE_LEAK: Variable "buf" going out of scope leaks the storage it points to.
CLANG_WARNING: overwriting variable
(rewrite not to confuse coverity)

pkcs15-cac.c

RESOURCE_LEAK: Variable "cert_out" going out of scope leaks the storage it points to.

pkcs15-coolkey.c

UNUSED_VALUE: unused return value

pkcs15-piv.c

RESOURCE_LEAK: Variable "cert_out" going out of scope leaks the storage it points to.

pkcs15-sc-hsm.c

DEADCODE

pkcs11/framework-pkcs15.c

RESOURCE_LEAK: Variable "p15_cert" going out of scope leaks the storage it points to.

pkcs15init/pkcs15-lib.c

CLANG_WARNING: Assigned value is garbage or undefined

pkcs15init/pkcs15-myeid.c

UNREACHABLE: Probably wrong placement of code block

tests/p15dump.c

IDENTICAL_BRANCHES

pkcs15-init.c

CLANG_WARNING: Potential leak of memory pointed to by 'args.der_encoded.value'

pkcs15-tool.c

RESOURCE_LEAK: Variable "cert" going out of scope leaks the storage it points to.
MISSING_BREAK: The above case falls through to this one.

sc-hsm-tool.c

CLANG_WARNING: Potential leak of memory pointed to by 'sp'

westcos-tool.c

FORWARD_NULL: Passing null pointer "pin" to "unlock_pin", which dereferences it.
(rewrite not to confuse coverity)

card-cac.c * CLANG_WARNING: The left operand of '<' is a garbage value card-coolkey.c * CLANG_WARNING: overwriting variable * CPPCHECK_WARNING: memory leak / overwrite variable * CLANG_WARNING: null pointer dereference * UNUSED_VALUE: unused return value card-gids.c * CLANG_WARNING: Branch condition evaluates to a garbage value * SIZEOF_MISMATCH: suspicious_sizeof card-myeid.c * RESOURCE_LEAK: Variable "buf" going out of scope leaks the storage it points to. * CLANG_WARNING: overwriting variable * (rewrite not to confuse coverity) pkcs15-cac.c * RESOURCE_LEAK: Variable "cert_out" going out of scope leaks the storage it points to. pkcs15-coolkey.c * UNUSED_VALUE: unused return value pkcs15-piv.c * RESOURCE_LEAK: Variable "cert_out" going out of scope leaks the storage it points to. pkcs15-sc-hsm.c * DEADCODE pkcs11/framework-pkcs15.c * RESOURCE_LEAK: Variable "p15_cert" going out of scope leaks the storage it points to. pkcs15init/pkcs15-lib.c * CLANG_WARNING: Assigned value is garbage or undefined pkcs15init/pkcs15-myeid.c * UNREACHABLE: Probably wrong placement of code block tests/p15dump.c * IDENTICAL_BRANCHES pkcs15-init.c * CLANG_WARNING: Potential leak of memory pointed to by 'args.der_encoded.value' pkcs15-tool.c * RESOURCE_LEAK: Variable "cert" going out of scope leaks the storage it points to. * MISSING_BREAK: The above case falls through to this one. sc-hsm-tool.c * CLANG_WARNING: Potential leak of memory pointed to by 'sp' westcos-tool.c * FORWARD_NULL: Passing null pointer "pin" to "unlock_pin", which dereferences it. * (rewrite not to confuse coverity)

frankmorgner

Thanks, good work!

When calling sc_pkcs15_free_certificate with NULL, we will hit an assert. So you should check for NULL before or remove the assert in the function.

frankmorgner · 2017-03-01T08:32:10Z

src/libopensc/pkcs15-cac.c

@@ -352,12 +352,14 @@ static int sc_pkcs15emu_cac_init(sc_pkcs15_card_t *p15card)
 r = sc_pkcs15_read_certificate(p15card, &cert_info, &cert_out);
 if (r < 0 || cert_out->key == NULL) {
 sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, "Failed to read/parse the certificate r=%d",r);
+ sc_pkcs15_free_certificate(cert_out);


if cert_out is NULL, then we will hit an assert in sc_pkcs15_free_certificate. So you should check for NULL here or remove the assert.

Yes, I will add the null check here to make it sane. Thanks for pointing it out (already fixed in the other case).

frankmorgner · 2017-03-01T08:35:12Z

src/pkcs15init/pkcs15-myeid.c

 if (sc_card_find_ec_alg(p15card->card, keybits, &prkey->u.ec.params.id) == NULL)
 LOG_TEST_RET(ctx, SC_ERROR_INVALID_ARGUMENTS, "Unsupported algorithm or key size"); 
+ break;


This change should be confirmed with an actual card. @mtrojnar or @hhonkanen could you have a look?

Thank you for pointing to appropriate people to discuss this change with.

The LOG_TEST_RET(..., SC_ERROR_INVALID_ARGUMENTS, ...) macro always returns SC_ERROR_INVALID_ARGUMENTS, so break; is redundant (does not do anything) here. The code would be more clear if SC_FUNC_RETURN() was used instead of LOG_TEST_RET().

@mtrojnar have a look on the full diff of the file. Originally, the break was before the sc_card_find_ec_alg() so it was not even called.

Also if sc_card_find_ec_alg() succeed, it would fall through to the default of the switch, which is certainly not intended.

The change looks sensible and the break was clearly in wrong place. In the previous function "myeid_create_key", there is the same check implemented correctly and I am 95% sure that this is ok, because I have tested this code. Please look at lines 440-447. I can test with an actual card but right now I am under high workload so it might take a while before I have the chance.

@hhonkanen did you have time to test the change?

Not yet, but I am trying to find some time for OpenSC dev this and next week. I will confirm this change after I have tested.

I tested myeid_store_key as follows: Using pkcs15-init tool I imported a 521 bit ECC key into a card that supports 521 bit keys. Traced through this function with debugger and it goes fine, the key was imported into the card.

Tested the same command with a card that supports ECC only up to 256 bit keys. We don't even get into this function, OpenSC notices earlier that the key size is not listed as supported and returns with error.

OK, thanks. Sounds good enough for me.

dengert · 2017-03-01T13:53:25Z

The comment from @frankmorgner :
When calling sc_pkcs15_free_certificate with NULL, we will hit an assert. So you should check for NULL before or remove the assert in the function.:

also applies to the change to card-piv.c and needs a change like e73b2ad

Jakuje · 2017-03-01T14:34:54Z

banging my head against the table. Obviously. Thanks for patience :)

frankmorgner · 2017-03-23T15:23:20Z

src/scconf/scconf.c

@@ -411,6 +411,8 @@ char *scconf_list_strdup(const scconf_list * list, const char *filler)
 if (filler) {
 len += scconf_list_array_length(list) * (strlen(filler) + 1);
 }
+ if (len == 0)
+ return NULL;
 buf = malloc(len);


according to the Linux manual, calling malloc with 0 should be fine, but that may be different on other platforms.

frankmorgner · 2017-03-23T15:24:47Z

src/pkcs15init/pkcs15-myeid.c

 if (sc_card_find_ec_alg(p15card->card, keybits, &prkey->u.ec.params.id) == NULL)
 LOG_TEST_RET(ctx, SC_ERROR_INVALID_ARGUMENTS, "Unsupported algorithm or key size"); 
+ break;


@hhonkanen did you have time to test the change?

frankmorgner requested changes Mar 1, 2017

View reviewed changes

Sanitize call to sc_pkcs15_free_certificate()

e73b2ad

Sanitize call to sc_pkcs15_free_certificate() in PIV too

bdf4522

Jakuje added 3 commits March 23, 2017 14:59

Coverity: FORWARD_NULL -- copy&paste error

389ffe5

CLANG_WARNING: Call to 'malloc' has an allocation size of 0 bytes

1133efa

Avoid malloc with 0 argument

f82bc20

frankmorgner reviewed Mar 23, 2017

View reviewed changes

frankmorgner approved these changes Mar 23, 2017

View reviewed changes

frankmorgner added enhancement Input needed labels Mar 23, 2017

frankmorgner closed this in c5e4012 Apr 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coverity fixes from last run (though still not all of them) #982

Coverity fixes from last run (though still not all of them) #982

Jakuje commented Mar 1, 2017 •

edited

Loading

frankmorgner left a comment

frankmorgner Mar 1, 2017

Jakuje Mar 1, 2017

frankmorgner Mar 1, 2017

Jakuje Mar 1, 2017 •

edited

Loading

mtrojnar Mar 1, 2017

Jakuje Mar 1, 2017

hhonkanen Mar 2, 2017

frankmorgner Mar 23, 2017

hhonkanen Mar 28, 2017

frankmorgner Mar 28, 2017

hhonkanen Mar 31, 2017

frankmorgner Mar 31, 2017

dengert commented Mar 1, 2017

Jakuje commented Mar 1, 2017

frankmorgner Mar 23, 2017

frankmorgner Mar 23, 2017

Coverity fixes from last run (though still not all of them) #982

Coverity fixes from last run (though still not all of them) #982

Conversation

Jakuje commented Mar 1, 2017 • edited Loading

frankmorgner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jakuje Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dengert commented Mar 1, 2017

Jakuje commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jakuje commented Mar 1, 2017 •

edited

Loading

Jakuje Mar 1, 2017 •

edited

Loading