-
Notifications
You must be signed in to change notification settings - Fork 166
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
Yubico U2F attestation cert with failing test #34
Conversation
I believe I understand the problem, though I have not yet figured out how to fix it. The signature in the certificate is an ASN.1 BIT STRING, with a non-zero number of unused bits (the first byte):
webpki is quite deliberately assuming that this won't happen, as evidenced by the name of the I am still working through the best "fix" for this. So far I have it handling the non-zero count, but I need to read through more of the code because I'm not sure how deep the assumption that all signatures will be composed of complete bytes actually goes. |
https://tools.ietf.org/html/rfc3447#section-8.2.1: "Output: an octet string of length k, where k is the length in octets of the RSA modulus n." That is, it is impossible to have a valid DER-encoded RSA PKCS#1 signature that has unused bits. I will ask somebody at Yubico about this. |
It looks like Yubico is pretty responsive to feedback about these things. Apparently they have a forum where such issues can be reported: https://forum.yubico.com/viewtopic.php?f=33&t=1650. I recommend reporting the issue on their forum and linking to the post from this PR. |
Posted to Yubico forum: https://forum.yubico.com/viewtopic.php?f=33&t=2526. I will report back if anything useful comes from it. Thanks for the advice @briansmith. |
Hi, so there is a set of six certificates that are basically broken. Older version of OpenSSL used to set that byte rather than clear it. A list of SHA256 fingerprints for these certs can be found here Specifically, the certificate used in this PR is The fix is basically to clear the offending byte as you can see here. |
Wow, that's amazing. Good waste of my time this week then! There's obviously nothing here for webpki then, so am closing this issue. Thanks for the pointers @a-dma, greatly appreciated! |
@robn I recommend, if possible, that you get new devices that are guaranteed to have well-formed certificates. Failing that, I recommend to do the preprocessing that Yubico's own code is doing before calling into webpki. If you'd like, please consider manually fixing one of these certificates and updating the PR with the "fixed" version. This way, we can see what works and doesn't work with webpki and these certificates after they've been preprocessed and/or on fixed devices. If/when we get them working in webpki, this would become a good regression test to minimize the chances that webpki accidentally breaks compatibility with these non-TLS-server certificates. And/or, this may identify more work that is needed for webpki to support them. |
Also, thanks @a-dma for the quick response! |
I've already confirmed both a Yubikey 4 and a Plug-Up have valid certificates. I'm intending to add the same workarounds to u2f-rs and Authen::U2F (my Perl U2F server lib), once I've discovered why it doesn't break. I could submit a pull request right now with a valid and invalid cert and basic parsing tests for both. I'm inclined to wait for the moment though, just in case any changes are required to webpki to deal with other differences these certs may have. |
* Do not fail verification for V3 certs with no extensions. * Rename error to MalformedExtensions. --------- Co-authored-by: Jani Monoses <[email protected]>
Not for merge.
This adds a failing test for parsing an attestation certificate from a Yubico U2F device. I have not yet learned enough about webpki/ring internals to figure out where the problem is. I will keep working on it, but would appreciate assistance if anyone can help.