va: Validate CAA parameter tags per RFC 8659 - #8977
Open
hablutzel1 wants to merge 3 commits into
Open
Conversation
RFC 8659 Section 4.2 defines a parameter tag as
"tag = (ALPHA / DIGIT) *( *("-") (ALPHA / DIGIT))", which permits
hyphens between alphanumerics. parseCAARecord rejected any tag
containing a hyphen (its character check excluded 0x2D), so a
well-formed issue/issuewild record carrying a hyphenated parameter was
treated as malformed and skipped. If that was the only relevant
record, issuance was then denied even for an otherwise authorized
request.
https://www.rfc-editor.org/rfc/rfc8659#section-4.2
An empty parameter tag is not valid, since RFC 8659 (and even RFC 6844) both require a tag to have at least one character. The tag regexp rejects it. https://www.rfc-editor.org/rfc/rfc8659#section-4.2
Validate the CAA parameter value with a regexp instead of a hand-rolled loop over runes, mirroring the parameter tag check. The behavior is unchanged: the value must be RFC 8659 Section 4.2 "value = *(%x21-3A / %x3C-7E)". This removes the last rune loop in parseCAARecord, so the staticcheck exclusions it needed apparently can go: the (dead already) inline "//lint:ignore S1029,SA6003" and the repo-wide S1029 and SA6003 entries in .golangci.yml too. https://www.rfc-editor.org/rfc/rfc8659#section-4.2
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
1 participant
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.
parseCAARecord's character check rejects hyphens in a parameter tag, which matches the narrower RFC 6844 grammar rather than the RFC 8659 one it should follow.From https://www.rfc-editor.org/rfc/rfc6844#section-5.2:
From https://www.rfc-editor.org/rfc/rfc8659#section-4.2:
The character check excludes 0x2D, so an extra hyphenated tag makes the whole record fail to parse and the caller skips it. If that was the record authorizing the CA, a request the CAA records actually authorize is denied. This simply can block a Subscriber from getting a certificate.
The same check is also too lenient in the other direction: it accepts an empty tag, because the character loop runs zero times over an empty string. An empty tag is an RFC violation, since RFC 8659
tagrequires at least one character.This last one is especially problematic and could have produced misissuances (didn't test myself to avoid the hassle of a Bugzilla Bug), because the TBRs require RFC 8659 adherence and RFC 8659 Section 4.2 says:
An empty issuer-domain-name authorizes no issuer, so whether a value is judged to match the grammar decides whether issuance is allowed.
Validating the tag with a regexp for the RFC 8659 grammar corrects both of the previous issues: interior hyphens are accepted, and leading hyphens, trailing hyphens, and empty tags are rejected.
For consistency the parameter value is validated with a regexp too. That part is separable and can be left out.
On performance, a regexp is slower than the hand-rolled loops, but I think that is not significant here, since CAA checking is dominated by the DNS lookups. For the tag, a loop matching the RFC 8659 grammar is also not very concise.
Finally, as mentioned earlier in #8975 (comment), it might be a good idea to implement this parsing logic in a single place and share it with the Persistent DCV method.