SimpleWebAuthn: Error prone toUint8Array method

PR #42 has been merged in with a fix for this issue. It is available in the newly released v0.8.0.

@mahnunchik #42 is the PR containing my tentative solution to this issue. Fortunately it’s not as breaking a change as I described earlier, and more importantly it manages a variety of values for challenge including ascii strings, utf-8 strings, and crypto.randomBytes(64).

And for good measure I hand-checked the values being passed to authenticators by startAttestation()/startAssertion() to confirm that they were the actual strings/buffers I was passing in as challenge to generateAttesationOptions() and generateAssertionOptions() respectively.

Another opinion: challenge for the server library is too storage related (*sql, mongo, redis etc) so it may be declared as out of scope of simple library.

Good readme how to generate challenge (crypto.randomBytes(64)) should be enough for the user.

My plan is to update Server to make the challenge parameter in generateAttestationOptions() and generateAssertionOptions() an optional string | Buffer. If no value for challenge is provided, the library will generate one. If a value for challenge is provided, it’ll be used instead.

The return values from these methods will now include the raw challenge, whether it was passed in or generated by the library:

// Something like this
const { options, rawChallenge } = generateAttestationOptions({...});

// Providing a challenge
const challenge = 'thisIsABadValueLol';
const { options, rawChallenge } = generateAttestationOptions({..., challenge });
assert(rawChallenge === challenge); // true

This will make it possible to pass directly into Server’s verify...response() methods.

This’ll be a breaking change, but it’ll be worth it because it’ll simplify and secure the use of the library even more ✌️

Awesome! We made a progress!

Thank you for your patience, it’s a subtle flaw in the system and I was having a hard time seeing it.

I think it should be possible to keep simplicity of library and solve the issue by the following:

You’re absolutely right, everything needed to encode and decode base64url already exist in both the front end and the back end. In fact I should be able to get rid of toUint8Array() completely after updating Browser accordingly. I’ll go ahead and make these changes.

I’m also going to update generateAttestationOptions() and generateAssertionOptions() to generate challenges internally. I thought it’d be okay to leave it to the implementer to construct sufficiently-complex challenges, but after you’re talking this through with me I realized the library should be in charge of generating these values because of all the gotcha’s associated with it.

Okay, I see what’s going on now. toUint8Array() doesn’t convert stringForClient back to bytes, it converts the base64url-encoded representation of bytes to a Uint8Array, and so of course uint8array !== bytes.

Awesome! We made a progress!

is there a sufficient security argument to be made for supporting arbitrary byte sequences as challenges instead of only supporting strings? Isn’t a 64-character string containing random characters sufficient entropy for the challenge?

What you think in term of security? “640K ought to be enough for anybody.”

I think it should be possible to keep simplicity of library and solve the issue by the following:

1. Use base64url.encode in the server package to convert challenge. It accepts true Buffer and/or String as you like.
2. Replace toUint8Array by base64URLStringToBuffer to convert challenge to Uint8Array.

It allows to use any source for the challenge: crypto.randomBytes, any strings or even some special random source.