Fix `ossl_x509v3_cache_extensions()`: `EXFLAG_NO_FINGERPRINT` is not an error by DDvO · Pull Request #16043 · openssl/openssl

DDvO · 2021-07-11T17:02:49Z

Upon closer inspection, it turns out that the root cause of the hickup discussed for #16036 is in ossl_x509v3_cache_extensions():
the case EXFLAG_NO_FINGERPRINT simply should not be regarded as an error.

This PR fixes the problem and on this occasion cleans up the error handling of that function.
The fix allows reverting the recent workaround in commit 6bfd3e5 on cmp_ctx_test.c regarding X509_new().

tests are added or updated

levitte · 2021-07-12T13:55:11Z

Have you looked in history why this was made an error?

What happens with OSSL_CMP_CTX_set1_srvCert()? It shouldn't accept an invalid certificate, should it?

levitte · 2021-07-12T14:47:52Z

There's been a few changes over time... at some point, a certificate that was impossible to make a fingerprint from was regarded as invalid. Why should that no longer be the case? I find it deeply troublesome that, by being sloppy with this, functions like OSSL_CMP_CTX_set1_srvCert() will potentially accept invalid certificates without even noticing!

DDvO · 2021-07-12T16:14:19Z

Have you looked in history why this was made an error?

I had introduced this myself in commit 1e41dad
and meanwhile I see it was wrong.

What happens with OSSL_CMP_CTX_set1_srvCert()? It shouldn't accept an invalid certificate, should it?

Actually the term 'invalid certificate' is very vague, and looks like I should have avoided that term in the implementation of OSSL_CMP_CTX_set1_srvCert() and friends.

The function X509v3_cache_extensions() has grown wild over the years, and at least its name should be changed because it constantly causes confusion. It does not do full validity check on the given cert, but just caches various stuff including a cert hash/fingerprint, and doing so needs to perform some very basic well-formedness checks.
The presence and correctness of the signature is a much more high-level topic not to be handled at this point.

I find it deeply troublesome that, by being sloppy with this, functions like OSSL_CMP_CTX_set1_srvCert() will potentially accept invalid certificates without even noticing!

No need to worry here - my definition

#define X509_invalid(cert) (!ossl_x509v3_cache_extensions(cert))

uses a rather strong name, but note that 'invalid' just covers very basic well-formedness of the cert structure.
What is more to the point is the comment I gave in the code soon thereafter:

    /* prevent misleading error later on malformed cert or provider issue */ \

So the function ossl_x509v3_cache_extensions() is called just for improving diagnostics.
Even if this was omitted, no insecurity is introduced because the cert is anyway verified at a later stage where needed.

mattcaswell · 2021-07-13T09:04:28Z

OTC: This change is not for 3.0. It can be considered post 3.0.

openssl-machine · 2022-03-15T00:07:18Z

This PR is in a state where it requires action by @openssl/otc but the last update was 244 days ago

openssl-machine · 2022-04-15T00:07:21Z

This PR is in a state where it requires action by @openssl/otc but the last update was 275 days ago

openssl-machine · 2022-05-16T00:07:18Z

This PR is in a state where it requires action by @openssl/otc but the last update was 306 days ago

openssl-machine · 2022-06-16T00:07:52Z

This PR is in a state where it requires action by @openssl/otc but the last update was 337 days ago

openssl-machine · 2022-07-17T00:07:49Z

This PR is in a state where it requires action by @openssl/otc but the last update was 368 days ago

t8m

OK for master if CI passes.

DDvO · 2022-07-18T11:12:18Z

Thanks @t8m for resuming the handling of this rather old PR - I was about to ask for that, but then saw your approval :)
CI did pass meanwhile.

So now asking Committers for the 2nd review.

…be an error This allows reverting the recent workaround on cmp_ctx_test regarding X509_new()

DDvO · 2022-08-15T16:42:20Z

Rebased to fix merge conflict.
Asking again @openssl/committers for 2nd review.

crypto/x509/v3_purp.c

…ld not be an error

beldmit

LGTM if tests passed

beldmit · 2022-08-16T10:55:00Z

@t8m could you please reconfirm?

openssl-machine · 2022-08-17T14:00:26Z

This pull request is ready to merge

hlandau · 2022-08-17T15:55:45Z

Is this for master or master and 3.0?

t8m · 2022-08-17T16:45:05Z

Master only as this kind of refactoring is IMO not a bug fix.

DDvO · 2022-08-18T07:30:35Z

