app.set('etag','weak') can collide fairly easily with different bodies <= 1KB

[stuartpb]

As initially conjectured in #2129 (comment), Express's default etag behavior is very broken.

As stated by section 2.1 of RFC 7232:

A "strong validator" is representation metadata that changes value
whenever a change occurs to the representation data that would be
observable in the payload body of a 200 (OK) response to GET.

[...]

A collision-resistant hash function applied to
the representation data is also sufficient if the data is available
prior to the response header fields being sent and the digest does
not need to be recalculated every time a validation request is
received.

[...]

In contrast, a "weak validator" is representation metadata that might
not change for every change to the representation data. This
weakness might be due to limitations in how the value is calculated,
such as clock resolution, an inability to ensure uniqueness for all
possible representations of the resource, or a desire of the resource
owner to group representations by some self-determined set of
equivalency rather than unique sequences of data. An origin server
SHOULD change a weak entity-tag whenever it considers prior
representations to be unacceptable as a substitute for the current
representation. In other words, a weak entity-tag ought to change
whenever the origin server wants caches to invalidate old responses.

For example, the representation of a weather report that changes in
content every second, based on dynamic measurements, might be grouped
into sets of equivalent representations (from the origin server's
perspective) with the same weak validator in order to allow cached
representations to be valid for a reasonable period of time (perhaps
adjusted dynamically based on server load or weather quality).
Likewise, a representation's modification time, if defined with only
one-second resolution, might be a weak validator if it is possible
for the representation to be modified twice during a single second
and retrieved between those modifications.

Likewise, a validator is weak if it is shared by two or more
representations of a given resource at the same time, unless those
representations have identical representation data. For example, if
the origin server sends the same validator for a representation with
a gzip content coding applied as it does for a representation with no
content coding, then that validator is weak.

TL;DR: A "weak etag" is an etag that is based on metadata. The "weak" in "weak etag" has nothing to do with collision-resistance weakness: all etags are interpreted to be semantically collision-resistant. Yet the behavior of Express when etag is set to weak (the default) is to use a hash-like function not resistant to collisions, prefixed with 'W/' (the signifier that denotes that the response would be semantically equivalent, if not byte equivalent, to other responses with that etag).

On top of being semantically wrong, this could be exploited by an attacker to set a value that would permute a page's etag to remain the same, denying requests for it from receiving the new version of the page whenever its content would be changed.

[stuartpb]

A correct implementation of weak etags would be to do something like hashing the JSON representation of the locals provided to a template (and the hash of the template itself) and compare those hashes when deciding whether to render the template or respond with a 304.

Needless to say, since templates can have non-deterministic components, this would be something a user would opt into with middleware.

[dougwilson]

A PR to fix this would be welcome :) If you can provide a demonstrated attack (you can always send it to me via email if it's sensitive), we can look at putting in resources to change it. Otherwise, we would welcome the community to contribute patches to express and the etag module that would better implement weak ETags without bringing down performance.

[stuartpb]

Since they're predicated on metadata, you can't really implement weak etags without application knowledge. It's a job best left to middleware.

[dougwilson]

If you can produce something that demonstrates a real-world problem, we would be much more willing to do something. A way to demonstrate is if you can show two routes that provide the same weak ETag and are both workable (i.e. if one is JSON, the the other should be valid JSON as well).

[dougwilson]

We are not technically violating anything, besides the framework-level weak ETag being odd-looking, because the spec only says

An origin server SHOULD change a weak entity-tag whenever it considers prior representations to be unacceptable as a substitute for the current representation

If that were a MUST then we would be in violation, but it's just a SHOULD, so it's not an absolute requirement.

[dougwilson]

But regardless, if you can show two routes that produce different bodies that send the same weak ETag, I would be much more convinced to change something (and hopefully keep the same speed we have now).

[stuartpb]

I really don't want to spend a protracted amount of time implementing this, but http://stackoverflow.com/a/1519711/34799 explains why it would be really simple to implement a preimage attack, especially in instances where the last bytes can be controlled (in which the complexity of an attack basically becomes O(1)).

[stuartpb]

And https://github.com/jlnt/tweakcrc appears to be an existing piece of code built specifically to do so.

[dougwilson]

, but http://stackoverflow.com/a/1519711/34799 explains why it would be really simple to implement a preimage attack, especially in instances where the last bytes can be controlled (in which the complexity of an attack basically becomes O(1)).

Yes, but that only applies to CRC32; what we send, CRC32 is not part of the equation; we include data like the length in there as well. Appending 4 bytes would alter the ETag we generate, even if the CRC32 is the same.

[stuartpb]

Yeah, but changing 4 bytes wouldn't, and it's equally plausible that a user would be able to control the last bytes while retaining the same size. (And, again, the bytes can be anywhere and it only somewhat increases the difficulty.)

Worth linking to https://github.com/jshttp/etag/blob/master/index.js here.

[dougwilson]

Gotcha. I'll have to take a look into this when I have time. I would definitely be willing to keep an issue open over in the etag module's repository about the potential for security issues and waiting for myself (or someone) to show demonstrable proof that etag is vulnerable.

[stuartpb]

Example after 5 minutes of playing with tweakcrc: {"username":"dougwilson","pwned":"false"} and {"username":"d��j�ilson","pwned":"ohyea"} both have a byte length of 42 and a CRC32 of 0x47EC7B58.

stuartpb@tweakcrc:~/workspace (master) $ xxd example-tweak-pwned.json                                                                                                                                           
0000000: 7b22 7573 6572 6e61 6d65 223a 2264 e7d3  {"username":"d..
0000010: 6a90 696c 736f 6e22 2c22 7077 6e65 6422  j.ilson","pwned"
0000020: 3a22 6f68 7965 6122 7d0a                 :"ohyea"}.

Yes, this involves bytes that are probably not valid UTF-8 (JSON.parse accepts the copy-pasted text happily in my browser). tweakcrc isn't exactly sophisticated (eg. it only accepts offsets that are multiples of 4 from the end). This would still work to break a naive route and/or HTML template (and there's probably another position that could be done with only valid ASCII characters, especially with a larger attack space).

[dougwilson]

This would still work to break a naive route and/or HTML template

Right. This is enough to convince me. Can you open an issue in the etag module (and perhaps include a link of this issue) and I want to get something addressed soon-ish.