Skip to content

Comments

fix mem leaks in try_decode_PKCS12() of crypto/store/#11733

Closed
DDvO wants to merge 6 commits intoopenssl:masterfrom
siemens:fix_memleak_try_decode_PKCS12
Closed

fix mem leaks in try_decode_PKCS12() of crypto/store/#11733
DDvO wants to merge 6 commits intoopenssl:masterfrom
siemens:fix_memleak_try_decode_PKCS12

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 5, 2020

When loading PKCS#12 files that include a cert/ca chain the chain container was not deallocated.
Fixing this I improved the layout of crypto/store/loader_file.c to satisfy check-format.pl -l.

@DDvO DDvO requested a review from levitte May 5, 2020 09:36
@DDvO
Copy link
Contributor Author

DDvO commented May 5, 2020

BTW, the mem leak came up experimenting how to use the OSSL_STORE API as requested by @mattcaswell for the CMP contribution chunk 11: #11470 (comment)

It indicates that the OSSL_STORE API is not sufficiently tested. In fact, I can't find any tests for it.

mattcaswell
mattcaswell requested changes May 5, 2020
View reviewed changes
@mattcaswell
Copy link
Member

mattcaswell commented May 5, 2020

It indicates that the OSSL_STORE API is not sufficiently tested. In fact, I can't find any tests for it.

There are lots of store tests in 90-test_store.t - but they all test via the command line store app rather than the API directly.

@DDvO DDvO force-pushed the fix_memleak_try_decode_PKCS12 branch from 9c3bf21 to 7fb97e8 Compare May 5, 2020 12:58
@DDvO DDvO changed the title fix memory leak in try_decode_PKCS12() of crypto/store/ fix mem leaks in try_decode_PKCS12() of crypto/store/ May 5, 2020
@DDvO
Copy link
Contributor Author

DDvO commented May 5, 2020 •
edited
Loading

It indicates that the OSSL_STORE API is not sufficiently tested. In fact, I can't find any tests for it.

There are lots of store tests in 90-test_store.t - but they all test via the command line store app rather than the API directly.

Ah, right.
I've made sure that these tests can run also if DSA and/or EC key generation does not work
(BTW, I had to disable tests based on pbeWithMD5AndDES-CBC apparently due to deprecation)
and added -chain -CAfile cacert.pem to the openssl pkcs12 -export calls
such that memory leaks etc. are caught also in case they are due to a non-empty chain.

@DDvO
Copy link
Contributor Author

DDvO commented May 5, 2020

Currently I always see one Travis CI failure like this:

 No space left on device at ./Configure line 1722.

@DDvO DDvO closed this May 5, 2020
@DDvO DDvO reopened this May 5, 2020
@DDvO
Copy link
Contributor Author

DDvO commented May 5, 2020 •
edited
Loading

No space left on device at ./Configure line 1722.

I'm not convinced that my mem leak fixes solved this 😜

levitte
levitte reviewed May 5, 2020
View reviewed changes
mattcaswell
mattcaswell requested changes May 6, 2020
View reviewed changes
@DDvO
Copy link
Contributor Author

DDvO commented May 6, 2020

I've meanwhile adapted OSSL_STORE_close() such that it succeeds gracefully when given a NULL argument, like any free() function does.

@DDvO
Copy link
Contributor Author

DDvO commented May 6, 2020

Note that I also fixed some issues in the OSSL_STORE documentation.

DDvO added a commit to mpeylo/cmpossl that referenced this pull request May 12, 2020
@DDvO
Preliminary fix of memory leak in try_decode_PKCS12() - full fix is in …
95d0923 
…openssl#11733
DDvO added a commit to mpeylo/cmpossl that referenced this pull request May 12, 2020
@DDvO
Preliminary fix of memory leak in try_decode_PKCS12() - full fix is in …
1e9221a 
…openssl#11733
@DDvO DDvO mentioned this pull request May 12, 2020
Closed
openssl-machine pushed a commit that referenced this pull request May 13, 2020
@DDvO
Preliminary fix of memory leak in try_decode_PKCS12() - full fix is in …
3c38fa4 
…#11733

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #11470)
t8m
t8m reviewed May 19, 2020
View reviewed changes
@DDvO DDvO force-pushed the fix_memleak_try_decode_PKCS12 branch from d6c6a08 to 3f71959 Compare May 22, 2020 11:26
@DDvO DDvO mentioned this pull request May 22, 2020
Closed
@levitte
Copy link
Member

levitte commented May 23, 2020

Regarding OSSL_STORE_close() being a kind of free(), it's semantically closer to fclose(). What's the result of fclose(NULL)? 😉

@DDvO
Copy link
Contributor Author

DDvO commented May 23, 2020

Regarding OSSL_STORE_close() being a kind of free(), it's semantically closer to fclose(). What's the result of fclose(NULL)? wink

I know that the semantics of fclose(NULL) is officially undefined,
and I believe that this is purely for historical reasons,
IMHO it is preferrable to define the behavior in this case as no-op.
For OSSL_STORE_close() we have the chance to do better 😄

@DDvO
Copy link
Contributor Author

DDvO commented May 23, 2020

I've just done the same enhancements of OSSL_STORE_open.pod as in #11912.

@levitte
Copy link
Member

levitte commented May 24, 2020

For commit messages, I disagree with "Allow NULL argument (as with *_free()) to OSSL_STORE_close()". Please make it "Allow NULL argument to OSSL_STORE_close()".
My reasoning is that although OSSL_STORE_close() does free its argument, it's semantically closer to fclose(), the freeing part is just an implementation detail.

t8m
t8m previously approved these changes Jun 2, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me as is, is there anything unresolved?

@t8m
Copy link
Member

t8m commented Jun 2, 2020

The commit message can be amended without a re-review or even during the merge.

@t8m t8m added the approval: done This pull request has the required number of approvals label Jun 2, 2020
@DDvO DDvO force-pushed the fix_memleak_try_decode_PKCS12 branch from 2536a42 to 68c19ee Compare June 5, 2020 10:23
@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

Meanwhile I've added also $use_des and successfully tested various combinations of
negating $use_md5, $use_des, $use_dsa, and $use_ecc.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

I also found that, due to the so far 'successful' skip of the tests in case their init failed,
the test cert generation using the rather new CA configuration file test/ca-and-certs.cnf
went wrong without being noticed by its author (apparently @richsalz in commit 4e6e57c).
I've corrected this as well and made sure that the tests fail if their init fails,
to prevent such obscure failure issues for the future.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

@levitte wrote:

Will need re-review

Yes. Now the PR is ready for that.

@t8m
Copy link
Member

t8m commented Jun 5, 2020

For some reason Travis build is not getting executed.

@DDvO DDvO closed this Jun 5, 2020
@DDvO DDvO reopened this Jun 5, 2020
@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

For some reason Travis build is not getting executed.

I kicked it by closing and re-opening the PR, but this did not help.
Apparently a more general issue?

@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

It looks like Travis just had a delay starting up. It's running again.

levitte
levitte requested changes Jun 5, 2020
View reviewed changes
@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 5, 2020
@DDvO
Copy link
Contributor Author

DDvO commented Jun 5, 2020

There was Travis failure, which pointed out that PKCS#12 generation is not possible in case no-des.
Fixup committed.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 6, 2020 •
edited
Loading

It turns out that there was another init failure in case no-des, which I've handled now.
@levitte, I meanwhile understand why when authoring 90-test_store.t you took the shortcut of simply letting the tests 'pass' (for void) if anything went wrong in init() 😉
which had the negative effect that several regressions went unnoticed.

I'm confident that now all relevant algorithm exclusion cases are finally sorted out and ask you to re-confirm your approval.

@openssl-machine
Copy link
Collaborator

openssl-machine commented Jun 6, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@DDvO
Fix 90-test_store.t for latest config, limits, providers, and disable…
d438c9a 
…d algos

Also make sure that the test do not 'pass' if their initialization fails.
Leave out the expensive parts of DSA key gen and RSA keygen for efficiency.
Fix use of the new CA configuration file test/ca-and-certs.cnf.
@DDvO DDvO force-pushed the fix_memleak_try_decode_PKCS12 branch from 04ea2c6 to d438c9a Compare June 6, 2020 18:12
@DDvO
Copy link
Contributor Author

DDvO commented Jun 6, 2020

Thanks @levitte for the swift re-review and re-confirmation.
I've just rebased and squashed the fixups (to be merged on Monday morning).

openssl-machine pushed a commit that referenced this pull request Jun 8, 2020
@DDvO
Fix code layout in crypto/store/loader_file.c satisfying check-format…
9f5ff44 
….pl -l

Reviewed-by: Richard Levitte <[email protected]>
(Merged from #11733)
openssl-machine pushed a commit that referenced this pull request Jun 8, 2020
@DDvO
Add chain to PKCS#12 test file generation in 90-test_store.t
4ca0155 
Reviewed-by: Richard Levitte <[email protected]>
(Merged from #11733)
openssl-machine pushed a commit that referenced this pull request Jun 8, 2020
@DDvO
Fix mem leaks and allow missing pkey and/or cert in try_decode_PKCS12()
c29ba6a 
Reviewed-by: Richard Levitte <[email protected]>
(Merged from #11733)
openssl-machine pushed a commit that referenced this pull request Jun 8, 2020
@DDvO
Fix documentation of OSSL_STORE
49f8400 
Among others, make clear that OSSL_STORE_close() meanwhile does nothing on NULL.

Reviewed-by: Richard Levitte <[email protected]>
(Merged from #11733)
openssl-machine pushed a commit that referenced this pull request Jun 8, 2020
@DDvO
Fix 90-test_store.t for latest config, limits, providers, and disable…
71273ab 
…d algos

Also make sure that the test do not 'pass' if their initialization fails.
Leave out the expensive parts of DSA key gen and RSA keygen for efficiency.
Fix use of the new CA configuration file test/ca-and-certs.cnf.

Reviewed-by: Richard Levitte <[email protected]>
(Merged from #11733)
@DDvO
Copy link
Contributor Author

DDvO commented Jun 8, 2020

Merged - thanks!

@DDvO DDvO closed this Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell requested changes

@levitte levitte levitte approved these changes

@t8m t8m t8m left review comments

+1 more reviewer

@richsalz richsalz richsalz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@DDvO @mattcaswell @levitte @t8m @openssl-machine @richsalz