Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path Normalization/Traversal - Context Matching #4275

Closed
venukb opened this issue Nov 6, 2019 · 22 comments · Fixed by #5954, #6003, mspnp/azure-databricks-streaming-analytics#13, mourjo/lestrange#3 or salesforce/centrifuge#2
Assignees

Comments

@venukb
Copy link

venukb commented Nov 6, 2019

We have an app with 2 servlet path defined

/ws/v1
/foo

/ws/v1 was exposed on edge (ATS). However /foo accessed on Jetty using this

/ws/v1/..;/..;/foo - so we had a path exposed on the edge which was not intended to be exposed

  • Is this matching done as per servlet spec?
  • Is there a configuration to avoid this in Jetty
  • I've heard that Undertow for e.g. does not allow the above.

(Jetty version: 9.4.22 - same behavior seen in older versions as well)

@joakime
Copy link
Contributor

joakime commented Nov 6, 2019

The use of ; in path segments is a URI with path parameters.

https://tools.ietf.org/html/rfc3986#section-3.3

As for how path parameters are to be treated.
That's an open issue on the servlet spec - jakartaee/servlet#18

There's been discussion about how to handle normalization when the URI is complicated with path parameters, URI templates variables, etc.
Right now, that's an undefined part of the servlet spec.

@joakime
Copy link
Contributor

joakime commented Nov 6, 2019

Java URI.normalize() also doesn't handle path parameters during requests for normalization.

package uri;

import java.net.URI;

public class URITest
{
    public static void main(String[] args)
    {
        dump(URI.create("http://machine.com/ws/v1/..;/..;/foo"));
        dump(URI.create("http://machine.com/ws/v1/../../foo"));
    }

    public static void dump(URI uri)
    {
        System.out.println("URI = " + uri);
        System.out.println("   .normalize = " + uri.normalize());
    }
}

results in ...

URI = http://machine.com/ws/v1/..;/..;/foo
   .normalize = http://machine.com/ws/v1/..;/..;/foo
URI = http://machine.com/ws/v1/../../foo
   .normalize = http://machine.com/foo

@venukb
Copy link
Author

venukb commented Nov 6, 2019

Yup agree ; is valid.
But /ws/v1/..;/..;/foo matches a servlet with url pattern /foo -

Seems like this is being misused. Was checking to see if there were any controls/options in Jetty - to not match paths like this

i.e. in org.eclipse.jetty.server.Request

I see _originalURI is /ws/v1/..;/../foo in this and _servletPath is /foo.
Is this match also as per Servlet Spec?

@gregw
Copy link
Contributor

gregw commented Nov 6, 2019

@venukb It is correct for jetty to match that normalised path to a servlet. But then the protection of /foo must also use normalised paths.
Can you tell us how you are protecting the /foo path? is it with a security constraint in web.xml or some other mechanism?

We should be applying security constraints to normalised URIs so /..; should not bypass that.

@gregw
Copy link
Contributor

gregw commented Nov 6, 2019

Also, as this potentially has security ramifications can you send your response via email to [email protected]

@gregw
Copy link
Contributor

gregw commented Nov 6, 2019

Note that I have retested Jetty security constraint handling and I cannot reproduce a problem.
I cannot bypass security constraints with URIs like /foo/bar/..;/..;/secret

Please email us more details of your issue - I expect that you are using some other form of protection that is not working on normalised paths.... but don't say what it is publicly.

@venukb
Copy link
Author

venukb commented Nov 7, 2019

Thanks. I'll share details on email.

Was told that Undertow does not allow this i.e. it rejects request rather than matching /foo.
Not keen to switch to undertow just for this and was seeing if Jetty was allowing this to conform to a specification

So was checking if there was a config to disallow this kind of request pattern.

@gregw
Copy link
Contributor

gregw commented Nov 7, 2019

There is no configuration to disable, as it is a perfectly legal URI. However, it would be trivial to write a filter and/or customizer that rejected any requests that have .. in their raw request URI... or if you wanted to be really string if the URI does not end with exactly the decoded contextPath+servletPath+pathInfo.

