Run tests also under valgrind #2756

Jakuje · 2023-04-21T16:22:41Z

~~The leaks in valgrind are not yet "enforced" with --error-exitcode=1 as it fails at several occasions that need to be fixed.~~

Checklist

PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

containers/opensc-test-piv/Containerfile

This is useful for running the CI locally in containers, which makes things much faster than it would need to download, build and install all the test tools.

There were needless assignment immediately rewritten with copy function, some of the structures were not copied and not freed from the target contexts and similar issues.

Reported by clang-tidy as set-but never read variable: /home/runner/work/OpenSC/OpenSC/src/libopensc/iso7816.c:366:2: warning: Value stored to 'r' is never read [clang-analyzer-deadcode.DeadStores] r = count; ^ /home/runner/work/OpenSC/OpenSC/src/libopensc/iso7816.c:366:2: note: Value stored to 'r' is never read

Accidentally introduced with 264b23a, reported by clang-tidy /home/runner/work/OpenSC/OpenSC/src/libopensc/card-westcos.c:664:7: warning: Although the value stored to 'cctx' is used in the enclosing expression, the value is never actually read from 'cctx' [clang-analyzer-deadcode.DeadStores] if ((cctx = EVP_CIPHER_CTX_new()) == NULL) ^ /home/runner/work/OpenSC/OpenSC/src/libopensc/card-westcos.c:664:7: note: Although the value stored to 'cctx' is used in the enclosing expression, the value is never actually read from 'cctx'

Reported by clang-tidy: /home/runner/work/OpenSC/OpenSC/src/libopensc/card-sc-hsm.c:854:3: warning: Value stored to 'len' is never read [clang-analyzer-deadcode.DeadStores] len = 0; ^ /home/runner/work/OpenSC/OpenSC/src/libopensc/card-sc-hsm.c:854:3: note: Value stored to 'len' is never read

Jakuje · 2023-05-19T15:21:39Z

This is getting to be ready for reviews. Now, valgrind is executed over the emulator tests as well as make check unit tests or shell tests, which surfaced several issues including reproducing the issue reported in #2775.

@frankmorgner can you have a look?

@swissbit-csteuer Can you have a look that the fixes for the isoapplet make sense?

I plan to remove the suppression file as there is nothing to suppress now (when we got rid of the pcsclite issues and issues brought in by glib/gobject/notifications), there will likely be some more issues from the isoapplet/openpgp, but I think this should be close to final version.

fixes OpenSC#2775

…estinfo This lead to using of uninitialized memory (tmp array) when talking to the pcsc and inevitable failure to verify the signature throughout the tests.

both tests executed during `make check` as well as the integration tests ran from the github CI with java card simulator or other software implementations.

swissbit-csteuer · 2023-05-22T07:46:32Z

src/pkcs15init/pkcs15-isoApplet.c

 	memcpy(pubkey->u.ec.ecpointQ.value, args.pubkey.ec.ecPointQ.value, args.pubkey.ec.ecPointQ.len);

 	/* The OID is also written to the pubkey->u.ec.params */
+	free(pubkey->u.ec.params.der.value);


Isn't that dangerous? We don't know where pubkey->u.ec.params.der.value points to and who "owns" it.

It would help if this would be a bit more described. In the past, the EC params were sometimes very wildly assigned to various structures without much care. I hope this has improved a bit over last year with the fuzzing and with this PR.

For this case, the pubkey comes from sc_pkcs15init_generate_key() function, where it is memset to zeroes and then the EC params are copied to that structure from the keygen params using sc_copy_ec_params() so it should not really be anything dangerous (unless somebody starts assigning here some dangerous values on the way).

Regarding the ec->params, we need to get a common understanding about memory owner ship and proper initialization/cleanup this is ture. But I'd like to postpone this for now to move on with all the good stuff in this PR.

Should I drop some of the patches/changes from the PR to get it merged (without these changes, the CI will most likely fail)? Or do you mean that we should just create a follow-up issue to review how the ec->params are handled in various cases?

swissbit-csteuer · 2023-05-22T08:51:40Z

src/libopensc/card-isoApplet.c

-		sc_pkcs1_strip_digest_info_prefix(NULL, data, datalen, tmp, &tmplen);
-		r = iso_ops->compute_signature(card, tmp, tmplen, out, outlen);
+		r = sc_pkcs1_strip_digest_info_prefix(NULL, data, datalen, tmp, &tmplen);
+		if (r == SC_SUCCESS) {
+			r = iso_ops->compute_signature(card, tmp, tmplen, out, outlen);
+		} else {
+			/* No digest info present? Use the value as it is */
+			r = iso_ops->compute_signature(card, data, datalen, out, outlen);
+		}


I think its better to return an error if the digest info is missing.

There is no digestinfo in case the RSA_PKCS_PSS mechanism is used (as visible in the modified p11test results). If you use the SHA*_RSA_PKCS_PSS mechanism, the input is either hashed inside of the card or in OpenSC, depending on the card capabilities, but with RSA_PKCS_PSS, the OpenSC gets the digest only with the length of hLen so there is no digestinfo involved (the digest is part of the PSS_PARAMS structure though):
https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html#_Toc30061139

At least that is how I covered this mechanism in the p11test and how it works with other cards so far. If you reading of the specs is different, feel free to correct me.

I'm reading the spec the same way, but I still do not understand how this has worked before where the return code of sc_pkcs1_strip_digest_info_prefix was not checked. Shouldn't the RSA_PKCS_PSS test cases in p11test have failed before that change?

I think they were failing, but given the vast amount of the combination for the RSA-PSS and that really no driver supports everything, the failures are silently ignored ...

Understood. Forwarding the data as-is to the card if no digest is present is the correct way then 👍

The valgrind macros set the VALGRIND variable without any arguments, which prevents it from failing if some issue is detected.

src/libopensc/card-openpgp.c

src/tests/p11test/p11test_case_common.c

frankmorgner

This pull request is great work, thank you!

frankmorgner · 2023-05-26T07:51:03Z

src/pkcs15init/pkcs15-isoApplet.c

 	memcpy(pubkey->u.ec.ecpointQ.value, args.pubkey.ec.ecPointQ.value, args.pubkey.ec.ecPointQ.len);

 	/* The OID is also written to the pubkey->u.ec.params */
+	free(pubkey->u.ec.params.der.value);


Regarding the ec->params, we need to get a common understanding about memory owner ship and proper initialization/cleanup this is ture. But I'd like to postpone this for now to move on with all the good stuff in this PR.

Jakuje · 2023-05-31T08:24:36Z

@frankmorgner can you clarify what you mean with the "good stuff" so we can move on with this? I see approvals but also some concerns in your last comment. Or should I merge it and open a follow-up issue to review the EC parameters ownership throughout the code?

frankmorgner · 2023-05-31T11:47:42Z

merge it and open a follow-up issue to review the EC parameters ownership throughout the code

Yes, that's what I wanted to say. thank you!

Jakuje · 2023-05-31T11:58:58Z

Thank you for the reviews! Merging. The new issue is #2790

frankmorgner · 2023-05-31T13:43:45Z

src/pkcs15init/pkcs15-lib.c

+				r = SC_ERROR_OUT_OF_MEMORY;
+				LOG_TEST_GOTO_ERR(ctx, r, "Cannot allocate EC params");
+			}
+			key.u.ec.ecpointQ.len = key.u.ec.ecpointQ.len;


CID 385152: Incorrect expression (NO_EFFECT) Assigning "key.u.ec.ecpointQ.len" to itself has no effect.

Fixed in #2791

Jakuje force-pushed the valgrind-ci branch 3 times, most recently from 648b958 to 5069f98 Compare April 24, 2023 18:38

Jakuje mentioned this pull request Apr 24, 2023

Integrate valgrind into CI tests #2736

Closed

frankmorgner requested changes Apr 24, 2023

View reviewed changes

containers/opensc-test-piv/Containerfile Show resolved Hide resolved

Jakuje force-pushed the valgrind-ci branch from 5069f98 to 7fc474a Compare April 25, 2023 15:37

Jakuje force-pushed the valgrind-ci branch 2 times, most recently from a9e02ad to fb2f07e Compare May 4, 2023 16:34

Jakuje mentioned this pull request May 4, 2023

Fix pkcs11-tool unwrap / incorrect CKA_ID #2765

Merged

Jakuje added 5 commits May 18, 2023 13:03

.editorconfig: Use tabs in shell scripts too

4698bf4

Remove trainling whitespace

603f3c4

tests: Extract the PIN to separate variable

1e59a6f

