Owasp-modsecurity-crs: 921100 incorrectly forbids "," (commas) in Transfer-Encoding

[fgsch]

If I'm reading this rule correctly, it will trigger for both multiple Transfer-Encoding headers and a single header with multiple values separated by comma.
The latter is perfectly valid, e.g. https://tools.ietf.org/html/rfc7230#page-29 and https://tools.ietf.org/html/rfc7230#page-82.

[fgsch]

Should this use the &REQUEST_HEADERS:/.../ "@gt 1" ... constructor instead?

[fgsch]

I'd like to open a PR for this.
Anyone has any opinions re using &... to count the number of headers?

[dune73]

Would you want to disallow more than one header?

I think we might be able to whitelist values for transfer-encoding, but the RFC speaks of Transfer-Extensions that seem to complicate things a lot.

Maybe we limit 921100 to Content-Length and then try to cope with Transfer-Encoding in a whitelisting manner at a higher PL?

[fgsch]

Let's take a step back. The issue here is that apache will join multiple T-E headers by merging them in a single header separated by "," (comma), which makes it impossible to distinguish from a single header with multiple values.

Is my understanding correct?

Please note that neither of the 2 links in the rule mention Transfer-Encoding and I can't find any reference saying that multiple T-E headers is invalid.

[dune73]

Yes, apache will join them with a comma as separator, while we check for the comma as a sign of request smuggling.

Regardless of where the comma comes from, it is considered OK within the T-E header. So I think we are doing this wrong.

[fgsch]

Personally I think we should:

1. Remove T-E from this rule
2. Add another rule that checks that both C-L and T-E are not present
3. Consider what to do with T-E in its own rule

What do you think?

[dune73]

I agree with you completely.

[fgsch]

Ok, I will open a PR with this and try to get more info regarding multiple T-E headers in the meantime.

[dune73]

What would really be helpful is a "value zoo" for HTTP headers. That is a collection of all sorts of HTTP headers as can be found in the wild. Finding information about exotic user-agents and browsers in special situations can be really tough.

[fgsch]

We could start with the basic ones (https://tools.ietf.org/html/rfc7230#section-4) and add any extensions as we found along. Thoughts?

[dune73]

Depends a bit on the PL. I'm OK with this plan for PL3. Less so for PL1 and 2. And I wonder how frequent the use of these transfer-extensions is. I have never seen this myself.
If we go with https://tools.ietf.org/html/rfc7230#section-4, would we whitelist? Whitelisting via parallel match is likely to be fastest and still readable if we ignore transfer-extensions.

[fgsch]

Yes, that's exactly what I meant. Sorry for being terse.
Basic ones in PL1, we consider others eventually, most likely at different PLs.

[dune73]

But does not that mean that we disallow transfer-extensions at PL1?
And that is exactly what I want to avoid.

I am OK with disallowing them at PL3 because they seem to be rare, but we should not fire into the dark at PL1.

[fgsch]

You are right, sorry. In that case, shouldn't this be settings like allowed_methods, etc. where you specify which ones are valid and by default we only include the basics?

I think extensions are rare enough that we can avoid them at PL1 but should be easy enough to add them if you really want to.

[dune73]

I'm skeptical. Can't we disallow them at PL3 and if we go for an extended period of time without any alerts, we shift the rule down to PL2 or PL1?

Using a variable for configuration would actually be nice, I think.

But whatever we do: more people with an opinion are needed.

[fgsch]

Do we agree in 1 and 2? I can work on that while we decide how to handle 3.

[dune73]

Yes, I think we agree on 1 and 2.

[csanders-git]

it's hard to put something so complicated that not many people are familiar with into a core release at a low PL level off the bat without review. I'd say that it makes sense based on our conversations that we shoot for basic abuse stuff for 3.2 in a reasonable PL level and then extensions and more complicate encoding in 3.3.

[spartantri]

for 3. it maybe good to do some basic known abuse checks for T-E at PL1 e.g. chunked but leave all extensions to PL 4 as it will require to do a chain of validations and we have not much supporting test data on this

[fgsch]

I'm going to open a PR to remove this rule together, because:

1. Content-Length is already handled in 920160
2. A comma in Transfer-Encoding is valid

From the convo it does not feel we have a agreement on how to handle T-E and the current rule is incorrect but if anyone has any objections please let me know.

[dune73]

That sounds reasonable, however, we are already in the feature freeze for 3.1.

[fgsch]

I consider this a bug though but it's your call.

[dune73]

That's true. But bugs and features are not so clear with CRS, I think.

[fgsch]

This has been fixed.

Whether we want to sanitize the T-E codings (potentially including the extensions) should be addressed in a separate issue/PR.