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

Yubico U2F attestation cert with failing test #34

Closed
wants to merge 1 commit into from
Closed

Yubico U2F attestation cert with failing test #34

wants to merge 1 commit into from

Conversation

robn
Copy link

@robn robn commented Jan 15, 2017

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.

@robn
Copy link
Author

robn commented Jan 19, 2017

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):

  268:d=1  hl=2 l=  11 cons: SEQUENCE
  270:d=2  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
  281:d=1  hl=4 l= 257 prim: BIT STRING
      0000 - 03 76 0e 36 95 65 ef 88-e9 04 28 04 d8 8f fa 5f   .v.6.e....(...._
  [...]

webpki is quite deliberately assuming that this won't happen, as evidenced by the name of the webpki::der::bit_string_with_no_unused_bits function.

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.

@briansmith
Copy link
Owner

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 webpki::der::bit_string_with_no_unused_bits function.

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.

@briansmith
Copy link
Owner

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.

@robn
Copy link
Author

robn commented Jan 19, 2017

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.

@a-dma
Copy link

a-dma commented Jan 19, 2017

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
A list of CN/serials is here

Specifically, the certificate used in this PR is
CA:99:31:21:84:6C:46:4D:66:60:96:D3:5F:13:BF:44:C1:B0:5A:F2:05:F9:B4:A1:E0:0C:F6:CC:10:C5:E5:11

The fix is basically to clear the offending byte as you can see here.

@robn
Copy link
Author

robn commented Jan 19, 2017

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 robn closed this Jan 19, 2017
@briansmith
Copy link
Owner

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

@briansmith
Copy link
Owner

Also, thanks @a-dma for the quick response!

@robn
Copy link
Author

robn commented Jan 19, 2017

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.

@briansmith briansmith mentioned this pull request Sep 11, 2017
13 tasks
Pencil-Yao pushed a commit to Pencil-Yao/webpki that referenced this pull request Dec 6, 2023
* Do not fail verification for V3 certs with no extensions.

* Rename error to MalformedExtensions.

---------

Co-authored-by: Jani Monoses <jani.monoses@gmail.com>
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.

None yet

3 participants