MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

Custom challenge not correctly decoded in expectedChallenge #480

Closed damianobarbati closed 10 months ago

damianobarbati commented 1 year ago

It's not clear how to pass then verify the custom challenge from server to client, then back to server, in the library. I'm providing:

What is the intended way to verify the challenge defined plain in generateRegistrationOptions? Shouldn't verifyRegistrationResponse decode properly the challenge returned back?

For now I'm currently working around it with this:

  const verification = await verifyRegistrationResponse({
    response: ctx.request.body,
    expectedChallenge: base64url.encode('hello'), // encode challenge 
    expectedOrigin: origin,
  });
MasterKale commented 1 year ago

Are you using something other than @simplewebauthn/browser on the front end to process and pass the options to navigator.credentials.create()? Because I did the following and it worked just fine:

// Options
const options = await generateRegistrationOptions({
  // ...
  challenge: 'hello',
});
req.session.currentChallenge = options.challenge;

// Browser
// Feed `options` into @simplewebauthn/browser's `startRegistration()`

// Server
const verification = await verifyRegistrationResponse({
  // ...
  expectedChallenge: isoBase64URL.fromString('hello'),
});
MasterKale commented 1 year ago

Though honestly I hardly ever recommend specifying your own value for challenge. It's supposed to be unique on every registration and authentication, and never reused, so I advise letting my library handle it by omitting a value for challenge when calling generateRegistrationOptions().

payton commented 1 year ago

Hey @MasterKale - First of all, thank you for building this out! It's such a strong tool that abstracts webauthn beautifully.

On the topic of this issue, I had similar initial thoughts to @damianobarbati. I was mostly surprised by the interface requiring me to base64 encode the challenge when it was not base64 encoded on calling generateRegistrationOptions().

Though honestly I hardly ever recommend specifying your own value for challenge.

In my case, I have an existing challenge flow, so it's more natural to bring my own challenge (that's not to say it couldn't be refactored, but not ideal).

MasterKale commented 1 year ago

2023-Me is looking at the challenge argument in generateRegistrationOptions() and wondering why it was only ever a string until recently, as it leads to a weird use case like what you're describing here.

What is the intended way to verify the challenge defined plain in generateRegistrationOptions? Shouldn't verifyRegistrationResponse decode properly the challenge returned back?

However, after reviewing my own documentation...

  1. https://simplewebauthn.dev/docs/packages/server#1-generate-registration-options
  2. https://simplewebauthn.dev/docs/packages/server#2-verify-registration-response
  3. https://github.com/MasterKale/SimpleWebAuthn/blob/master/example/index.ts#L155-L159

...I've always advised SimpleWebAuthn users to persist options.challenge out of generateRegistrationOptions() to then feed back into verifyRegistrationResponse() as expectedChallenge.

In my case, I have an existing challenge flow, so it's more natural to bring my own challenge (that's not to say it couldn't be refactored, but not ideal).

This sounds like a legitimate use case for specifying your own challenge argument when calling generateRegistrationOptions(). But is there something preventing you from persisting options.challenge afterwards to use later in verifyRegistrationResponse()?

For the majority of users, if you use SimpleWebAuthn as documented then you won't have to know to base64url-encode your challenge value when you pass it in as expectedChallenge. I can think about ways to make it easier to reuse a value between challenge and expectedChallenge, but I still think it's simpler for my library users to disregard the value of options.challenge and simply dump it back into verifyRegistrationResponse() as I document.

payton commented 1 year ago

This sounds like a legitimate use case for specifying your own challenge argument when calling generateRegistrationOptions(). But is there something preventing you from persisting options.challenge afterwards to use later in verifyRegistrationResponse()?

The lift is pretty low to do this! I think the larger callout here is that two separate parties ended up coming to the same workaround, which felt like the most "natural" fix for us.

My general thought process for challenge-based auth is:

  1. Generate and store a reference to the challenge
  2. Build the message with generated challenge (generateRegistrationOptions)
  3. Return the message for the client to sign

My choice to persist my own challenge immediately after generation felt more natural, and I view generateRegistrationOptions as more of a challenge formatter than something that will transform the input challenge.

This isn't something that I was planning on calling out, but I noticed the issue already existed, so I figured it may be helpful to add my own 2c here.

For the majority of users, if you use SimpleWebAuthn as documented then you won't have to know to base64url-encode your challenge value when you pass it in as expectedChallenge.

Agreed!

MasterKale commented 1 year ago

Thank you for the feedback @payton and @damianobarbati, honestly. It's good for me to review older code like this from time to time and practical problems like the one that you both independently came upon are perfect for helping me frame potential improvements.

My general thought process for challenge-based auth is:

  1. Generate and store a reference to the challenge
  2. Build the message with generated challenge (generateRegistrationOptions)
  3. Return the message for the client to sign

This totally makes sense to me. I think I can do better for more advanced use cases like yours' without sacrificing the ease-of-use for the majority of users who don't want/need to otherwise bother with challenge generation.

I'll noodle on this over the holidays, see what comes to mind 🦃 🎄

MasterKale commented 10 months ago

I did some brainstorming today and I can't yet see a win-win scenario.

First I considered stopping base64url-encoding challenge values in generate...Options() methods when they're strings. That would enable RP's, like you, to specify the same value (e.g. "hello") for challenge and expectedChallenge when calling @simplewebauthn/server methods.

However, authenticators always return the challenge bytes as base64url-encoded in clientDataJSON:

Screenshot 2023-12-29 at 6 02 39 PM

Thinking ahead a little bit, I could see this being a potential trade-off during debugging. It's easy to take a WebAuthn response, paste it into https://debugger.simplewebauthn.dev, and copy-paste the value needed for the expectedChallenge argument. If expectedChallenge was modified to be the base64url-decoded value instead, though, then that would require RP devs to know to decode clientDataJSON.challenge to confirm the challenge they passed in as challenge when generating options. I guess I could update the debugger to do that work for me (...and maybe this particular issue I'm foreseeing is a "me" problem that'll make it less straightforward for me to confirm issues that people open here 🤔)

Looking at the history of this project, the expectedChallenge arguments in both verification methods have been documented in the code with this description since July 2020 (way back in #42):

The base64url-encoded options.challenge returned by generateRegistrationOptions()

(emphasis mine)

Making changes to accommodate your specific use case isn't a clear winner in my book, so at this moment in time I'm not inclined to make any functional changes to support it. As

I'm open to suggestions for how I might improve documentation to communicate the need to base64url-encode custom challenges, though. Perhaps something under Advanced Guides in the docs? It could more explicitly document what we talked about in here, about needing to use isoBase64URL.fromString() when feeding your custom string value for challenge in as expectedChallenge later 🤔

MasterKale commented 10 months ago

I've added a new Advanced Guide on how to use custom strings as challenges, including during verification. It captures the learnings from this issue in a way that should hopefully help clear this up for others who have similar use cases as yours:

https://simplewebauthn.dev/docs/advanced/server/custom-challenges#use-a-custom-string-for-challenge