Merged - thanks @levitte, @t8m and @beldmit

…be an error This allows reverting the recent workaround on cmp_ctx_test regarding X509_new() Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #16043)

DDvO · 2022-08-18T07:36:53Z

Master only as this kind of refactoring is IMO not a bug fix.

Well, to me, removing a wrongly thrown error is a bug fix, though a minor one.
Do you see a problem backporting this?

t8m · 2022-08-18T08:29:15Z

I wouldn't recommend backporting this. It is really border-line on whether this is a bug fix or not.

DDvO · 2022-08-18T08:32:40Z

All right, so closing.

…be an error This allows reverting the recent workaround on cmp_ctx_test regarding X509_new() Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from openssl#16043)

…NT should not be an error From OpenSSL commit 2c05607cd91fc5aab6d61f0324104d63a091d705 openssl/openssl#16043 This allows reverting the recent workaround on cmp_ctx_test regarding X509_new()

DDvO added approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Jul 11, 2021

DDvO mentioned this pull request Jul 12, 2021

ASN.1: Refuse to encode to DER if non-optional items are missing [3.0] #16036

Closed

DDvO requested a review from t8m July 12, 2021 16:23

t8m added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Jul 13, 2021

t8m added this to the 3.0.0 milestone Jul 13, 2021

mattcaswell modified the milestones: 3.0.0, Post 3.0.0 Jul 13, 2021

mattcaswell removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Jul 13, 2021

t8m added the triaged: OTC evaluated label Jul 13, 2021

t-j-h mentioned this pull request Jul 13, 2021

Document behaviour change for various X509 functions #16065

Closed

t8m closed this Jul 18, 2022

t8m reopened this Jul 18, 2022

t8m added approval: review pending This pull request needs review by a committer triaged: refactor The issue/pr requests/implements refactoring and removed triaged: bug The issue/pr is/fixes a bug labels Jul 18, 2022

t8m approved these changes Jul 18, 2022

View reviewed changes

t8m removed the approval: otc review pending label Jul 18, 2022

DDvO requested review from paulidale and removed request for paulidale July 19, 2022 07:22

DDvO requested a review from paulidale July 19, 2022 12:19

DDvO force-pushed the fix_ossl_x509v3_cache_extensions-NO_FINGERPRINT branch from fb8a0dc to d2d6cf8 Compare August 15, 2022 16:08

Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT should not …

3b7d1ff

…be an error This allows reverting the recent workaround on cmp_ctx_test regarding X509_new()

DDvO force-pushed the fix_ossl_x509v3_cache_extensions-NO_FINGERPRINT branch from d2d6cf8 to 3b7d1ff Compare August 15, 2022 16:16

beldmit reviewed Aug 15, 2022

View reviewed changes

crypto/x509/v3_purp.c Show resolved Hide resolved

crypto/x509/v3_purp.c Show resolved Hide resolved

fixup! Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT shou…

bb9ead5

…ld not be an error

beldmit approved these changes Aug 16, 2022

View reviewed changes

t8m approved these changes Aug 16, 2022

View reviewed changes

t8m 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 Aug 16, 2022

openssl-machine 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 17, 2022

t8m added the branch: master Applies to master branch label Aug 17, 2022

DDvO closed this Aug 18, 2022

Uh oh!

Conversation

DDvO commented Jul 11, 2021

Uh oh!

levitte commented Jul 12, 2021

Uh oh!

levitte commented Jul 12, 2021

Uh oh!

DDvO commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Jul 13, 2021

Uh oh!

openssl-machine commented Mar 15, 2022

Uh oh!

openssl-machine commented Apr 15, 2022

Uh oh!

openssl-machine commented May 16, 2022

Uh oh!

openssl-machine commented Jun 16, 2022

Uh oh!

openssl-machine commented Jul 17, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Jul 18, 2022

Uh oh!

DDvO commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beldmit left a comment

Choose a reason for hiding this comment

Uh oh!

beldmit commented Aug 16, 2022

Uh oh!

openssl-machine commented Aug 17, 2022

Uh oh!

hlandau commented Aug 17, 2022

Uh oh!

t8m commented Aug 17, 2022

Uh oh!

DDvO commented Aug 18, 2022

Uh oh!

DDvO commented Aug 18, 2022

Uh oh!

t8m commented Aug 18, 2022

Uh oh!

DDvO commented Aug 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DDvO commented Jul 12, 2021 •

edited

Loading

DDvO commented Aug 15, 2022 •

edited

Loading