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

fix(pcd/webauthn-pcd): base64 encode expected challenge #2186

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

arthurgousset
Copy link
Contributor

@arthurgousset arthurgousset commented Dec 11, 2024

Description

  1. fix(pcd/webauthn-pcd): base64 encodes expected challenge. This fixes a bug in WebAuthnPCDPackage.verify. A reproducible example of the bug is included below.
  2. feat(apps/consumer-client): adds reproducible WebAuthn PCD bug in consumer-client app.

Other changes

  1. test(pcd/webauthn-pcd): base64 encode challenge in unit test
  2. chore(pcd/webauthn-pcd): temporarily enables any credential to be used for signing. The previous allowCredentials configuration, wouldn't find my 1Password credential. I'm not sure if this is a bug, or just on my machine. Happy to revert this commit before merging, but I couldn't reproduce the bug without enabling this.

Tested

Before bug fix

I added an example in the consumer-client app (as of commit 47398f3) to reproduce bug. You have to build and checkout the consumer-client at commit 47398f3 to reproduce the bug successfully.

Steps to reproduce:

  1. Run consumer client

    # in root directory
    $ yarn
    $ yarn build
    $ yarn dev
    
  2. Visit WebAuth example in consumer client: http://localhost:3001/#/examples/add-pcd

    image
  3. Register new WebAuthn credential (I used 1Password to store it). Works as expected ✅

    image
  4. Sign PCD with WebAuthn. Works as expected ✅

    image
  5. Verify PCD with WebAuthn. Fails because the given challenge does not equal expect challenge ❌

    image
    Error verifying with WebAuthn: Error: Unexpected authentication response challenge "IntcbiAgXCJBXCI6IDEyMyxcbiAgXCJCXCI6IDMyMSxcbiAgXCJDXCI6IFwiaGVsbG9cIixcbiAgXCJEXCI6IFwiZm9vYmFyXCIsXG4gIFwiRVwiOiAxMjMsXG4gIFwiRlwiOiA0Mjk0OTY3Mjk1LFxuICBcIkdcIjogNyxcbiAgXCJIXCI6IDgsXG4gIFwiSVwiOiA5LFxuICBcIkpcIjogMTAsXG4gIFwiS1wiOiAtNSxcbiAgXCJvd25lclwiOiB7XG4gICAgXCJjcnlwdG9ncmFwaGljXCI6IFwiMHgyOTVlNDdiNWQ4ZWFkNDFiYmI0YjlmZTMwYmExZGEwZjFlYWY4ZDUxNDZjZjBkNzE1M2QxODc4Y2IyOTA4OTUxXCJcbiAgfSxcbiAgXCJvd25lclY0XCI6IHtcbiAgICBcImVkZHNhX3B1YmtleVwiOiBcIjFuc1BHRjY2dXVEZkJucFFEKzdvLzlTUFg3TDBKRG4rbHViS2grUUd1UzhcIlxuICB9XG59Ig", expected ""{\n  \"A\": 123,\n  \"B\": 321,\n  \"C\": \"hello\",\n  \"D\": \"foobar\",\n  \"E\": 123,\n  \"F\": 4294967295,\n  \"G\": 7,\n  \"H\": 8,\n  \"I\": 9,\n  \"J\": 10,\n  \"K\": -5,\n  \"owner\": {\n    \"cryptographic\": \"0x295e47b5d8ead41bbb4b9fe30ba1da0f1eaf8d5146cf0d7153d1878cb2908951\"\n  },\n  \"ownerV4\": {\n    \"eddsa_pubkey\": \"1nsPGF66uuDfBnpQD+7o/9SPX7L0JDn+lubKh+QGuS8\"\n  }\n}""
    

    The given challenge is simply base64 encoded, while the expected challenge is not:

    # in terminal
    $ echo -n "IntcbiAgXCJBXCI6IDEyMyxcbiAgXCJCXCI6IDMyMSxcbiAgXCJDXCI6IFwiaGVsbG9cIixcbiAgXCJEXCI6IFwiZm9vYmFyXCIsXG4gIFwiRVwiOiAxMjMsXG4gIFwiRlwiOiA0Mjk0OTY3Mjk1LFxuICBcIkdcIjogNyxcbiAgXCJIXCI6IDgsXG4gIFwiSVwiOiA5LFxuICBcIkpcIjogMTAsXG4gIFwiS1wiOiAtNSxcbiAgXCJvd25lclwiOiB7XG4gICAgXCJjcnlwdG9ncmFwaGljXCI6IFwiMHgyOTVlNDdiNWQ4ZWFkNDFiYmI0YjlmZTMwYmExZGEwZjFlYWY4ZDUxNDZjZjBkNzE1M2QxODc4Y2IyOTA4OTUxXCJcbiAgfSxcbiAgXCJvd25lclY0XCI6IHtcbiAgICBcImVkZHNhX3B1YmtleVwiOiBcIjFuc1BHRjY2dXVEZkJucFFEKzdvLzlTUFg3TDBKRG4rbHViS2grUUd1UzhcIlxuICB9XG59Ig" | base64 --decode
    "{\n  \"A\": 123,\n  \"B\": 321,\n  \"C\": \"hello\",\n  \"D\": \"foobar\",\n  \"E\": 123,\n  \"F\": 4294967295,\n  \"G\": 7,\n  \"H\": 8,\n  \"I\": 9,\n  \"J\": 10,\n  \"K\": -5,\n  \"owner\": {\n    \"cryptographic\": \"0x295e47b5d8ead41bbb4b9fe30ba1da0f1eaf8d5146cf0d7153d1878cb2908951\"\n  },\n  \"ownerV4\": {\n    \"eddsa_pubkey\": \"1nsPGF66uuDfBnpQD+7o/9SPX7L0JDn+lubKh+QGuS8\"\n  }\n}

After bug fix

Steps to reproduce:

  1. Run consumer client

    # in root directory
    $ yarn
    $ yarn build
    $ yarn dev
    
  2. Visit WebAuth example in consumer client: http://localhost:3001/#/examples/add-pcd

    image
  3. Register new WebAuthn credential (I used 1Password to store it). Works as expected ✅

    image
  4. Sign PCD with WebAuthn. Works as expected ✅

    image
  5. Verify PCD with WebAuthn. Fails because the given challenge does not equal expect challenge

    image

Related issues

None. We discussed this in the Telegram group.

Backwards compatibility

Yes, fixes bug in existing WebAuthn PCD library.

Documentation

None beyond this PR.

@arthurgousset arthurgousset changed the title fix(pcd): Webauthn PCD fix(pcd/webauthn-pcd): base64 encode expected challenge Dec 11, 2024
@arthurgousset arthurgousset marked this pull request as ready for review December 11, 2024 16:57
Copy link
Member

@robknight robknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the issue with the message claiming success when verification fails, this looks good to me.

Verification seems to fail for me using Firefox and Bitwarden, but I think this is due to my configuration being weird and not an underlying issue - it works fine in Safari and Chrome.

}
try {
await verifyWebAuthnPCD(serializedWebAuthnPCD);
console.log("Verified successfully with WebAuthn");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message seems to be incorrect in the case where verifyWebAuthnPCD did not succeed. However, since it returns Promise<void>, there's no way to know here if the PCD was verified or not.

This can cause confusing messages like this:

image

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.

2 participants