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

ocsp: better validate OCSP response's certificates #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cipherboy
Copy link

We make three changes here:

  1. Allow iterating over all given certificates to find the one that signed this OCSP response, as RFC 6960 does not guarantee an order and some CAs send multiple certificates, and
  2. Allow the passed issuer to match the certificate that directly signed this response, and
  3. Lastly, we document the unsafe behavior of calling these functions with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf cert as an additional cert in the response field (without using a delegated OCSP certificate), Go would err with a bad signature, as it expected the intermediate CA to have signed the wire copy (even though it was the exact same certificate).

Also includes a code comment around the "bad signature on embedded certificate" error message, indicating that this isn't strictly the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Fixes golang/go#59641

@gopherbot
Copy link
Contributor

This PR (HEAD: 4da111c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/485055 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy (even though
it was the exact same certificate).

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Fixes golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy force-pushed the better-ocsp-issuer-validation-semantics branch from 4da111c to 7ee4c84 Compare April 17, 2023 13:57
@gopherbot
Copy link
Contributor

Message from Alex Scheel:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/485055.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 7ee4c84) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/485055 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Alex Scheel:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/485055.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/crypto/ocsp: ParseResponse makes incorrect choices about response and issuer verification
2 participants