tls: Fix OCSP version tag handling#20079
Merged
ggreenway merged 1 commit intoenvoyproxy:mainfrom Feb 28, 2022
Merged
Conversation
PR envoyproxy#12307 added a handrolled OCSP parser, but handled the version field incorrectly. ASN.1 tags are more than just a number. They also have a class (universal, context-specific, application, or private). Class zero is "universal", which is for the built-in types like INTEGER or SEQUENCE. A tag written like [1] is not universal but "context-specific". That means one needs to write CBS_ASN1_CONTEXT_SPECIFIC | 1, not plain 1. Plain 1 is BOOLEAN. ASN.1 TLVs additionally are either constructed or primitive. Explicitly-tagged types are always constructed (they contain other TLVs). The constructed bit is encoded alongside the tag, so CBS/CBB treat it as part of the tag. See https://letsencrypt.org/docs/a-warm-welcome-to-asn1-and-der/ for further background. BoringSSL's CBS/CBB APIs assume familiarity with ASN.1, so reviewers for future ASN.1 PRs in Envoy should read over it. An OCSP response contains the following field: version [0] EXPLICIT Version DEFAULT v1, With Version defined as: Version ::= INTEGER { v1(0) } The outermost tag should be written as CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0. ocsp.cc was using 0. This PR fixes it. (Other tags in ocsp.cc appear to have been correct.) It also fixes the encoding in the test to reflect the EXPLICIT tagging. The test was written as if the version used IMPLICIT tagging. The old code would not have successfully parsed any OCSP response with an explicit version number. Likely the reason this wasn't noticed before is because the field is DEFAULT v1 and v1 is the only defined version. That means any OCSPv1 resolver is not only allowed to omit the version, but *required* to do so under DER. As Envoy only implements v1, it should be rejecting the field, rather than skipping it, but I've left the (originally intended) behavior as-is for now. Finally, this PR fixes some tests in asn1_utility_test.cc to use more plausible test data. The test used tag [UNIVERSAL 0] rather than [0]. ([0] is encoded 0xa0 when constructed.) This PR is necessary to avoid Envoy breaking with future versions of BoringSSL. Tag [UNIVERSAL 0] is particularly fraught because it is reserved for use by the encoding. Notably, it is part of BER indefinite-length EOC markers. Using it as an actual tag has some weird conseequences and future versions of BoringSSL will no longer parse it. Signed-off-by: David Benjamin <davidben@google.com>
Member
|
@ggreenway does this need another review? |
ggreenway
approved these changes
Feb 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message:
PR #12307 added a handrolled OCSP parser, but handled the version field incorrectly.
ASN.1 tags are more than just a number. They also have a class (universal, context-specific, application, or private). Class zero is "universal", which is for the built-in types like INTEGER or SEQUENCE.
A tag written like [1] is not universal but "context-specific". That means one needs to write
CBS_ASN1_CONTEXT_SPECIFIC | 1, not plain 1. Plain 1 is BOOLEAN. ASN.1 TLVs additionally are either constructedor primitive. Explicitly-tagged types are always constructed (they contain other TLVs). The constructed bit is encoded alongside the tag, so CBS/CBB treat it as part of the tag.
See https://letsencrypt.org/docs/a-warm-welcome-to-asn1-and-der/ for further background. BoringSSL's CBS/CBB APIs assume familiarity with ASN.1, so reviewers for future ASN.1 PRs in Envoy should read over it.
An OCSP response contains the following field:
With Version defined as:
The outermost tag should be written as
CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0. ocsp.cc was using0. This PR fixes it. (Other tags in ocsp.cc appear to have been correct.) It also fixes the encoding in the test to reflect the EXPLICIT tagging. The test was written as if the version used IMPLICIT tagging.The old code would not have successfully parsed any OCSP response with an explicit version number. Likely the reason this wasn't noticed before is because the field is DEFAULT v1 and v1 is the only defined version. That means any OCSPv1 resolver is not only allowed to omit the version, but required to do so under DER. As Envoy only implements v1, it should be rejecting the field, rather than skipping it, but I've left the (originally intended) behavior as-is for now.
Finally, this PR fixes some tests in asn1_utility_test.cc to use more plausible test data. The test used tag [UNIVERSAL 0] rather than [0]. ([0] is encoded 0xa0 when constructed.)
This PR is necessary to avoid Envoy breaking with future versions of BoringSSL. Tag [UNIVERSAL 0] is particularly fraught because it is reserved for use by the encoding. Notably, it is part of BER indefinite-length EOC markers. Using it as an actual tag has some weird conseequences and future versions of BoringSSL will no longer parse it.
Signed-off-by: David Benjamin davidben@google.com
Additional Description:
Risk Level: Low
Testing: Existing unit tests
Docs Changes: n/a
Release Notes:
Platform Specific Features: n/a