Fix #4275 fail URIs with ambiguous segments

[gregw]

Fix #4275 fail URIs with ambiguous segments.

[gregw]

@sbordet @joakime nudge

[sbordet]

Since we are changing HttpURI parsing, I think we should go the extra mile and do it as per RFC3986.

https://tools.ietf.org/html/rfc3986#section-3.3 says that even the , can be used as separator between a segment and a segment-parameter.

But if we go to the ABNF, possibly any sub-delims can be present, and we should not consider those as part of the segment.
Doing so would probably require another compliance mode.

I changed testAmbiguousSegments() and using a , instead of ; fails the test.

[gregw]

@sbordet I don't see why we should treat ',' as a parameter separator? It might be used as such in some schemes, but not in HTTP. We will never split a segment by ',', but we do split by ';'. I think we need to be absolutely consistent with our handling to avoid issues, so if we start splitting on ',' here, we'd have to split everywhere and I don't think that is valid to do.

We are treating ..; as ambiguous because the RFC says we should only normalise .. and not ..;, yet we know we later split on ; so that only .. is left, which an application might normalise or might give back to us and we will normalise. I don't see the same problem with ..,.

[sbordet]

I don't see why we should treat ',' as a parameter separator?

Because the RFC says:

For example, the semicolon (";") and equals ("=") reserved characters are
often used to delimit parameters and parameter values applicable to
that segment. The comma (",") reserved character is often used for
similar purposes. For example, one URI producer might use a segment
such as "name;v=1.1" to indicate a reference to version 1.1 of
"name", whereas another might use a segment such as "name,1.1" to
indicate the same.

So if ; and , are interchangeable, and we only fix ;, then we'll have the same problem when someone uses ,, no?
And the same for all the other sub-delims characters.

The fact that it's not used typically in HTTP does not mean pentesters can't generate such URIs to validate URI handling.

[gregw]

, and ; are not interchangeable.
I known of no HTTP "URI producer" or consumer that will treat , in a segment as special.

We only have an issue with HTTP schema URIs as that is all we handle. We never split on ,, we do not remove ,param. I see no reason to not allow .., as a segment. Sure it is a silly looking segment, but then there are many other silly segments that we don't block.

Remember, we are not disallowing ..; because the RFC tells us to. We are disallowing it because the RFC tells us that it is a normal path segment and not a relative one, yet we know that we later strip the ; and thus can treat it as special. None of that holds for ,

[joakime]

There are multiple things to consider here.

First there's Reserved Characters - https://tools.ietf.org/html/rfc3986#section-2.2
Which defines the sub-delims as ...

      reserved    = gen-delims / sub-delims

      gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"

      sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

Which means , and ; are definitely equivalent.

The following example URIs should be considered in this PR.

/a/..;/b/
/a/..;foo/b/
/a/..;foo=bar/b/
/a/..,bar/b/
/a/..,foo=bar/b/
/a/..$/b/
/a/..!/b/
/a/..&/b/
/a/.."/b/
/a/..()/b/
/a/..*/b/
/a/..+/b/
/a/..=bar/b/

Then we have Remove Dot Segments - https://tools.ietf.org/html/rfc3986#section-5.2.4
Which is part of the resolution process for URIs.
Which pretty much tells us that . and .. are to be removed, even if they have parameters.

This is getting quite complicated, and I'm wondering if we REALLY need to support URI parameters at all?

I have been unable to find another web server that supports them.
Even Apache httpd doesn't support them.

Two examples:

# These work
$ curl https://en.wikipedia.org/wiki/Jetty
$ curl https://httpd.apache.org/docs/2.4/mod/mod_rewrite.html

# These do not work as expected
$ curl https://en.wikipedia.org/wiki;/Jetty
# above results in a redirect to the mainpage
$ curl https://httpd.apache.org/docs/2.4/mod;/mod_rewrite.html
# above results in a 404 status response

[joakime]

An interesting read on a complex / contrived URI example.

https://techdocs.broadcom.com/us/en/ca-enterprise-software/devops/devtest-solutions/10-4/using/using-ca-service-virtualization/ca-service-virtualization-concepts/parsing-uris-in-http-requests.html

[joakime]

This is getting quite complicated, and I'm wondering if we REALLY need to support URI parameters at all?

I retract this statement, I've found many examples in REST libraries that use various forms of the reserved characters in the URI to mean special things to the REST library.

Some even making the "contrived" example in the devtest link look easy to follow.

[gregw]

@sbordet and @joakime, you are over complicating this and missing the actual issue!

Our URI handling is correct according to both the RFC and the servlet spec. However, there is a problem between those two spec. The RFC says that ; might be a special character, yet does not say it should be stripped at the URI level. However the servlet spec expects that path parameters will be stripped. Our code is aware of this and we treat mapping of both security constraints and servlets in exactly the same way, so there can be no bypasses. However, because we strip parameters from the URI before giving them to the servlet, then we throw away information and the application can get a path with a .. segment. It should know that this is not really a .. segment and that it came either from %2e%2e or ..;param. However, it is pretty easy for the application to forget this an normalise it, or not re-encode it and give it back to us, so we normalise it.

This is ONLY a problem with ; and % encoding, because they are the only two ways that we can ever decode a URI and give the application a .. segment. The solution I have coded here is to just not support %2e%2e and ..;param (plus a few variations of those). If there are some other URIs that we also decode to result in .. segments, then we need to handle those as well, but none of the examples either of you have listed appear to be such examples.

[gregw]

Note also that the code doesn't actually treat ; that specially. It has modified the URI parser so that any character than ends a segment (;, ?, '#, etc.) will result in the segment being check as potentially ambiguous.

However, I will double check how we handle things like /foo/..?bar ...

[gregw]

OK, I think I have found one more related issue. The RFC says the URI is:

URI         = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

We correctly normalise /foo/bar/..?query to /foo#query, but we don't do /foo/bar/..#fragment to /foo#fragment. Thus when we strip the fragment later, we may get a similar problem.
So I think our URIUtil canonical path methods should probably also handle # just like ?, even though that should never come over the wire.

[joakime]

I like the majority of this PR, I just don't like one aspect of the ambiguousSegments list.

[gregw]

@sbordet nudge

[sbordet]

I would not do null checks for just assignments.
Path needs null check because path.length() may NPE.

[gregw]

I'm doing null checks because if we parse the path, we may discover params, fragments and queries in it. So either we through if they are non null, or just not overwrite them unless we have explicit non null values.