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

timingSafeEqual functionality #270

Open
jespertheend opened this issue Aug 13, 2021 · 22 comments
Open

timingSafeEqual functionality #270

jespertheend opened this issue Aug 13, 2021 · 22 comments

Comments

@jespertheend
Copy link

Nodejs has the built in timingSafeEqual(a, b) function.
Are there any plans for getting this in SubtleCrypto as well?

@panva
Copy link
Member

panva commented Aug 13, 2021

This would be a very welcome addition.

@jespertheend
Copy link
Author

I'd like to add this to the spec, but I haven't done any spec work this big before. Do you think this would be a good first bug?

@twiss
Copy link
Member

twiss commented Aug 19, 2021

Hey 👋 Thanks for the proposal. While I think this addition could make sense for SubtleCrypto, it would seem to be out of scope for the new charter that the WebAppSec WG is planning to adopt, which is more focused on maintaining the API (making sure it stays secure, adding more modern algorithms, etc) rather than adding new features.

In addition, there's some overlap here with Constant-Time WebAssembly, and there's some question whether low-level crypto operations (which rely on constant-time comparisons) may make more sense to implement in Web Assembly. For example, note that verifying an HMAC is already supported as a higher-level operation in Web Crypto. Do you have any other concrete use cases of a constant-time comparison function that aren't covered by Web Crypto already but that you believe would make more sense to implement in JavaScript than WebAssembly?

Regardless of the above, before we specify this, there needs to be interest from implementors first. We could of course try to find out if there's interest, but with Constant-Time WebAssembly (hopefully) around the corner, IMHO this seems lower priority than the maintenance work that needs to happen, so personally I would wait for both of those to happen, and then re-evaluate whether this is needed.

@jespertheend
Copy link
Author

Thanks for the info, sounds good 👍

I needed the functionality server side in Nodejs. I ended up using the non standard timingSafeEqual(a, b) but I was a bit surprised there was no equivalent in the subtle interface.
I'm not sure if there's any use case outside of Nodejs, perhaps it could be useful in Deno.

My specific use case is that I generate a random password and send it to the client while storing a digest in my database.
Then to authenticate the user sends the generated password and I compare its digest with the stored one using timingSafeEqual.
But perhaps there is a better way to do this that I'm not aware of?

@twiss
Copy link
Member

twiss commented Aug 19, 2021

No, makes sense. Technically speaking, comparing hashes is probably less timing-sensitive than certain other operations, because with a strong key derivation function (such as Argon2 or scrypt), learning anything about the hash should not reveal anything about the password, unless the password is very weak. But nevertheless it's probably good practice for that case, yeah 👍

If both Node.js and Deno are interested in implementing this in Web Crypto, then it may be worth checking with the browsers whether they are potentially also interested in implementing it, to see if it would be worth standardizing.

@tniessen
Copy link
Contributor

My specific use case is that I generate a random password and send it to the client while storing a digest in my database.
Then to authenticate the user sends the generated password and I compare its digest with the stored one using timingSafeEqual.
But perhaps there is a better way to do this that I'm not aware of?

This sounds fine, assuming you also use a random salt for each stored digest :)

If both Node.js and Deno are interested in implementing this in Web Crypto

In general, I do believe that timing-safe comparisons are an essential component to any system that uses low-level cryptographic features and I am not generally opposed to adding it to WebCrypto.

That being said, JavaScript is not good for using low-level cryptographic features without introducing a ton of side channels. For example, it turns out to be difficult just to test timingSafeEqual within Node.js core, most likely due to optimizations that V8 applies to the surrounding code.

WebAssembly, for example, allows much better control over side channels than JavaScript and still cannot provide timing safety, which is the primary reason behind the constant-time WebAssembly proposal.

The main use case of timingSafeEqual in Node.js is to validate untrusted inputs against known secrets, which is exactly what @jespertheend described, and which is much more common on the server than in web browsers.

@jasnell
Copy link

jasnell commented Jul 14, 2022

Just revisiting this. For Cloudflare Workers, we are looking at the possibility of implementing crypto.subtle.timingSafeEqual(a, b) as an extension to SubtleCrypto with the same semantics as Node.js' implementation. We want to make sure that doing so is not going to conflict with any standards work here. I'm happy to write up spec text for this.

/cc @twiss @tniessen

@twiss
Copy link
Member

twiss commented Jul 15, 2022

Just revisiting this. For Cloudflare Workers, we are looking at the possibility of implementing crypto.subtle.timingSafeEqual(a, b) as an extension to SubtleCrypto with the same semantics as Node.js' implementation.

Alrighty 👍 Looking at Node.js's docs, just one comment I'd have regarding

When both of the inputs are Float32Arrays or Float64Arrays, this function might return unexpected results due to IEEE 754 encoding of floating-point numbers. In particular, neither x === y nor Object.is(x, y) implies that the byte representations of two floating-point numbers x and y are equal.

I would perhaps suggest to instead restrict the input types, and do the same check that getRandomValues does, i.e.

If array is not an Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, BigInt64Array, or BigUint64Array, then throw a TypeMismatchError and terminate the algorithm.

(we could come up with a name for this set, if it makes things simpler, e.g. "integer array views").

We want to make sure that doing so is not going to conflict with any standards work here. I'm happy to write up spec text for this.

Hmhm. Other than the above, I don't see any issues or risks for conflict, but having some spec text for this would probably be valuable 👍 If there's interest from other implementers, submitting it to the WICG might also make sense.

@armfazh
Copy link

armfazh commented Feb 14, 2023

Regardless of the above, before we specify this, there needs to be interest from implementors first.

+1 to include this functionality.

In our use case, we use this auxiliary function in Typescript.

https://github.com/cloudflare/opaque-ts/blob/d6d5c55470bcad5e8b763db6040b09ccf6dcd520/src/util.ts#L111-L121

However, there are no guarantees that operations are performed by the integer ALU rather than the floating point unit (FPU).

@larsqa
Copy link

larsqa commented Apr 26, 2023

+1

With the rise of Edge runtime, and Next.js middleware using said runtime, this is a required security API for handling, for example, static access tokens in endpoints.

@larsqa
Copy link

larsqa commented Apr 26, 2023

I've been looking deeper into this, and for the record, I'm a cryptography novice.

@armfazh
https://github.com/cloudflare/opaque-ts/blob/d6d5c55470bcad5e8b763db6040b09ccf6dcd520/src/util.ts#L111-L121

However, there are no guarantees that operations are performed by the integer ALU rather than the floating point unit (FPU).

This seems to be a bigger issue in Node.js than expected, which is also highlighted in this JS library and made Deno use an approach with DataView instead of Uint8Array (but can't understand the specific reason behind it).

A similar concern has been raised with the Node.js team, and the "Double HMAC verification" pattern has been recommended. However, it turned out that this issue was a bug in CRYPTO_memcmp instead with the implementation, so no HMAC comparison is used in Node.js.

@twiss
Copy link
Member

twiss commented Apr 26, 2023

If I understood your first link correctly, the issue is that in JavaScript, accessing a number might not be constant-time on 32-bit V8 (and possibly other JS engines), since it first has to check the first bit to see whether it's actually a number or a pointer. (I was actually under the impression that 64-bit JS engines do something similar, but I might be wrong.)

But, I think for a native API like Web Crypto, this shouldn't be an issue, because the function should be able to read the bytes in the typed array and compare them directly, without needing to check whether they're pointers, since a typed array can only contain numbers. Even when passing in a Uint32Array for example, it can still access the individual bytes, if it prefers. Even if it wants to take the HMAC before comparing the bytes, that should also be possible when accepting typed arrays.

In general, implementations of such a function in JS can't really be guaranteed to be constant-time, I think; so the implementation should always be done natively.

@larsqa
Copy link

larsqa commented Apr 27, 2023

@twiss, thanks for the elaboration!

JS can't really be guaranteed to be constant-time

I see. Would the "Double HMAC verification" pattern then be a constant-time alternative, since it doesn't rely on a bitwise comparison pattern?
It's also recommended in various posts:

I'd imagine that the HMAC comparison itself leaking length doesn't matter because the prefix of the double HMAC will constantly change. You cannot rely on the assumption that you've correctly guessed the first byte between hmac(key, leftValue) and hmac(key, rightValue) because from the next byte on, the whole HMAC will change, effectively turning the attack into a brute-force instead of a time-based.


A shower thought - would a WebAssembly solution guarantee constant-time?

@twiss
Copy link
Member

twiss commented Apr 27, 2023

I see. Would the "Double HMAC verification" pattern then be a constant-time alternative, since it doesn't rely on a bitwise comparison pattern?

The pedantic answer would be no, there could still be timing differences, when you read the values to be HMAC'd, for example (if you implement the HMAC'ing in JS). That would probably be very difficult to exploit though, so it's probably "safe enough" (especially if you use Web Crypto for the HMAC), but it still seems safer to implement it entirely natively, IMHO.


