Fix some decoder issues including a regression #21603

mattcaswell · 2023-07-31T12:10:17Z

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case.

#Fixes #21588

While fixing the above I noticed some other odd behaviour with the decoders and fixed that too.

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed.

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders.

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed.

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes openssl#21588

We must not ask for a password when attempting to read parameters.

t-j-h · 2023-07-31T13:10:22Z

Okay for master of course. This does seem enough like a bug fix to be considered for backfit if @t8m thinks it should go into 3.1 and 3.0.

t8m · 2023-07-31T20:10:31Z

Yes, imo this is bug fix and should go to all 3.x branches.

test/pemtest.c

slontis · 2023-08-01T03:28:43Z

crypto/encode_decode/decoder_lib.c

            ok = (rv > 0);
-            if (ok)
+            if (ok) {
+                data->flag_construct_called = 1;


If this is correct behavior then it may be worth changing the name of the 'flag_construct_called', since technically beforehand it is doing what was described i.e. the constructor was called..

I have no objection to @mattcaswell renaming it - I don't particularly find the current names all that enlightening.
My approval stands with or without the renaming being suggested by @slontis

That seems to be more "refactor" than bug fix which I'd rather not do in this PR since it is aimed at backport.

I am actually not sure if this change hasn't some other unwanted consequences. Apparently the test suite passes but there might be a performance impact or some corner cases (which we do not test) might be broken.

I doubt it will cause much problem. There are very few places where this flag is ever read. It's read in OSSL_DECODER_from_bio to determine whether to add an error to the stack or not (which is actually the reason for this fix, because it was omitting adding an error in cases where it should). It's also read later on in the same function to determine whether a following decoder successfully called construct (and therefore we should stop looking).

OK, I am fine with merging this now. But perhaps @levitte could look at the consequences when he returns from his vacation.

mattcaswell · 2023-08-01T10:34:04Z

Trivial fixup pushed (whitespace only), addressing @slontis's comment. I assume the approvals still stand.

openssl-machine · 2023-08-01T14:00:20Z

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.

t8m · 2023-08-01T18:06:53Z

crypto/encode_decode/decoder_lib.c

            ok = (rv > 0);
-            if (ok)
+            if (ok) {
+                data->flag_construct_called = 1;


OK, I am fine with merging this now. But perhaps @levitte could look at the consequences when he returns from his vacation.

t8m · 2023-08-01T18:10:38Z

Merged to master, 3.1 and 3.0 branches. Thank you.

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603)

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603)

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes #21588 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603)

We must not ask for a password when attempting to read parameters. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603)

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 6207f2b)

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 564e5b7)

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes #21588 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 0d0791e)

We must not ask for a password when attempting to read parameters. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit df3d609)

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 6207f2b)

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 564e5b7)

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes #21588 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit 0d0791e)

We must not ask for a password when attempting to read parameters. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21603) (cherry picked from commit df3d609)

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603)

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603)

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes openssl#21588 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603)

We must not ask for a password when attempting to read parameters. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603)

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603) (cherry picked from commit 6207f2b)

We're always supposed to add the fallback "unsupported" error if we don't have anything better. However in some cases this wasn't happening because we were incorrectly setting "flag_construct_called" - even though the construct function had failed. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603) (cherry picked from commit 564e5b7)

The PEM_read_bio_Parameters[_ex] function does not have the capability of specifying a password callback. We should not use the fallback password callback in this case because it will attempt to send a prompt for the password which might not be the correct thing to do. We should just not use a password in that case. Fixes openssl#21588 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603) (cherry picked from commit 0d0791e)

We must not ask for a password when attempting to read parameters. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21603) (cherry picked from commit df3d609)

mattcaswell added 4 commits July 31, 2023 12:28

Don't add the msblob/pvk decoders if they're not suitable

7dae97f

msblob only decodes public/private keys (not just params). pvk only decodes private keys. If the requested selection doesn't intersect with the above then don't consider those decoders.

Add a test for PEM_read_bio_Parameters()

ae35202

We must not ask for a password when attempting to read parameters.

mattcaswell mentioned this pull request Jul 31, 2023

PEM_read_bio_Parameters(BIO *bp, EVP_PKEY **x) : something has changed since v3.0 ! #21588

Closed

t8m added the triaged: bug The issue/pr is/fixes a bug label Jul 31, 2023

t8m approved these changes Jul 31, 2023

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jul 31, 2023

t-j-h approved these changes Jul 31, 2023

View reviewed changes

t-j-h 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 Jul 31, 2023

t8m added the tests: present The PR has suitable tests present label Jul 31, 2023

slontis reviewed Aug 1, 2023

View reviewed changes

test/pemtest.c Outdated Show resolved Hide resolved

slontis reviewed Aug 1, 2023

View reviewed changes

fixup! Add a test for PEM_read_bio_Parameters()

f15a687

t8m approved these changes Aug 1, 2023

View reviewed changes

t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 1, 2023

t8m closed this Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some decoder issues including a regression #21603

Fix some decoder issues including a regression #21603

mattcaswell commented Jul 31, 2023

t-j-h commented Jul 31, 2023

t8m commented Jul 31, 2023

slontis Aug 1, 2023

t-j-h Aug 1, 2023

mattcaswell Aug 1, 2023

t8m Aug 1, 2023

mattcaswell Aug 1, 2023

t8m Aug 1, 2023

mattcaswell commented Aug 1, 2023

openssl-machine commented Aug 1, 2023

t8m Aug 1, 2023

t8m commented Aug 1, 2023

Fix some decoder issues including a regression #21603

Fix some decoder issues including a regression #21603

Conversation

mattcaswell commented Jul 31, 2023

t-j-h commented Jul 31, 2023

t8m commented Jul 31, 2023

slontis Aug 1, 2023

Choose a reason for hiding this comment

t-j-h Aug 1, 2023

Choose a reason for hiding this comment

mattcaswell Aug 1, 2023

Choose a reason for hiding this comment

t8m Aug 1, 2023

Choose a reason for hiding this comment

mattcaswell Aug 1, 2023

Choose a reason for hiding this comment

t8m Aug 1, 2023

Choose a reason for hiding this comment

mattcaswell commented Aug 1, 2023

openssl-machine commented Aug 1, 2023

t8m Aug 1, 2023

Choose a reason for hiding this comment

t8m commented Aug 1, 2023