Add fuzzer for testing pkcs15init functionality #2500
Conversation
Otherwise unchecked access to fi_path.value/ffi_path.value leads to read out of borders.
When profile->p15_spec is NULL
`do_erase()` function calls `sc_pkcs15init_erase_card()`, that gets pointer to sc_pkcs15_card as argument, but after calling `sc_pkcs15_bind()` overwrites this pointer with newly allocated sc_pkcs15_card, which is then stored in profile->p15_data
In sc_pkcs15init_init_prkdf() allocate sc_ec_parameters and do not use the struct from variable on stack.
xhanulik
force-pushed
the
pkcs15init_fuzz
branch
from
3c7f0d7 to
eb72297
Compare
By creating new object in sc_pkcs15init_init_prkdf(), ecparams will be copied into new allocated structure and freed with object in sc_pkcs15_free_object(). Name of the curve in pkcs15-init.c needs to be freed separately after sc_pkcs15init_generate_key() ends.
src/pkcs15init/pkcs15-lib.c
Outdated
| object = NULL; | ||
| r = SC_SUCCESS; | ||
|
|
||
| LOG_FUNC_RETURN(ctx, r); |
There was a problem hiding this comment.
The above two lines can be simplified as LOG_FUNC_RETURN(ctx, SC_SUCCESS);.
There was a problem hiding this comment.
The similar pattern is used in other functions you changed in the pkcs15-lib.c. Can you check also these? Either by using SC_SUCCESS explicitly or falling through to the only return, what would make more sense.
xhanulik
force-pushed
the
pkcs15init_fuzz
branch
from
2e8a695 to
73ee1f5
Compare
xhanulik
force-pushed
the
pkcs15init_fuzz
branch
2 times, most recently
from
f468c27 to
d2d8fa9
Compare
xhanulik
force-pushed
the
pkcs15init_fuzz
branch
from
5cdded0 to
d2da95b
Compare
src/tools/pkcs15-init.c
Outdated
| sc_unlock(g_p15card->card); | ||
| if (algorithm == SC_ALGORITHM_EC) | ||
| /* allocated in parse_alg_spec() */ | ||
| free(keygen_args.prkey_args.key.u.ec.params.named_curve); |
There was a problem hiding this comment.
It looks strange to have this here.
In parse_alg_spec(), would it cause any problems if we take (part of) the user supplied spec for u.ec.params.named_curve avoiding the strdup() in the first place?
There was a problem hiding this comment.
I replaced the free with sc_pkcs15_free_prkey(). Removing strdup() would probably also mean to remove const from spec param and I!m not sure about all places where named_curve is used then.
|
The CI fuzzer also shows some other problem that's still present. |
|
@frankmorgner do you have some more comments or can we merge this? It looks like the new fuzzer is still a bit unstable, but unfortunately the CIFuzz did not provide reproducer so we will have to handle it as it on the go. |
|
It's fine, we can merge |
3 participants
This PR adds a fuzzer for testing functionality from the
pkcs15-inittool.It splits the input into two parts - the first is parsed as a profile file (via
fuzz_pkcs15init_bindthat works similarly tosc_pkcs15init_bind, but parsing the profile as string not as file). The second is set of APDUs for reader operations (as insc_pkcs15_reader.c). Then there aredo_*functions working as some of the operations inpkcs15-init(sc_pkcs15init_*for storing pin, data, generating keys and erasing card).Corpus file contains profile part put together from
pkcs15.profileandmyeid.profile, zero byte for separation and APDUs from MyEID card (generation described inREADMEfile by @Jakuje) trimmed to fit operations.Also, there are some fixes for locally found bugs, mainly memory leaks.
Checklist