Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Systemic presence of extra byte for registration with eddsa #436

Closed
Gashmob opened this issue Jul 10, 2023 · 11 comments · Fixed by #437
Closed

Systemic presence of extra byte for registration with eddsa #436

Gashmob opened this issue Jul 10, 2023 · 11 comments · Fixed by #437
Assignees
Labels
ongoing investigation Trying to find what's wrong

Comments

@Gashmob
Copy link
Contributor

Gashmob commented Jul 10, 2023

Version(s) affected

4.5.1

Description

In goal to register a passkey on a website, I use your sub-library webauthn. 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 (L146 in AttestationObjectLoader.php). If I comment this line then all is good.

I've tried to decompose the attestion 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. I've also tried to use the duo-labs in python to check the response and have exactly the same problem: an error is raised for presence of extra bytes.

Did we really need to check that ?

How to reproduce

In backend, to generate options I do:

PublicKeyCredentialCreationOptions::create(
            $this->relying_party_entity,
            $user_entity,
            $challenge,
            [
            new PublicKeyCredentialParameters(PublicKeyCredentialDescriptor::CREDENTIAL_TYPE_PUBLIC_KEY, Algorithms::COSE_ALGORITHM_EDDSA),
            new PublicKeyCredentialParameters(PublicKeyCredentialDescriptor::CREDENTIAL_TYPE_PUBLIC_KEY, Algorithms::COSE_ALGORITHM_ES256),
            new PublicKeyCredentialParameters(PublicKeyCredentialDescriptor::CREDENTIAL_TYPE_PUBLIC_KEY, Algorithms::COSE_ALGORITHM_RS256),
        ]
        )
            ->setAttestation(PublicKeyCredentialCreationOptions::ATTESTATION_CONVEYANCE_PREFERENCE_NONE)
            ->excludeCredentials(...$registered_sources)->jsonSerialize();

These options are sent to front as they are. Then I use this nodejs lib https://simplewebauthn.dev/docs/packages/browser/, and the result is sent back to the backend:

$attestation_statement_manager = new AttestationStatementSupportManager();
        $attestation_statement_manager->add(new NoneAttestationStatementSupport());
$this->credential_loader = new PublicKeyCredentialLoader(
                new AttestationObjectLoader($attestation_statement_manager)
            )
// ...
$this->credential_loader->loadArray($response)

Possible Solution

No response

Additional Context

The passkey used is a Yubikey 5 NFC (firmware 5.4.3)

@Spomky
Copy link
Contributor

Spomky commented Jul 10, 2023

Hi @Gashmob,

Thank you for the detailed report. I tried to reproduce it with a Yubikey 5 (firmware 5.1.2) and I cannot reproduce that issue.
Could you please share the credential options and response as JSON object?

Many thanks.
Regards.

@Spomky Spomky self-assigned this Jul 10, 2023
@Spomky Spomky added the ongoing investigation Trying to find what's wrong label Jul 10, 2023
@LeSuisse
Copy link

Hi,

I tried to reproduce it with a Yubikey 5 (firmware 5.1.2) and I cannot reproduce that issue.

Same on my side but our Yubikeys does not support EdDSA:

YubiKeys currently support RS256 (Firmware 5.1.X and below only), EdDSA, ES256, and Ed25519 (Firmware 5.2.X and above only).

@Gashmob
Copy link
Contributor Author

Gashmob commented Jul 11, 2023

The options:

{
	"rp": {
		"name": "Tuleap",
		"id": "tuleap-web.tuleap-aio-dev.docker"
	},
	"user": {
		"name": "admin",
		"id": "MTAx",
		"displayName": "Site Administrator"
	},
	"challenge": "oq1vpg74u-TmqW3Dv2LwU_jH00NQf65OqpMhrvr7yPY",
	"pubKeyCredParams": [
		{
			"type": "public-key",
			"alg": -8
		},
		{
			"type": "public-key",
			"alg": -7
		},
		{
			"type": "public-key",
			"alg": -257
		}
	],
	"attestation": "none"
}

And the response:

{
	"clientExtensionResults": {},
	"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"
	},
	"type": "public-key"
}

@Spomky
Copy link
Contributor

Spomky commented Jul 11, 2023

