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

Add entire issuer chain to trusted X509_STORE when stapling OCSP_Response #96792

Merged
merged 7 commits into from
Jan 11, 2024
Merged

Add entire issuer chain to trusted X509_STORE when stapling OCSP_Response #96792

merged 7 commits into from
Jan 11, 2024

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jan 10, 2024 •
edited
Loading

Fixes #96659.

When validating OCSP Staple retrieved in SslStreamCertificateContext, we create an X509_STORE to specify which certificates are to be considered trusted. Until now, we passed in only the immediate issuer of the validated certificate. If this was only an intermediate CA certificate and the OCSP was signed by a delegated signer, the signer verification process would fail because the signer could not be anchored in a trusted Root CA.

Since the CheckOcspGetExpiry function is used by other code paths, I believe the safer fix is to add also the rest of the certificate from the chain built as part of constructing the SslStreamCertificateContext instance, rather than using OCSP_PARTIAL_CHAIN to enable anchoring in trusted intermediate (only) CAs. See also discussion on the original issue.

This was not uncovered by existing tests because the current tests are exercising codepaths which call CheckOcspGetExpiry from different callstack (see #96659 (comment)). For now I tested the code via the repro code from the issue mentioned above. Automated tests are delegated to #96791.

@ghost
Copy link

ghost commented Jan 10, 2024

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #96659.

When validating OCSP Staple retrieved in SslStreamCertificateContext, we create an X509_STORE to specify which certificates are to be considered trusted. Until now, we passed in only the immediate issuer of the validated certificate. If this was only an intermediate CA certificate and the OCSP was signed by a delegated signer, the signer verification process would fail because the signer could not be anchored in a trusted Root CA.

Since the CheckOcspGetExpiry function is used by other code paths, I believe the safer fix is to add also the rest of the certificate from the chain built as part of constructing the SslStreamCertificateContext instance, rather than using OCSP_PARTIAL_CHAIN to enable anchoring in trusted intermediate (only) CAs. See also discussion on the original issue.

This was not uncovered by existing tests because the current tests are calling CheckOcspGetExpiry via different code path. For now I tested the code via the repro code from the issue mentioned above. Automated tests are delegated to #96791.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Security
Milestone: -

@ghost ghost assigned rzikm Jan 10, 2024
@rzikm @bartonjs
Update src/libraries/System.Net.Security/src/System/Net/Security/SslS…
c300fc3 
…treamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@bartonjs bartonjs changed the title Add entire issuer chain to trusted X509_STORE when validating OCSP_Response Add entire issuer chain to trusted X509_STORE when stapling OCSP_Response Jan 10, 2024
wfurt
wfurt approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@rzikm
Copy link
Member Author

rzikm commented Jan 10, 2024

looks like we actually always need to always keep the root from AddRootCertificate🤦

@bartonjs
Copy link
Member

FWIW, you should be able to test this locally by just adding PkiOptions.IssuerAuthorityHasDesignatedOcspResponder in src\libraries\System.Net.Security\tests\FunctionalTests\CertificateValidationRemoteServer.cs line 199.

Longer term that probably needs to come from a parameter in that helper routine so that there are tests both with and without it; but that's the fastest "how do I test this?" that I can come up with.

@rzikm
Copy link
Member Author

rzikm commented Jan 10, 2024

I have extracted the CertificateAuthority and RevocationResponder to a console app, so I am now running against that and was wondering why it was not working anymore. A fix is incoming.

@rzikm
Copy link
Member Author

rzikm commented Jan 10, 2024 •
edited
Loading

I tested latest version with openssl s_client .... -status and

  • I am getting a delegated-signed response
  • the response is updated if <5min to expiration.

PTAL to confirm there is nothing terribly wrong in the last commit.

@rzikm rzikm requested a review from bartonjs January 10, 2024 21:39
@rzikm
Copy link
Member Author

rzikm commented Jan 10, 2024

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jan 11, 2024
Closed
@karelz karelz added this to the 9.0.0 milestone Jan 11, 2024
@build-analysis build-analysis bot mentioned this pull request Jan 11, 2024
Open
@rzikm
Copy link
Member Author

rzikm commented Jan 11, 2024

No related failures in the outerloop run.

@rzikm rzikm merged commit a6bf4e4 into dotnet:main Jan 11, 2024
125 checks passed
rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Jan 11, 2024
@rzikm @bartonjs
Add entire issuer chain to trusted X509_STORE when stapling OCSP_Resp…
df4171f 
…onse (dotnet#96792)

* Add entire issuer chain to trusted X509_STORE when validating OCSP_Response

* Code review feedback

* More code review feedback

* Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Fix compilation

* Always include root certificate

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
This was referenced Jan 11, 2024
Merged
Merged
wfurt pushed a commit that referenced this pull request Jan 16, 2024
@rzikm @bartonjs
[release/8.0] Fix server-side OCSP stapling on Linux (#96838)
85c2772 
* Add entire issuer chain to trusted X509_STORE when stapling OCSP_Response (#96792)

* Add entire issuer chain to trusted X509_STORE when validating OCSP_Response

* Code review feedback

* More code review feedback

* Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Fix compilation

* Always include root certificate

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Recover from failed OCSP download. (#96448)

* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry

* Don't shorten OCSP expriation on failed server OCSP fetch (#96972)

* Don't shorten OCSP expriation on failed server OCSP fetch

* Code review feedback

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@rzikm @bartonjs @tmds
Add entire issuer chain to trusted X509_STORE when stapling OCSP_Resp…
2a93b35 
…onse (dotnet#96792)

* Add entire issuer chain to trusted X509_STORE when validating OCSP_Response

* Code review feedback

* More code review feedback

* Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Fix compilation

* Always include root certificate

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2024
@bartonjs bartonjs added cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. tracking This issue is tracking the completion of other related issues. and removed cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. labels Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vcsjones vcsjones vcsjones left review comments

@wfurt wfurt wfurt approved these changes

@bartonjs bartonjs bartonjs approved these changes

Assignees

@rzikm rzikm

Labels
area-System.Net.Security cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. tracking This issue is tracking the completion of other related issues.
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

Unable to verify signer of OCSP Response on Linux
5 participants
@rzikm @bartonjs @vcsjones @wfurt @karelz