@gregw
Copy link
Contributor

gregw commented Jul 20, 2020

Out of an abundance of caution I have reviewed this issue again to see if we really should be normalising /..;/ out. Re-reading https://tools.ietf.org/html/rfc3986#section-3.3 the answer is once again NO because the RFC only defines the behaviour of path segments ".." and ".", so a path segment of "..;" is not ".." and should not walk the hierarchy. So we are technically correct.....

HOWEVER! It is obvious that there is still a risk if path parameters are later stripped from the URI, which is then passed to a component that does interpret "..". Moreover, I really don't see any valid use-case for /..;/ other than trying to hack around web security.

Thus I'm now thinking that we should just treat such URIs as illegal. Not even follow the .., but just return a 400 response treating these as bad URIs.

@joakime @sbordet @lorban see any reason why not?

@gregw gregw reopened this Jul 20, 2020
@gregw
Copy link
Contributor

gregw commented Jul 20, 2020

Hmmm, but then should we also reject paths with /%2e%2e/?

@joakime
Copy link
Contributor

joakime commented Jul 20, 2020

We should review any delimiter then, not just limited to the one specific to path parameters.

https://tools.ietf.org/html/rfc3986#section-2.2

I remembered this from a prior issue #2067

@gregw
Copy link
Contributor

gregw commented Jul 20, 2020

Actually, I think we are good here. We feed the request handling process with:
URIUtil.canonicalPath(uri.getDecodedPath()), thus we strip parameters and decode first, then we do canonical path handling. So /%2e%2e/ and /..;/ will both be handled prior to any security checks, so security and request handling will work on the same normalised URI and there is no security risk here.

I'm closing again.

@gregw gregw closed this as completed Jul 20, 2020
@oglueck
Copy link
Contributor

oglueck commented Feb 8, 2021

Well, it's not just about security restriction in Jetty. It's also a publishing issue. You publish /ws/v1 on a reverse proxy to the Internet, but /foo only to internal networks. Both without security configuration in Jetty, but solely access restrictions on reverse proxy. Now any Internet use can abuse this path traversal to access /foo.
Also if ; is used as a parameter separator, why is /ws/v1/..;/..;/foo normalized into /foo then? That URI is
/ws/v1/.. and its parmeters are /.. and /foo . So the normalized path is /ws and not /foo.
If ; is not treated as parameter separate then ..; is a path component and not a traversal, so still below /ws/v1.
For me the normalization into /foo is abstruse.

@gregw
Copy link
Contributor

gregw commented Feb 8, 2021

On review, I agree that the normalisation is strange. It is a bad combination of two reasonable steps: it is according to the spec to not consider "..;" the same as a ".." segment; It is a reasonable decoding of a "..;" segment to result in "..".

Internally we don't have a security issue, because we apply the same process to matching and security constraints, but I agree that in a wider system it is problematic.

I'm currently thinking that we should just throw an ISE when parsing any URI that includes a segment with "..;" or ".;<param" in it (where param can be empty). I do not see any valid uses for such segments.

@gregw gregw reopened this Feb 8, 2021
@sbordet
Copy link
Contributor

sbordet commented Feb 8, 2021

Also if ; is used as a parameter separator, why is /ws/v1/..;/..;/foo normalized into /foo then? That URI is
/ws/v1/.. and its parmeters are /.. and /foo .

I don't think that is correct since / is a reserved character, so I interpret /ws/v1/..;/..;/foo as /ws/v1/.., then an empty parameter for segment /.., then another segment /.., again with an empty parameter, then /foo.
So excluding segment parameters, the URI is /ws/v1/../../foo, which normalizes to /foo.

To have /ws/v1/.. with 2 segment parameters like you interpret it, it should be /ws/v1/..;%2F..;%2Ffoo.

@gregw I'm more on the lines of just dropping middle-segments parameters (the only one to retain is a final segment parameter named jsessionid), but retain the segments, so the example above should be normalized to /foo.

@gregw
Copy link
Contributor

gregw commented Feb 8, 2021