tests: Use separate file for the second cert

1d380a4

ci: Split code download and build

db0601f

This is useful for running the CI locally in containers, which makes things much faster than it would need to download, build and install all the test tools.

Jakuje force-pushed the valgrind-ci branch 3 times, most recently from 6f2bde4 to d2fa3b2 Compare May 18, 2023 19:05

Jakuje added 13 commits May 19, 2023 10:34

pkcs11-tool: Avoid memory leaks

9fcc84e

pkcs11-tool: Do not compare uninitialized memory if GetRandom fails

4a98ebf

containers: Sync with CI

3f38fb0

pkcs15init: Remove duplicate cleanup

2fa656f

p11test: Avoid memory leaks deteted by valgrind

684d993

pkcs15init: Revise cleanup after keygen and storing keys

2347801

There were needless assignment immediately rewritten with copy function, some of the structures were not copied and not freed from the target contexts and similar issues.

pkcs11: Free memory after storing keys

1e56c0e

isoapplet: Do not leak memory if params were allocated already

5f199c4

Mark clang-tidy warnings errors

1b669c3

tools: Avoid set-but-not-used variable in netkey-tool

b3a3641

Jakuje requested a review from frankmorgner May 19, 2023 15:13

Jakuje added 3 commits May 19, 2023 17:55

openpgp: Fix fingerprint calculation

e1b3631

fixes OpenSC#2775

isoapplet: Fix RSA-PSS calculation when the data does not contain dig…

22ecc68

…estinfo This lead to using of uninitialized memory (tmp array) when talking to the pcsc and inevitable failure to verify the signature throughout the tests.

tests: Update isoapplet v1 reference p11test log

e9dc24a

Jakuje force-pushed the valgrind-ci branch 3 times, most recently from dd1f634 to d5e5c24 Compare May 19, 2023 19:22

Jakuje added 3 commits May 19, 2023 21:59

openpgp: Fix modulus length calculation in pkc15init layer

3dc240b

openpgp: Avoid leaking file structure

7e4b9f2

tests: Support running under valgrind

55b17b3

both tests executed during `make check` as well as the integration tests ran from the github CI with java card simulator or other software implementations.

Jakuje force-pushed the valgrind-ci branch from d5e5c24 to 55b17b3 Compare May 19, 2023 20:00

swissbit-csteuer reviewed May 22, 2023

View reviewed changes

Jakuje added 3 commits May 22, 2023 14:47

tests: Properly propagate the VALGRIND env variable to the shell tests

32e3da3

The valgrind macros set the VALGRIND variable without any arguments, which prevents it from failing if some issue is detected.

pkcs11-tool: Avoid dereferencing freed members when reading public keys

5236a3e

tests: Check exit codes of helper functions

3258b5a

xhanulik reviewed May 25, 2023

View reviewed changes

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

src/libopensc/card-openpgp.c Outdated Show resolved Hide resolved

src/tests/p11test/p11test_case_common.c Outdated Show resolved Hide resolved

Jakuje added 2 commits May 25, 2023 16:15

fixup! p11test: Avoid memory leaks deteted by valgrind

17f0adb

fixup! openpgp: Free allocated memory

704d8ab

frankmorgner mentioned this pull request May 26, 2023

openpgp: fixed corruption during write of RSA key #2777

Closed

5 tasks

frankmorgner approved these changes May 26, 2023

View reviewed changes

swissbit-csteuer approved these changes May 26, 2023

View reviewed changes

Jakuje mentioned this pull request May 31, 2023

Review, clarify and document ownership of ec->params and similar throught the code #2790

Open

Jakuje merged commit 9f7e83f into OpenSC:master May 31, 2023

Jakuje mentioned this pull request May 31, 2023

Fix issues introduced with #2756 #2791

Merged

5 tasks

frankmorgner reviewed May 31, 2023

View reviewed changes

xhanulik mentioned this pull request Jul 3, 2023

New release 0.24.0 #2792

Closed

Run tests also under valgrind #2756

Run tests also under valgrind #2756

Uh oh!

Conversation

Jakuje commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Jakuje commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankmorgner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakuje commented May 31, 2023

Uh oh!

frankmorgner commented May 31, 2023

Uh oh!

Jakuje commented May 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jakuje commented Apr 21, 2023 •

edited

Loading

Jakuje commented May 19, 2023 •

edited

Loading