Error prone toUint8Array method

[mahnunchik]

I've faced that toUint8Array method treats input as ASCII string: https://github.com/MasterKale/SimpleWebAuthn/blob/bc9ad0f68fc49c4ac23cd22428248faa26d3f9b6/packages/browser/src/helpers/toUint8Array.ts#L6

It is impossible to change transformation of challenge on the client.

1. Attestation https://github.com/MasterKale/SimpleWebAuthn/blob/bc9ad0f68fc49c4ac23cd22428248faa26d3f9b6/packages/browser/src/methods/startAttestation.ts#L27
2. Assertion https://github.com/MasterKale/SimpleWebAuthn/blob/bc9ad0f68fc49c4ac23cd22428248faa26d3f9b6/packages/browser/src/methods/startAssertion.ts#L27

Whats wrong?

First: Challenge verification will fail if challenge string contains non ASCII character. For example: abcж yep, ж occupy 2 bytes.

const value = 'abcж';

const array = Uint8Array.from(value, c => c.charCodeAt(0));
const string = String.fromCharCode.apply(null, array);

/// 'abcж' !== 'abc6'
value !== string

Second: user may want to transfer challenge from server in different format:

• ASCII string - current assumption
• HEX string - I prefer this format
• OCTET string
• base64 or base64url strings
• and so on

So it would be better to allow user configure challenge encoding or move conversion and appropriate helper(s) to userland instead hard coded into the lib.

[MasterKale]

Thank you for reporting this, I think it's a fair point that the library should support UTF-8 strings. I'll look to fix that in the next release.

Regarding other potential challenge formats: the primary reason ASCII is assumed is because challenges need to be transmittable as application/json content. While I'm willing to concede the point that UTF-8 encoding can be considered de facto and so should be supported, I hesitate to commit to introducing support for arbitrary encoding/decoding of challenges. When a simple ASCII guid is sufficient as a challenge, where's the value in the library supporting myriad other encodings? The name of the library is SimpleWebAuthn, after all 😄

[MasterKale]

UTF-8 values in challenges should now be supported in the just-released @simplewebauthn/browser v0.7.3.

[mahnunchik]

Hi, thank you.

For now it is impossible to use crypto.randomBytes(64).toString('hex') as a challenge because it doesn't have any sense to encode random bites as a UTF-8 string to transfer to client.

Also it is not mandatory to transfer data as application/json so it should be more appropriate to encode data as base64url (url safe) but not as a UTF-8 string.

I hope it should be possible to opt out of mandatory conversion of challenge as UTF-8 string to use own convention even in Simple library.

[mahnunchik]

@MasterKale One more argument against sending challenge as a UTF-8 string is that challenge came back from the client as a base64url string: https://github.com/MasterKale/SimpleWebAuthn/blob/e9ef3215693225203920e39707cc6abeca25ae44/packages/server/src/assertion/verifyAssertionResponse.ts#L79

So for consistency and to keep the library simple it would be better to send challenge from server to client (and decode as Uint8Array) as base64url string.

[MasterKale]

One of the bigger pain points of WebAuthn is that you have to pass in Buffers for things like challenge and user.id. The challenge is in transferring those server-generated values to the front end. As a long-time front end dev, I'm well aware that JSON is the de-facto method of getting those values from the back end to the front end. JSON doesn't support transferring arbitrary byte sequences unless you encode them to some kind of string, of which ascii and utf-8 encoding are the most common.

For now it is impossible to use crypto.randomBytes(64).toString('hex') as a challenge because it doesn't have any sense to encode random bites as a UTF-8 string to transfer to client.

You absolutely can use crypto.randomBytes(64).toString('hex') as a value for challenge when generating options for and verifying attestations because the call to .toString('hex') returns an ASCII-encoded string of characters.

So for consistency and to keep the library simple it would be better to send challenge from server to client (and decode as Uint8Array) as base64url string.