@sbordet The problem is that there are multiple paths through the specifications:

  1. It is wrong to normalise /ws/v1/..;/..;/foo to /foo because RFC 3896 explicitly only allow a segment of exactly .. as a relative segments.
  2. But is is valid for the servlet spec to ignore internal parameters and thus it is OK to strip parameters from /ws/v1/..;/..;/foo to get /ws/v1/../../foo. If the application then subsequently normalises this, then it is valid for this to end up being normalised to /foo

So the URI is both /foo and not /foo. Ambiguity can then result. Within Jetty we are consistent, so it is not a problem, but I think it is a likely problem in application code, as we strip the internal parameters.

I think the best solution is to treat any URIs with segments of ..; or %2e%2e etc. as bad URIs. I have the code to do this already, but am just debating if we should do this for all HttpURI instances, or only when they are used for a request received by the server. Also, should we have a compliance mode to turn this on/off?

@oglueck
Copy link
Contributor

oglueck commented Feb 8, 2021

@sbordet I agree. It was new to me that each path segment could have parameters. And probably it is to reverse proxy implementors. Apache for example doesn't normalize it like that and will happily request the wrong backend.

@gregw
Copy link
Contributor

gregw commented Feb 8, 2021

@sbordet .... and we do just drop middle segment parameters.... that's the problem as it normalises to /foo when it should not according to a direct interpretation.
I think giving a 400 for any of these segments is the best way to not get involved in some kind of exploit, since whatever interpretation we adopt, if some intermediary or application adopts a different interpretation then problem can result. Best just to avoid the whole problem.

@sbordet
Copy link
Contributor

sbordet commented Feb 8, 2021

@gregw I don't see in https://tools.ietf.org/html/rfc3986#section-3.3 that .. must not have parameters.

In any case, I agree that a 400 is acceptable -- such weird paths are typically just used as attacks, not in real applications.

@gregw
Copy link
Contributor

gregw commented Feb 8, 2021

@sbordet the RFC talks about

the special "." and ".." complete path segments

Also it says:

Aside from dot-segments in hierarchical paths, a path segment is
considered opaque by the generic syntax. URI producing applications
often use the reserved characters allowed in a segment to delimit
scheme-specific or dereference-handler-specific subcomponents. For
example, the semicolon (";") and equals ("=") reserved characters are
often used to delimit parameters and parameter values applicable to
that segment.

So . and .. resolution take place in the general URI normalisation; whilst things like parameters are scheme-specific.
There is nothing in the RFC that allows you to strip parameters and then normalise .. and .

gregw added a commit that referenced this issue Feb 8, 2021
Fix #4275 fail URIs with ambiguous segments.
@gregw gregw linked a pull request Feb 8, 2021 that will close this issue
@gregw gregw closed this as completed in 20ef71f Feb 16, 2021
This was referenced Mar 13, 2021
gregw added a commit that referenced this issue Sep 28, 2021
A URI like `/foo/%2e%2e;/bar` should be ambiguous both because of the encoded dots and because of the parameters.  This means that the default setting of jetty-9 is a bit more secure as this path is considered ambiguous if either Violation.SEGMENT or Violation.PARAM is set.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Oct 11, 2021
* Improve #4275 ambiguous URIs

A URI like `/foo/%2e%2e;/bar` should be ambiguous both because of the encoded dots and because of the parameters.  This means that the default setting of jetty-9 is a bit more secure as this path is considered ambiguous if either Violation.SEGMENT or Violation.PARAM is set.
@gregw
Copy link
Contributor

gregw commented Oct 11, 2021

Need to cherry-pick #6939 to jetty-10

@gregw gregw reopened this Oct 11, 2021
lachlan-roberts added a commit that referenced this issue Oct 12, 2021
* Improve #4275 ambiguous URIs

A URI like `/foo/%2e%2e;/bar` should be ambiguous both because of the encoded dots and because of the parameters.  This means that the default setting of jetty-9 is a bit more secure as this path is considered ambiguous if either Violation.SEGMENT or Violation.PARAM is set.

Signed-off-by: Lachlan Roberts <[email protected]>
lachlan-roberts added a commit that referenced this issue Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment