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.

Sign up for GitHub

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

Jump to bottom

Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT is not an error #16043

Closed
wants to merge 2 commits into from
Closed

Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT is not an error #16043

wants to merge 2 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Jul 11, 2021

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

@DDvO DDvO added approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Jul 11, 2021
@DDvO DDvO mentioned this pull request Jul 12, 2021
Closed
@levitte
Copy link
Member

levitte commented Jul 12, 2021

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
Copy link
Member

levitte commented Jul 12, 2021

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
Copy link
Contributor Author

DDvO commented Jul 12, 2021 •
edited

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.

@DDvO DDvO requested a review from t8m July 12, 2021 16:23
@t8m t8m added the hold: need otc decision The OTC needs to make a decision label Jul 13, 2021
@t8m t8m added this to the 3.0.0 milestone Jul 13, 2021
@mattcaswell
Copy link
Member

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

@mattcaswell mattcaswell modified the milestones: 3.0.0, Post 3.0.0 Jul 13, 2021
@mattcaswell mattcaswell removed the hold: need otc decision The OTC needs to make a decision label Jul 13, 2021
@t8m t8m added the triaged: OTC evaluated This issue/pr was triaged by OTC label Jul 13, 2021
@t-j-h t-j-h mentioned this pull request Jul 13, 2021
Closed
@openssl-machine
Copy link
Collaborator

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

@openssl-machine
Copy link
Collaborator

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

@openssl-machine
Copy link
Collaborator

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

@openssl-machine
Copy link
Collaborator

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

@openssl-machine
Copy link
Collaborator

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

@t8m t8m closed this Jul 18, 2022
@t8m t8m reopened this Jul 18, 2022
@t8m 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
t8m approved these changes Jul 18, 2022
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.

OK for master if CI passes.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jul 18, 2022
@DDvO
Copy link
Contributor Author

DDvO commented Jul 18, 2022

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.

@DDvO DDvO requested review from paulidale and removed request for paulidale July 19, 2022 07:22
@DDvO DDvO requested a review from paulidale July 19, 2022 12:19
@DDvO DDvO force-pushed the fix_ossl_x509v3_cache_extensions-NO_FINGERPRINT branch from fb8a0dc to d2d6cf8 Compare August 15, 2022 16:08
@DDvO
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 DDvO force-pushed the fix_ossl_x509v3_cache_extensions-NO_FINGERPRINT branch from d2d6cf8 to 3b7d1ff Compare August 15, 2022 16:16
@DDvO
Copy link
Contributor Author

DDvO commented Aug 15, 2022 •
edited

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

beldmit
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
beldmit
beldmit approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM if tests passed

@beldmit
Copy link
Member

beldmit commented Aug 16, 2022

@t8m could you please reconfirm?

@t8m 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 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
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Aug 17, 2022

Is this for master or master and 3.0?

@t8m t8m added the branch: master Merge to master branch label Aug 17, 2022
@t8m
Copy link
Member

t8m commented Aug 17, 2022

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

@DDvO
Copy link
Contributor Author

DDvO commented Aug 18, 2022

Merged - thanks @levitte, @t8m and @beldmit

openssl-machine pushed a commit that referenced this pull request Aug 18, 2022
@DDvO
Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT should not …
2c05607 
…be an error

This allows reverting the recent workaround on cmp_ctx_test regarding X509_new()

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #16043)
@DDvO
Copy link
Contributor Author

DDvO commented Aug 18, 2022

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
Copy link
Member

t8m commented Aug 18, 2022

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

@DDvO
Copy link
Contributor Author

DDvO commented Aug 18, 2022

All right, so closing.

@DDvO DDvO closed this Aug 18, 2022
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT should not …
ebcf1aa 
…be an error

This allows reverting the recent workaround on cmp_ctx_test regarding X509_new()

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#16043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit approved these changes

@paulidale paulidale Awaiting requested review from paulidale

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: OTC evaluated This issue/pr was triaged by OTC triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Milestone
Post 3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@DDvO @levitte @mattcaswell @openssl-machine @beldmit @hlandau @t8m