webpki
webpki copied to clipboard
briansmith
Fix name constraints check
The test validates the end entity certificate against the self-signed CA from the Web Platform Tests (WPT) conformance suite.
The certificates are somewhat unusual in that they have really long "Name Constraints" and "Subject Alternative Name" fields but other libraries parse them just fine.
webpki doesn't seem able to handle the "Name Constraints" field in the
CA certificate. The der::nested() call in `parse_subtrees() in
src/name/verify.rs expects its closure to consume all input but it
consumes only the first item on the list.
I wanted to fix the issue but I wasn't sure what the best way forward was and figured putting up this PR would be a good conversation starter.
Looking at the first bytes of the Name Constraints field in ca.der:
\x30\x82\x1f\x8e\xa0\x82\x1f\x8a\x30\x13\x82\x11web-platform.test
\x30\x82\x1f\x8e is a sequence (0x30) of length (0x82) 0x1f8e == 8078 octets. Looks okay.
\xa0\x82\x1f\x8a is a ContextSpecificConstructed0 of length 0x1f8a == 8074 octets. Okay.
\x30\x13 is a sequence of 19 bytes. Okay.
\x82\x11web-platform.test looks dubious. The 0x82 says it's a 16 bits length prefix but it's followed by a single byte 0x11 or 17, which is indeed the length of "web-platform.test".
I'm confused why openssl would a) generate such a certificate, and b) parse it okay.
@briansmith Can you tell me if I'm on the right track here?
\x82\x11web-platform.testlooks dubious.
I was mistaken. https://tools.ietf.org/html/rfc5280#section-4.2.1.10 says it's a GeneralName so this is a context-specific (0x80) dnsName (0x02) containing an IA5String. The name constraint is well-formed. It does seem like the bug is in webpki.
Figured it out, updated the pull request with a fix.
@briansmith Please review, thanks.
@est31 Can you confirm that this fixes your issue as well?
@bnoordhuis it seems to fix #135, but it still gives me an UnknownIssuer error when I try it on my rcgen test.
Even if I don't pass the empty DirectoryName and only pass "dev" it still gives an error. Even if I give the "crabs.dev" error, it still errors. It works well for openssl. Would have to debug webpki again, but I feel that your PR improves the situation so :+1: from me.
@briansmith You mentioned on May 2 in https://github.com/briansmith/ring/pull/1265 that you had a fix in the works. Has that landed yet? (And if so, where? Here or there?)
+1 researching issues I'm facing with corporate certs lead me here. Any news?
@bnoordhuis there is now a fork in the rustls org. Would you be interested in resubmitting your changes there so we can review it? We will consider publishing the next version of rustls with a dependency on our forked rustls-webpki.
@djc I can do that but some changes to ring are also needed (briansmith/ring#1265) and it looks like you didn't fork that?
I suppose I could open-code it but that's kind of horrible.