caddyhttp: Reject conflicting values in query strings

[mholt]

This is a proposed change, I am not sure if this is the right thing to do yet.

We got a report about how the current behavior of our query matcher can lead to unexpected results in backend applications that rely on it to match query strings one way that is different from how they interpret query strings. Namely, if a key is repeated with different values in either the URI or the matcher, the result can be surprising.

Strictly speaking this is not a security vulnerability in Caddy, but can lead to bugs if not carefully tested.

The current query matcher allows ?a=1&a=2 to match "a": ["1"] because, technically, the URI does indeed set a=1. But it also sets a=2. The query matcher is not wrong, but it could be surprising if the backend reads a as 2 instead.

This change does not match query strings that have conflicting values for a key. It's a little stricter. However, this may not be the desired behavior, as there are valid use cases for matching the above scenario. It could be that the user wants to match a request where a given key has at least this value, even if it has other values.

Of course, if your matcher has "a": ["1", "2"] and the query string contains ?a=1&a=2, should it match? Well, if the matcher semantics are to "AND" the values, then yes; if they are to "OR" the values, then yes, but if they are to "XOR" the values, then no; in other words, if the key a may be only either 1 or 2, then this must not match because the two are in conflict.

There are countless ways to match query strings. I am of the opinion that a general-purpose web server should be permissive here and allow many possibilities, since it does not of itself present any security concerns. Any security considerations should be taken by the application actually relying on the query strings.

Ideally the query matcher should be as strict as possible. It could be possible that with multiple query matchers, more specific boolean logic can be achieved with stricter query matching. And if so, I'd like to do that. Needs more exploring.

Ultimately, if a backend/application relies on the query string, it needs to do its own parsing (but this is not unique to Caddy or any web server, really). As Filippo said, "Relying on parser alignment for security is doomed."

[mholt]

I won't merge this as-is, but I may consider a different approach that implements strict checking with an opt-in sort of syntax/flag, for example prefixing or suffixing the key with ! to indicate strict mode.

In the meantime, I believe the CEL matcher can express more complex query string comparisons.

I might close this and just do a new PR, we'll see. The QS matcher is not very commonly used, thankfully.