Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't forget the datatype when decoding a PEM file #13329

Closed
wants to merge 8 commits into from

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 5, 2020

The OSSL_STORE code was forgetting the datatype that we read from the
PEM header when decoding the DER.

Fixes #13046

WIP because it needs a test.

@levitte - I'm not sure where to put a test for this? Can you give me a hint?

@levitte
Copy link
Member

@levitte levitte commented Nov 5, 2020

@levitte - I'm not sure where to put a test for this? Can you give me a hint?

I would write a separate recipe for this, that generates a X9.42 and a DSA PEM file, and capture the output of openssl storeutl on them, and see that you get the BEGIN line you expect for each.

mattcaswell added 3 commits Nov 5, 2020
The OSSL_STORE code was forgetting the datatype that we read from the
PEM header when decoding the DER.

Fixes #13046
Add tests for various deprecated PEM_read_bio_*() functions to ensure
they can still read the various files.
There have been instances where OSSL_STORE got confused between DSA and
DH params (e.g. see issue #13046) due the DER encoding of DH and DSA params
looking identical. Therefore we test that we get the types that we expect.
@mattcaswell mattcaswell changed the title WIP: Don't forget the datatype when decoding a PEM file Don't forget the datatype when decoding a PEM file Nov 18, 2020
@mattcaswell mattcaswell force-pushed the mattcaswell:dont-forget-datatype branch to 8a2594f Nov 18, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 18, 2020

I've rebased this to fix a conflict with master and extended it with additional tests.

My first attempt at adding tests was to create a test that called various PEM_read_bio_* functions to check that we got the expected result back. This effectively does the same thing as the reproducer in #13046. To my surprise I found that the tests I had written were passing in master even without my fix applied!! On further investigation I found that issue in #13046 no longer exists. It was "fixed" by commit 35426b2. However the "fix" only addresses the symptoms by changing PEM_read_bio_DHparams() to not use OSSL_STORE anymore. The bug in OSSL_STORE remains.

Therefore I have kept the original tests that I wrote (they would have caught the issue before 35426b2 was applied). I've also added more tests to test OSSL_STORE directly.

This PR is now out of WIP. Please take a look.

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 18, 2020

Fixup pushed to address some missing no-dh and no-dsa guards.

test/pem_read_depr_test.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member

@levitte levitte commented Nov 19, 2020

I saw a surprising memleak in the last Travis results... have you tried an ASAN build?

(considering the tracebacks I saw, I might have to step in... unless you can quickly see what goes wrong)

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 19, 2020

I saw a surprising memleak in the last Travis results... have you tried an ASAN build?

Leak was in the test. Now fixed.

test/ossl_store_test.c Show resolved Hide resolved
Copy link
Member

@levitte levitte left a comment

If CIs agree...

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 20, 2020

Travis failures are unrelated to this PR.

@levitte
Copy link
Member

@levitte levitte commented Nov 20, 2020

Travis failures are unrelated to this PR.

Confirmed

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Nov 20, 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.

@slontis
Copy link
Contributor

@slontis slontis commented Nov 25, 2020

@mattcaswell are you saying that this PR doesnt fix the ossl store issue? If so is there an issue for this?

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 25, 2020

@mattcaswell are you saying that this PR doesnt fix the ossl store issue? If so is there an issue for this?

I assume you are referring to this comment by me:

My first attempt at adding tests was to create a test that called various PEM_read_bio_* functions to check that we got the expected result back. This effectively does the same thing as the reproducer in #13046. To my surprise I found that the tests I had written were passing in master even without my fix applied!! On further investigation I found that issue in #13046 no longer exists. It was "fixed" by commit 35426b2. However the "fix" only addresses the symptoms by changing PEM_read_bio_DHparams() to not use OSSL_STORE anymore. The bug in OSSL_STORE remains.

What I was trying to say (apparently unsuccessfully) is that there has been a "fix" already applied that meant the reproducer in #13046 no longer works. But that other "fix" did not address the OSSL_STORE bug. This PR does address that bug.

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 25, 2020

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
The OSSL_STORE code was forgetting the datatype that we read from the
PEM header when decoding the DER.

Fixes #13046

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13329)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
Add tests for various deprecated PEM_read_bio_*() functions to ensure
they can still read the various files.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13329)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
There have been instances where OSSL_STORE got confused between DSA and
DH params (e.g. see issue #13046) due the DER encoding of DH and DSA params
looking identical. Therefore we test that we get the types that we expect.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13329)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants