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

Fix name constraints check #226

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

Conversation

bnoordhuis
Copy link

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.

@bnoordhuis
Copy link
Author

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?

@bnoordhuis
Copy link
Author

\x82\x11web-platform.test looks 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.

@lucacasonato
Copy link

Possibly related to #20

There were two bugs. webpki didn't:

1. read the X.509 Name Constraints field in its entirety, nor

2. check the certificate subject against the constraints correctly

(1) is a simple fix, (2) requires reading the Common Name from the
certificate.

Closes briansmith#20, briansmith#134.
@bnoordhuis bnoordhuis changed the title [WIP] add failing test Fix name constraints check May 1, 2021
@bnoordhuis
Copy link
Author

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?

@est31
Copy link

est31 commented May 1, 2021

@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 👍 from me.

@bnoordhuis
Copy link
Author

@briansmith You mentioned on May 2 in briansmith/ring#1265 that you had a fix in the works. Has that landed yet? (And if so, where? Here or there?)

@kaizensparc
Copy link

Hello,
Do we have any news about this?

@jimisaacs
Copy link

+1 researching issues I'm facing with corporate certs lead me here. Any news?

@djc
Copy link

djc commented Sep 20, 2022

@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.

@bnoordhuis
Copy link
Author

@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.

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.

6 participants