web-auth / webauthn-framework

FIDO-U2F / FIDO2 / Webauthn Framework
MIT License
403 stars 53 forks source link

Type-Error #505

Closed Nevercold closed 10 months ago

Nevercold commented 10 months ago

Version(s) affected

4.7.3

Description

I use "SimpleWebAuthn" in the frontend https://github.com/MasterKale/SimpleWebAuthn Since I upgraded to the latest version (4.7.3) from (4.6), the error "Type Error" occurs there, which has to do with the transports in the option request. The transports are stored as "usY" (as an example) instead of "usb" and are also output like this in allowCredentials. This leads to an error that did not exist at that time. I have already created a discussion at SimpleWebAuthn where this turned out to be the problem. https://github.com/MasterKale/SimpleWebAuthn/discussions/470

Please Help, thanks!

How to reproduce

Sample Code: https://gist.github.com/Nevercold/0d401686c7e0db911b047396fe0052d0 Output:

{
        "challenge": "MmUzMmNhYTBlNTBhZjQ4MGUwZjc4ZjU3M2I3OTNlZTE1OTc5M2M2ZGE0YzIxZTJmNjc3N2ZmNDViYzI4ZjJmOA",
        "rpId": "...",
        "userVerification": "discouraged",
        "allowCredentials": [
            {
                "type": "public-key",
                "id": "...",
            },
            {
                "type": "public-key",
                "id": "...",
                "transports": [
                    "internal",
                    "hybriQ"
                ]
            },
            {
                "type": "public-key",
                "id": "...",
                "transports": [
                    "nfc",
                    "usY"
                ]
            }
        ],
        "timeout": 60000
}

Possible Solution

No response

Additional Context

No response

Spomky commented 10 months ago

Hello @Nevercold,

🤔 Honestly, I do not see any reason to have such weird values. The library just receive the list of transport and use it as-it-is without modification of any kind.

I suspect a bad encoding or a modification in the way the data is fetch or saved in the DB.

Could you please share the following information?

In addition, when you downgrade to the version 4.6.x, is it working as expected? Is there any difference between existing data and new authenticators since v4.7.x?

Many thanks. Regards.

Nevercold commented 10 months ago

Hey! here is a stored PublicKeyCredentialSource

{
    "publicKeyCredentialId": "...",
    "type": "public-key",
    "transports": [
        "nfc",
        "usY"
    ],
    "attestationType": "basic",
    "trustPath": {
        "type": "Webauthn\\TrustPath\\CertificateTrustPath",
        "x5c": [...]
    },
    "aaguid": "2fc0579f-8113-47ea-b116-bb5a8db9202a",
    "credentialPublicKey": "...",
    "userHandle": "OGNhM2ExOTYtY2UxMy0xMWVkLThiNTctZGU3MzdiZDI1NTIw",
    "counter": 2,
    "otherUI": null
}

I store the data simply via PHP-PDO and write them directly to the database. (collation utf8mb4_general_ci)

$publicKeyCredentialSource = self::authenticatorAttestationResponseValidator()
        ->enableMetadataStatementSupport(new MetadataStatementRepository(), new StatusReportRepository(), new PhpCertificateChainValidator(new Client(), Fido2::Psr17Factory()))
        ->check($authenticatorAttestationResponse,
          PublicKeyCredentialCreationOptions::createFromArray($publicKeyCredentialCreationOptions), $serverRequest);

$publicKeyCredentialSource->jsonSerialize() <-- this saved to database

I just downgraded again to the 4.6.3, and there the output is also with "Y" (usY). However, the keys were also created in this version. The same is on the 4.7.3. The problem with the type error also only occurs on Apple devices, on all others this is not a problem, so I probably did not notice it directly when testing.

Spomky commented 10 months ago

😬$publicKeyCredentialSource->jsonSerialize() <-- this saved to database

I think the error comes from this line. As I mentioned in another issue, jsonSerialize is not supposed to be called directly, but used by json_encode. It should be json_encode($publicKeyCredentialSource) instead -with optional flags if needed (in general, I had at least JSON_THROW_ON_ERROR)-

Nevercold commented 10 months ago

Same problem, json_encode($publicKeyCredentialSource) just added

I also had it with json_encode at that time, but had changed it during the upgrade (for whatever reason)

{
   "publicKeyCredentialId":"...",
   "type":"public-key",
   "transports":[
      "nfc",
      "usY"
   ],
   "attestationType":"basic",
   "trustPath":{
      "type":"Webauthn\\TrustPath\\CertificateTrustPath",
      "x5c":[
         "-----BEGIN CERTIFICATE-----....-----END CERTIFICATE-----\n"
      ]
   },
   "aaguid":"2fc0579f-8113-47ea-b116-bb5a8db9202a",
   "credentialPublicKey":"...",
   "userHandle":"OGNhM2ExOTYtY2UxMy0xMWVkLThiNTctZGU3MzdiZDI1NTIw",
   "counter":1,
   "otherUI":null
}
Spomky commented 10 months ago

Many thanks. That is really a strange behaviour. The library does nothing with the list and the DB operations look good. So it may come from the data received from the client. Could you please share the options generated by the library and the authentication response as JSON objects? I would like to make sure there is no decoding issue (maybe CBOR decoding). Do you use MasterKale/SimpleWebAuthn directly or the stimulus controller from this project?

Nevercold commented 10 months ago

I have found the problem, it has nothing to do with this library.

I had an old function to fix the base64 padding, that was switched in before the response was processed, and that's what caused the error. I have now removed it and the problem is solved.

Sorry for the trouble, and thanks for the help.

Spomky commented 10 months ago

Hi, No problem. I am happy to read that this issue is resolved.

github-actions[bot] commented 9 months ago

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.