During attestation and assertion the authenticator takes the arbitrary-challenge-string-convereted-to-Buffer and base64url encodes it, which is why we encode the challenge to base64url within verifyAssertion(). There's no real benefit to double-encoding such values at any point in this chain...

[MasterKale]

And to clarify a "base64/base64url-encoded" string is a valid ascii-encoded string.

[mahnunchik]

Yep, base64/base64url-encoded is a valid ascii string but it uses subset of 0-255 ascii symbols mapped to Uint8Array.

Decoding hex string as a ascii string (as you suggested) uses only 0-15 numbers for each of Uint8Array slot (numbers 0-255).

Yep, it works but it is requires dirty hack on server side to convert true buffer of random bytes from crypto.randomBytes to hex decoded as ascii...

[MasterKale]

What kind of "dirty hack" on the back end are you referring to? I used this when calling generateAttestationOptions() in the example project and it worked without issue:

const challenge = crypto.randomBytes(64).toString('hex');

[mahnunchik]

Ok, let's go step by step:

1. WebAuthn requires Buffer (ArrayBuffer) to be passed.
2. It is common to pass Uint8Array (Implementation of ArrayBuffer).
3. Each element of Uint8Array is integer value in range 0-255.
4. When you convert any string to Uint8Array when each character is mapped to Uint8 (value between 0-255).

The issue: when you convert hex string to Uint8Array when you use only integers from from 48 (0) to 57 (9) 97 (a) to 102 (f).

The ascii and utf-8 strings have the same issue but not as clearly as hex because they have large cardinality. Usually you should not use symbols from 0 to 31 https://en.wikipedia.org/wiki/ASCII

[MasterKale]

Okay, so this is an argument against encoding bytes to hex since its representation in ascii doesn't use the full 0-255 range of characters? I guess I see how that could limit entropy when used as a challenge generation scheme, but not how it negatively affects SimpleWebAuthn's functionality as challenge generation happens outside of the library. SimpleWebAuthn expects a string simply because it can't prepare a JSON-compatible payload with values that are raw bytes.

According to the WebAuthn spec, the goal of the challenge is to prevent replay attacks by containing a lot of entropy:

In order to prevent replay attacks, the challenges MUST contain enough entropy to make guessing them infeasible. Challenges SHOULD therefore be at least 16 bytes long.

It sounds to me like it's up to the implementer to decide a challenge structure that's suitable for their use case. If it's decided that hex is unsuitable for generating enough entropy when represented as an ascii-encoded string then there are alternative encodings that can be used when generating challenges. In that case I'd end up interpreting your request as one to update the docs to indicate preferred ways to generate string values for the challenge parameter of generateAttestationOptions().

[MasterKale]

I guess what I'm really trying to get to is, what kind of improvement to SimpleWebAuthn are you requesting?

[mahnunchik]

challenge generation happens outside of the library

Yep, but the user have to convert it to utf-8 string to be compatible with @simplewebauthn/browser library. Not every sequence of bytes could be converted to valid utf-8 string.

let's come from the other end. @simplewebauthn/browser library and underlaying navigator.credentials returns challenge as a base64url string. Why the library invents new format of ascii/binary string? Why not just pass to the library challenge as the same base64url string string to keep input and output similar and simple?

Any sequence of bytes may be encoded and transferred in JSON as base64url string even ascii and utf-8 strings and raw buffer.

So the simple option:

• Input: base64url
• Output: base64url

Strange option:

• Input: utf8 string which will be encoded to buffer of bytes as char codes.
• Output: base64url

[MasterKale]

Any sequence of bytes may be encoded and transferred in JSON as base64url string even ascii and utf-8 strings and raw buffer.

So the simple option:

• Input: base64url
• Output: base64url

This is currently supported, though...

const challenge = base64url.encode(crypto.randomBytes(64));
inMemoryUserDeviceDB[loggedInUserId].currentChallenge = challenge;

res.send(generateAttestationOptions({ ..., challenge });

SimpleWebAuthn enables devs to pick their poison with respect to converting an arbitrary Buffer to a string representation, base64url included.

This discussion has got me wondering if SimpleWebAuthn shouldn't internalize challenge generation too. generateAttestationOptions() could return options and the base64url string it used for challenge so that this can be solved in an opinionated manner that avoids too much bikeshedding...

[mahnunchik]

This is currently supported, though...

Nope, It's look like it is supported. The issue is the same: to convert to UInt8Array with 0-255 values used alphabet with 64 different values. Because toUint8Array(requestOptionsJSON.challenge) is hard coded and user could not opt out using it.

Some code:

// bytes is Buffer which is Uint8Array https://nodejs.org/api/buffer.html#buffer_buffer
const bytes = crypto.randomBytes(64);

// string to pass to client library @simplewebauthn/browser
const stringForClient = base64url.encode(bytes);

// from helpers of client library https://github.com/MasterKale/SimpleWebAuthn/blob/master/packages/browser/src/helpers/toUint8Array.ts
const uint8array = toUint8Array(stringForClient);

// buffer generated on server not equal buffer passed to navigator.credentials
uint8array !== bytes

Do you understand me?

[MasterKale]

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.

While I consider this further I'm wondering if you can answer this: 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?

[mahnunchik]

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.

[MasterKale]

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.

[mahnunchik]

Yep build-in challenge generator would simplify usual use cases.

But, please leave the option to pass user defined Buffer/String challenge to generateAttestationOptions and generateAssertionOptions.

Motivation:

• Some users may want use short challenge 16 bytes long to reduce communication package size.
• In opposite to increase security it may be necessary to use challenge 64 bytes or even more.
• Challenge storage question: fixed or dynamic length?

It makes no sense to provide all the options in the library, but the ability to pass custom challenge as Buffer/String instead of default would be awesome.

[MasterKale]

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 :v:

[mahnunchik]

Some thoughts:

• If the library uses user provided challenge then the raw output is not necessary because user already has raw data for challenge to pass it to verify methods.
• If the library generates default challenge then user may store base64url options.challenge but it requires verify methods to supports expectedChallenge as string | Buffer | base64url.

🤔

[mahnunchik]

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.

[MasterKale]

If the library uses user provided challenge then the raw output is not necessary because user already has raw data for challenge to pass it to verify methods.

It'll get returned anyway for sake of a consistent return value. In those situations the implementer can simply ignore it 😄

If the library generates default challenge then user may store base64url options.challenge but it requires verify methods to supports expectedChallenge as string | Buffer | base64url.

The way I see it expectedChallenge can share the same type as the newly-optional challenge, string | Buffer. It'll still be base64url-encoded within verifyAttestationResponse()/etc... since the challenge from the authenticator will be coming in base64url-encoded. Besides, it'd be unpleasant to try and discern whether a string value is already base64url-encoded since typeof string === typeof base64urlString

[mahnunchik]

typeof string === typeof base64urlString yep, it is to error prone...

See my previous comment https://github.com/MasterKale/SimpleWebAuthn/issues/39#issuecomment-665969097

[MasterKale]

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.

A single, simple, sane method of generating challenges is available to the library thanks to Node's crypto module, so I'm inclined to add it in.

Maybe instead of returning a raw Buffer by default I always return the base64url-encoded challenge, even when a custom challenge is provided. This way it always returns a value that's relatively easier to store across various storage solutions. That'd then mean updating the verify...() methods to no longer encode challenges internally, too... 🤔

[mahnunchik]

Base64url is not fixed length encoding so it is impossible to use CHAR(64) to store it.

I'd prefer to have helper method to generate challenge outside generate* methods

[MasterKale]

CHAR(128) would be more than enough to store the 86 bytes in a base64url-encoded 64-byte Buffer, just use that, storage is cheap 😛

[MasterKale]

@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.

[MasterKale]

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