A shower thought - would a WebAssembly solution guarantee constant-time?

WebAssembly is not guaranteed to be constant-time, no, though there is a proposal for constant-time WebAssembly instructions.

@larsqa
Copy link

larsqa commented Apr 27, 2023

@twiss there could still be timing differences, when you read the values to be HMAC'd, for example (if you implement the HMAC'ing in JS)
I appreciate a lot your thoughts on my questions/concerns!

I'm curious, how would the following leak time (pseudo-code)?

const key = "abc" // some strong key
function compare(a, b){
    return hmac(key, a) === hmac(key, b)
}

compare("abc", "abc") // true

From my understanding as i written before, lets imagine:

  • hmac(key, "abc") returns hsjq28s...
  • hmac(key, "a") returns h81ka...
    Even if both have the same first value in the hash, it wouldn't matter because after the second value hmac(key, "ab"), a totally different hash is generated kwu19....

WebAssembly is not guaranteed to be constant-time, no, though there is a proposal for constant-time WebAssembly instructions.

Interesting, thanks for sharing!

@twiss
Copy link
Member

twiss commented Apr 28, 2023

Well - depending on the JS engine, reading the values a and b from memory could be non-constant-time. Again, whether that's exploitable is doubtful, but it's probably not ideal if that's the case.

@larsqa
Copy link

larsqa commented Apr 28, 2023

reading the values a and b from memory could be non-constant-time. Again, whether that's exploitable is doubtful,

@twiss, I think I'm following now. I might not be able to wrap my head entirely around the situation, but I believe that even if there is a time-leak with reading a and b from memory, the variating time it takes for the "hmac double verification" to finish cancels this issue out, making it nearly impossible.

Don't understand me wrong, I'm all for a native implementation! I'm just looking for an alternative that can be safely used in Browser/Edge environments until this spec has been released and implemented in Edge/Major Browsers.

@larsqa
Copy link

larsqa commented Apr 29, 2023

@twiss / @armfazh - I ended up writing a time safe comparison npm library that uses the "Double hmac verification" pattern with WebCrypto's HMAC, that could be used meanwhile. It's not perfect, but I'd imagine it to be a safer alternative to the existing libraries that rely on the bitwise XOR comparison. It runs "only" 60 times slower than Node.js' crypto.timingSafeEqual if you provide your own hmac key.

Repo: https://github.com/advename/web-timing-safe-equal

@jimmywarting
Copy link

I'm thinking: why dose it have to be on the crypto at all and why should it only be able to deal with typed arrays?
couldn't it be more beneficial to have something like Array.isEqual() that operates the way like safe-time-equal operates that can also check weather or not two regular array are equal?

@twiss
Copy link
Member

twiss commented Jan 18, 2024

@jimmywarting The way timingSafeEqual is implemented typically relies on it being passed byte arrays. For example, see CRYPTO_memcmp in OpenSSL, which Node.js's timingSafeEqual uses.
Having non-uniform types in the array might also introduce timing differences when reading the values, so it's not even clear (at least to me) that a general Array.timingSafeEqual could be implemented at all.
And finally, the typical use case for this is around comparing hashed passwords and things like that, so I think crypto.subtle.timingSafeEqual makes sense as a place to have it, if anywhere.

@tniessen
Copy link
Contributor

@jimmywarting How would that work? The timingSafeEqual function in Node.js doesn't even work for Float32Array etc. because equality for 32-bit IEEE-754 numbers is difficult to determine using memory equality. How would this work for arrays of objects?

@twiss
Copy link
Member

twiss commented Jan 18, 2024

@larsqa Your comments made me realize that the current API kind of already does provide a way to do a timing-safe comparison, as follows:

async function timingSafeEqual(bufferSource1, bufferSource2) {
    const algorithm = { name: 'HMAC', hash: 'SHA-256' };
    const key = await crypto.subtle.generateKey(algorithm, false, ['sign', 'verify']);
    const hmac = await crypto.subtle.sign(algorithm, key, bufferSource1);
    const equal = await crypto.subtle.verify(algorithm, key, hmac, bufferSource2);
    return equal;
}

Even though the spec doesn't say that the comparison of the HMACs needs to be done in constant-time, all implementations I checked (Chromium, Webkit and Gecko) do so; and of course the HMAC adds some safety even if the comparison is not done in constant-time.

Perhaps we should additionally specify that this comparison must be done in constant-time, as a first step - that might also reduce the need for this method? (Although it might still be convenient, and would of course be faster if it doesn't have to do two HMACs.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants