Worker Auth with headers example should not use == to check key equality

[benlongo]

Which Cloudflare product does this pertain to?

Workers

Existing documentation URL(s)

https://developers.cloudflare.com/workers/examples/auth-with-headers/

Section that requires update

The comparison of the pre-shared keys.

What needs to change?

I believe the use of == in theory enables timing attacks as the string comparison will terminate on the first differing character.

How should it change?

I'm unsure. Node has timingSafeEqual but it looks like that hasn't landed in Web Crypto yet https://github.com/w3c/webcrypto/issues/270.

Perhaps this implementation would work in the meantime https://github.com/Bruce17/safe-compare/blob/master/index.js#L22.

Additional information

No response

[KianNH]

Is the issue specifically with == or === as well? The example you linked uses ===

[benlongo]

I don't think it matters. Both will use the efficient implementation and stop at the first differing character.

[kentonv]

The timing issue is probably not exploitable in practice due to the sheer amount of noise present, but it is theoretically a problem and we should do something about it.

Pointing people to a third-party package probably introduces a bigger security issue in practice (supply chain attacks). Ideally we should implement timingSafeEqual in the Workers Runtime, matching Node's behavior.

Meanwhile, though, IMO the example would be better if PRESHARED_AUTH_HEADER_VALUE contained a hash of the value, instead of the raw value. The code should then hash the actual header, and compare the hashes. This would add meaningful security to the example (leaking the code no longer reveals the key), and it also makes the comparison timing irrelevant (since the attacker cannot practically control prefixes of the hash in order to test one character at a time).

[benlongo]

Agreed that a hash solves the issue without introducing additional vectors (like the supply chain attack). The performance hit from hashing is almost certainly negligible.

[jasnell]

Documentation PR for crypto.subtle.timingSafeEqual(a,b) => https://github.com/cloudflare/cloudflare-docs/pull/5856

[KianNH]

Since the input buffers for timingSafeEqual need to be the same length, my first thought is that you need a psk.length === PRESHARED_AUTH_HEADER_VALUE.length check (or a try/catch to handle the TypeError: Input buffers must have the same byte length.)

Is the .length check similarly vulnerable to a timing attack?

[benlongo]

I'm not 100% sure, but I believe a .length check would be constant time. You could theoretically infer the length of the key if it steps out early.

[kentonv]

.length is a simple integer so comparing it is constant-time. However, indeed, if the rest of the comparison is skipped when the length is matched, then an attacker may be able to determine the correct length by checking for timing differences at different lengths. However, just knowing the length probably isn't a big deal in most cases. They still wouldn't be able to use timing to determine the content.

[jasnell]

For timingSafeEqual, both lengths are checked before any content comparison is made. The actual comparison uses CRYPTO_memcmp, which per the docs, "takes an amount of time dependent on len, but independent of the contents of the memory regions pointed to by a and b."

The comparison itself will not leak timing information but code around it can still.