Many thanks for the additional input.
I think I found the issue. The associated public key seems to be truncated. The received data is a301634f4b5003272067456432353531392198200818d7183718cc1829185c1894181c1896184f18dd18e618f018f218be183f183b1848187c187f1822182b18600a1826188418921850187d18d618e81889
This corresponds to {1: "OKP", 3: -8, -1: "Ed25519"}, but this is not a valid public key as the x component is missing. It seems it is part of the remaining data (to be confirmed). (3:-8 corresponds to alg: 'EdDSA')

It tried to read the data on https://cbor.me/, but the result is the same: the object is not valid.
My assumption is that the browser does not correctly encode the authenticator data.
I may be wrong and I will investigate more on that.
In the meantime, would you mind to test with another browser?

@Spomky
Copy link
Contributor

Spomky commented Jul 11, 2023

Hi,

I tried to reproduce it with a Yubikey 5 (firmware 5.1.2) and I cannot reproduce that issue.

Same on my side but our Yubikeys does not support EdDSA:

YubiKeys currently support RS256 (Firmware 5.1.X and below only), EdDSA, ES256, and Ed25519 (Firmware 5.2.X and above only).

Excellent. Many thanks for the information. I was not aware of those details. Unfortunately, it is not possible to upgrade to the latest firmware version

@Gashmob
Copy link
Contributor Author

Gashmob commented Jul 12, 2023

but this is not a valid public key as the x component is missing. It seems it is part of the remaining data (to be confirmed). (3:-8 corresponds to alg: 'EdDSA')

When I've decoded the binary by hand, I've that authenticator data length is 261 bytes, it's coherent, it's exactly the amount that remains. But when I was at the public key cbor map, the lenght of the map was 3: key type, algorithm and curve. But yes, for okp key we need the also the field -2 for x (https://www.rfc-editor.org/rfc/rfc9053#name-octet-key-pair). If we assume that the map has length 4, the next field is -2, but its value is not a byte string but an array of 32 unsigned int. Even if it was extensions (but extension flag is set to 0), the format was not valid, I've found first a negative integer and not a map.

In the meantime, would you mind to test with another browser?

I've tried on firefox and chrome, but same problem on each.

@Spomky
Copy link
Contributor

Spomky commented Jul 12, 2023

This is exactly my conclusion.
In the extracted data a301634f4b50032720674..., if you change a3 into a4 (i.e. a map of 4 items instead of 3), the whole data is decoded as expected and the result is as follows:

{
    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
    ]
}

The key seems to be correct because we are expecting 32 bytes for the x component.
What I can do is detect malformed public keys and create a workaround to restructure the data.
EDIT: by "restructure" I also mean changing the byte array into a binary string.

I've tried on firefox and chrome, but same problem on each.

If it doesn't come up for the browser, I guess the key itself is causing the problem.
I will ask Yubico if they are aware of this issue and if it can be resolved.

@Spomky Spomky linked a pull request Jul 12, 2023 that will close this issue
5 tasks
@Spomky
Copy link
Contributor

Spomky commented Jul 12, 2023

Hi @Gashmob,

I am going to release a bug fix. See #437.
Before that, it is possible for you to test this dev branch and make sure everything is fine?

@Gashmob
Copy link
Contributor Author

Gashmob commented Jul 13, 2023

Hi, it works well 👍 Thank you!

@Spomky
Copy link
Contributor

Spomky commented Jul 15, 2023

Closing as merged. Will be released soon

@Spomky Spomky closed this as completed Jul 15, 2023
Gashmob added a commit to Gashmob/webauthn-framework that referenced this issue Jul 25, 2023
Closes web-auth#448

Previously added for issue web-auth#436, the field kty must be text string or
int but not byte string (rfc8152 section 7)
Spomky added a commit that referenced this issue Jul 26, 2023
* Fix wrong creation of eddsa public key

Closes #448

Previously added for issue #436, the field kty must be text string or
int but not byte string (rfc8152 section 7)

* Add tests to check if fixed OKP key can be check by validator

issue #448

* Fix coding standard & cose-lib dependency version in webauthn package

* cose-lib 4.2.3+ required

---------

Co-authored-by: Florent Morselli <florent.morselli@spomky-labs.com>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
ongoing investigation Trying to find what's wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants