-
Notifications
You must be signed in to change notification settings - Fork 18k
crypto/x509: reject BMPStrings that use invalid characters #71862
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
Comments
@rolandshoemaker There is also T61String (aka TeletexString), which (i think) no one knows what it really is. See mine CL 487755 which didn't get much attention. |
Oh thanks for the pointer, I completely missed that CL. Will take a closer look. |
Change https://go.dev/cl/651275 mentions this issue: |
@rolandshoemaker would this change "fix" an issue where there is "trailing data" in some certificates? I am reading a certificate from a TPM chip and when I parse it with ParseCertificate (just to double-check that it is indeed a valid x509 certificate), it fails because of this particular "trailing data" error. When I look at the certificate using a hex editor, I see a lot of "extra" FF characters like this: When I run If it is something else, just let me know. |
@DwayneBradley-eaton is appears unrelated. We reject trailing data in when parsing certificates, as that is a violation of the DER encoding rules. When retrieving you may be getting extra padding at the end of your TPM object or something. |
@rolandshoemaker Do you have a link to the DER encoding rules? I have never seen this before when reading certs from a TPM. It is only happening on this particular TPM chip vendor I am currently interacting with. And it is not JUST the |
@DwayneBradley-eaton look at https://letsencrypt.org/pl/docs/a-warm-welcome-to-asn1-and-der/ (it does not seem to load currently, but try with a wayback). Also you might check the x/crypto/cryptobyte package and look at x509/parser.go for reference. |
I think it's the (Let's Encrypt just redid their website, so I think there might be some small goblins to chase out) Edit: and I think the canonical reference for DER is ITU X.690: https://www.itu.int/rec/T-REC-X.690/en |
The BMPString type is a UCS-2 encoded string. UCS-2 is a defunct uint16 code point based string encoding that was subsumed into UTF-16. There were a large number of unused code points in UCS-2 that became surrogate points in UTF-16 (characters which used multiple code points).
In our BMPString "parser", we just decode the string as UTF-16, which is mostly okay, except it accepts surrogate characters, which should be rejected. This can lead to confusing behavior where we parse an invalid BMPString when we should reject it.
BMPString is basically unused in the webpki (it's disallowed by the CABF BRs), and 5280 only allows it for backwards compatibility with old DNs. We should just reject invalid BMPStrings. We could also consider removing BMPString support entirely.
Thanks to Jinfeng Guo for reporting this issue.
The text was updated successfully, but these errors were encountered: