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

Systemic presence of extra byte for registration with eddsa #160

Closed
Gashmob opened this issue Jul 10, 2023 · 8 comments · Fixed by #167
Closed

Systemic presence of extra byte for registration with eddsa #160

Gashmob opened this issue Jul 10, 2023 · 8 comments · Fixed by #167

Comments

@Gashmob
Copy link

Gashmob commented Jul 10, 2023

In goal to register a passkey on a website, I use a php library (https://github.com/web-auth/webauthn-framework). When I try to register a new credential with eddsa as 'prefered' algorithm, the load of passkey's response failed for presence of extra bytes in authentication data .If I comment the check then all is good.

I've tried to decompose the attestation object by hand and find that:

00000000: 10100011 01100011 01100110 01101101 01110100 01100100 01101110 01101111  .cfmtdno
          map    3 txt    3        f        m        t txt    4        n        o

00000008: 01101110 01100101 01100111 01100001 01110100 01110100 01010011 01110100  negattSt
                 n        e txt    7        a        t        t        S        t

00000016: 01101101 01110100 10100000 01101000 01100001 01110101 01110100 01101000  mt.hauth
                 m        t map    0 txt    8        a        u        t        h

00000024: 01000100 01100001 01110100 01100001 01011001 00000001 00000101 00010110  DataY...   ==> 25 means length is in 2 next bytes (total length 292, -261 = 31)
                 D        a        t        a byt   25               261 <rpIdHash

00000032: 10110000 00101101 11111011 11000011 11010100 11001100 10100011 01111110  .-.....~
00000040: 10111100 00101111 01100101 00010110 01100101 10011011 00010010 00100001  ./e.e..!
00000048: 00001101 10111001 11100001 00000001 10001010 10111001 11110001 00111010  .......:
00000056: 10010110 10010000 01100011 10001110 10100110 11111101 10101000 01000101  ..c....E   ==> flags UP:/:UV:/:/:/:AT:ED little-endian
                                                               rpIdHash> <flags >                        1 0  1 0 0 0  1  0

00000064: 00000000 00000000 00000000 00000001 00101111 11000000 01010111 10011111  ..../.W.   ==> signCount: 1
          <signCount                        > <aaguid
                                              <attestedCredentialData

00000072: 10000001 00010011 01000111 11101010 10110001 00010110 10111011 01011010  ..G....Z   ==> aaguid: 2fc0579f-8113-47ea-b116-bb5a8db9202a
00000080: 10001101 10111001 00100000 00101010 00000000 10000000 11101000 00101111  .. *.../   ==> credentialIdLength: 128
                                      aaguid> <credentialIdLen> <credentialId

00000088: 11100110 10111101 11100011 00000000 11100100 11101100 11001001 00111110  .......>
00000096: 00000000 00010110 01000100 10001010 11010000 00001111 10100110 11110010  ..D.....
00000104: 10001010 00000001 00011010 01101111 10000111 11111111 01111011 00001100  ...o..{.
00000112: 11111100 10100100 10011001 10111110 10101111 10000011 00110100 01001100  ......4L
00000120: 00110110 01100000 10110101 11101100 10101011 11110111 00101010 00111011  6`....*;
00000128: 00101000 00111000 10100000 11001100 01111101 10000111 11010011 11111010  (8..}...
00000136: 01011000 00101001 00101011 01010011 01000100 10011100 11111111 00010011  X)+SD...
00000144: 10101101 01101001 01110011 00101101 01110101 00100001 01100100 10011101  .is-u!d.
00000152: 00110110 01011100 11001011 11000101 11010000 10100000 11111010 01001011  6\.....K
00000160: 01001110 00001001 11101010 11101001 10010101 00110111 00100110 00011111  N....7&.
00000168: 00101111 01000100 00001001 00111111 10001111 01001111 11010100 11001111  /D.?.O..
00000176: 01010111 10010110 11100000 11111110 01011000 11111111 00000110 00010101  W...X...
00000184: 11111111 11000101 10001000 00101000 00110110 10111011 11100111 10111001  ...(6...
00000192: 10011011 00001000 10111110 00101001 10000110 01110010 00011100 00011100  ...).r..
00000200: 01011010 01101010 11000111 11110011 00101101 00110010 00100000 11011001  Zj..-2 .
00000208: 10110011 01001101 10001101 11101110 00101111 11001001 10100011 00000001  .M../...    ==> 1 -> kty: OKP
                                                  credentialId> map    3 uin    1
                                                                <credentialPublicKey

00000216: 01100011 01001111 01001011 01010000 00000011 00100111 00100000 01100111  cOKP.' g    ==> 3 -> alg: -8 (eddsa)
          txt    3        O        K        P uin    3 nin    7 nin    0 txt    7

00000224: 01000101 01100100 00110010 00110101 00110101 00110001 00111001 00100001  Ed25519!    ==> -1 -> crv: Ed25519
                 E        d        2        5        5        1        9 nin    1              ==> -2 -> should be public key (bstr)
                                                    credentialPublicKey> <extensions? (should be a map 101 not a negative integer 001)
                                                 attestedCredentialData>

00000232: 10011000 00100000 00010110 00011000 11110110 00011000 01011001 00011000  . ....Y.
          arr   24       32 uin   22 uin   24      246 uin   24       89 uin   24

00000240: 11111010 00011000 00101110 00010100 00011000 01110101 00011000 00111010  .....u.:
               250 uin   24       46 uin   20 uin   24      117 uin   24       58

00000248: 00011000 10000100 00010111 00011000 01010010 00011000 01110100 00011000  ....R.t.
          uin   24      132 uin   23 uin   24       82 uin   24      116 uin   24

00000256: 01111010 00011000 11000101 00010011 00011000 11011001 00011000 11000101  z.......
               122 uin   24      197 uin   19 uin   24      217 uin   24      197

00000264: 00011000 10000011 00011000 00101101 00011000 11101101 00011000 00011000  ...-....
          uin   24      131 uin   24       45 uin   24      237 uin   24       24

00000272: 00011000 11101010 00011000 10001111 00011000 00101110 00011000 01110100  .......t
          uin   24      234 uin   24      143 uin   24       46 uin   24      116

00000280: 00000111 00011000 01011110 00011000 11110100 00010101 00011000 11001100  ..^.....
          uin    7 uin   24       94 uin   24      244 uin   21 uin   24      204

00000288: 00011000 11001001 00011000 01101101                                      ...m
          uin   24      201 uin   24      109

If we follow strictly the webauthn doc, yes, there is some extra bytes. So to check if the problem comes from the library I use, I decide to try another one (yours).

I've write this little script:

import sys
from webauthn.helpers import (parse_attestation_object, base64url_to_bytes)

if __name__ == "__main__":
    if len(sys.argv) != 2:
        print("Expect one argument (filename)")
        exit(1)

    filename = sys.argv[1]
    print("decode", filename)
    file = open(filename, "r")
    base64 = file.read()
    bytes = base64url_to_bytes(base64)
    parse_attestation_object(bytes)
    
    pass

The file contains:

o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVkBBRawLfvD1MyjfrwvZRZlmxIhDbnhAYq58TqWkGOOpv2oRQAAAAEvwFefgRNH6rEWu1qNuSAqAIDoL-a94wDk7Mk-ABZEitAPpvKKARpvh_97DPykmb6vgzRMNmC17Kv3KjsoOKDMfYfT-lgpK1NEnP8TrWlzLXUhZJ02XMvF0KD6S04J6umVNyYfL0QJP49P1M9XluD-WP8GFf_FiCg2u-e5mwi-KYZyHBxaasfzLTIg2bNNje4vyaMBY09LUAMnIGdFZDI1NTE5IZggFhj2GFkY-hguFBh1GDoYhBcYUhh0GHoYxRMY2RjFGIMYLRjtGBgY6hiPGC4YdAcYXhj0FRjMGMkYbQ

But your lib also raised an error webauthn.helpers.exceptions.InvalidAuthenticatorDataStructure: Leftover bytes detected while parsing authenticator data.

So, did we really need to check that ?

I use a Yubikey 5 NFC (firmware 5.4.3) and have also opened an issue on the php lib web-auth/webauthn-framework#436

@MasterKale
Copy link
Collaborator

MasterKale commented Aug 15, 2023

Hello @Gashmob, thank you for the thorough breakdown. I see in web-auth/webauthn-framework#436 that this is due to malformed authData in the attestationObject reported by the authenticator during registration. Specifically a CBOR a3 (a list of three objects) should be a4 instead, but the authenticator incorrectly encodes the information. Flipping the byte seems to allow the response to parse as expected...interesting.

I'm evaluating a potential fix for this issue in this library. Stay tuned.

@MasterKale
Copy link
Collaborator

I've created #167 that should fix this issue. I'll let you know when it's released.

@MasterKale
Copy link
Collaborator

MasterKale commented Aug 15, 2023

@Gashmob Thank you for your patience. You'll be happy to hear that webauthn==1.10.1 is now available with a fix for this issue. Thank you again for reporting it ✌️

@MasterKale
Copy link
Collaborator

MasterKale commented Aug 15, 2023

Wow, @Gashmob, I grabbed this response from web-auth/webauthn-framework#436...

{
  "id": "ma2Y7hbtrzJtoDR4N2PkazhnrO6_58gZ8mO8epx-6aCnR9Jtio8Ge1w0_msV7HniYmLIH9yxOW8Yu_9ze_y8oj-MehAozj1jFTsjlQUEc_dxdzG5uFJTn6_RnzhulEWCcZZwcvlNTYne99MpWAD31c-4IuEr-eRRV1DWSANcax0",
  "rawId": "ma2Y7hbtrzJtoDR4N2PkazhnrO6_58gZ8mO8epx-6aCnR9Jtio8Ge1w0_msV7HniYmLIH9yxOW8Yu_9ze_y8oj-MehAozj1jFTsjlQUEc_dxdzG5uFJTn6_RnzhulEWCcZZwcvlNTYne99MpWAD31c-4IuEr-eRRV1DWSANcax0",
  "response": {
    "attestationObject": "o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVkBCRawLfvD1MyjfrwvZRZlmxIhDbnhAYq58TqWkGOOpv2oRQAAAAIvwFefgRNH6rEWu1qNuSAqAICZrZjuFu2vMm2gNHg3Y-RrOGes7r_nyBnyY7x6nH7poKdH0m2KjwZ7XDT-axXseeJiYsgf3LE5bxi7_3N7_LyiP4x6ECjOPWMVOyOVBQRz93F3Mbm4UlOfr9GfOG6URYJxlnBy-U1Nid730ylYAPfVz7gi4Sv55FFXUNZIA1xrHaMBY09LUAMnIGdFZDI1NTE5IZggCBjXGDcYzBgpGFwYlBgcGJYYTxjdGOYY8BjyGL4YPxg7GEgYfBh_GCIYKxhgChgmGIQYkhhQGH0Y1hjoGIk",
    "clientDataJSON": "eyJjaGFsbGVuZ2UiOiJvcTF2cGc3NHUtVG1xVzNEdjJMd1VfakgwME5RZjY1T3FwTWhydnI3eVBZIiwib3JpZ2luIjoiaHR0cHM6Ly90dWxlYXAtd2ViLnR1bGVhcC1haW8tZGV2LmRvY2tlciIsInR5cGUiOiJ3ZWJhdXRobi5jcmVhdGUifQ"
  },
  "clientExtensionResults": {},
  "type": "public-key"
}

And the credential public key is actually pretty messed up:

{1: 'OKP', 3: -8, -1: 'Ed25519', -2: [8, 215, 55, 204, 41, 92, 148, 28, 150, 79, 221, 230, 240, 242, 190, 63, 59, 72, 124, 127, 34, 43, 96, 10, 38, 132, 146, 80, 125, 214, 232, 137]}

kty is "OKP" instead of 1, and alg is "Ed25519" instead of 6, x is an array of numbers...

What kind of authenticator did you say you got this from? A YubiKey? Running which firmware?

Compare that with my YubiKey 5 running Firmware 5.4.3 - it returns what I'd expect given all of the other well-behaving authenticators I've interacted with thus far:

{1: 1, 3: -8, -1: 6, -2: b'8a\x86\x91\xbcx\xd1\xb3\x92\x87tKfq"\xd5\xcf\xf1~I\xf9\xc7\xeaA!\x9d\x01\x12\xabM\xf3Z'}

kty is 1, alg is 6, and x is bytes as expected.

I kinda want to revert #167 and say, "that authenticator should never be asked for Ed25519 public keys." 🤔

@MasterKale
Copy link
Collaborator

Huh, wild, if you look at Section 8.2 in RFC8152 it apparently spells out the requirement that kty be the string "OKP", but while alg should be the string "EdDSA" it's "Ed25519" instead.

I wonder if EC2 and RSA public keys use numbers for kty and alg because of this section of the spec:

https://www.w3.org/TR/webauthn-2/#sctn-encoded-credPubKey-examples

OKP public keys are underspecified in WebAuthn, I suppose unsurprisingly given how new support for Ed25519 in WebAuthn is. 🤔

@emlun
Copy link

emlun commented Aug 15, 2023

It looks like the RFC8152 is using 'OKP' as a human-friendly (though ironically confusing) alias of the actual value 1. It's also confusing that the type of kty is defined as tstr / int, but there are only integer values defined in the registry, both on the IANA registry page and in Section 13 of RFC8152:

   Key types are identified by the 'kty' member of the COSE_Key object.
   In this document, we define four values for the member:

   +-----------+-------+-----------------------------------------------+
   | Name      | Value | Description                                   |
   +-----------+-------+-----------------------------------------------+
   | OKP       | 1     | Octet Key Pair                                |
   | EC2       | 2     | Elliptic Curve Keys w/ x- and y-coordinate    |
   |           |       | pair                                          |
   | Symmetric | 4     | Symmetric Keys                                |
   | Reserved  | 0     | This value is reserved                        |
   +-----------+-------+-----------------------------------------------+

                         Table 21: Key Type Values

As far as I can tell, all actual values in the COSE registry (the "Value" and "Label" columns) seem to be integer typed.

@MasterKale
Copy link
Collaborator

Thanks for wading in here @emlun. I thought the string names might be allowed since the type was, as you noted, tstr / int but I'm glad to hear someone else say that it really should only ever be an int as per the values registered with IANA.

@sbweeden
Copy link

From the "COSE Algorithms" section of https://www.iana.org/assignments/cose/cose.xhtml, "Strings of length greater than 2" are supposedly under "Expert Review", though I must admit I don't entirely know what that means. Personally I suspect the structure is contrived, and that the values should be integers.

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 a pull request may close this issue.

4 participants