jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
266 stars 87 forks source link

Clarify normalization of methods taking paths #453

Closed gregw closed 5 months ago

gregw commented 2 years ago

After #424 for #18, we need to further clarify how methods that take paths will treat non-normal and/or encoded characters.

An example of an issue could be an application that take pathInfo for a servlet mapped to / and uses that to request a resource or request dispatcher. If a URI like /foo/%252e%252e/WEB-INF/web.xml is received, then that will decode to a pathInfo of /foo/%25%25/WEB-INF/web.xml. How should that arg be treated by thos methods? Should it be rejected because the method doesn't accept encoded paths? Or should it be decoded and normalized to /WEB-INF/web.xml, but then rejected because it is not the canonical path?

Note that getRequestDispatcher allows for relative paths within the docroot, so asking for ../foo/file.txt should be allowed, even though it is not canonical. But should %2e%2e/foo/file.txt be allowed?

The getResource method has the following warning:

This method bypasses both implicit (no direct access to WEB-INF or META-INF) and explicit (defined by the web application) security constraints. Care should be taken both when constructing the path (e.g. avoid unsanitized user provided data) and when using the result not to create a security vulnerability in the application.

Methods that need clarification are:

gregw commented 2 years ago

@markt-asf @stuartwdouglas Did we come to a consensus of the best fix for these kind of issues?

markt-asf commented 2 years ago

I don't think we had consensus.

My expectation is that RequestDispatcher is going to have to decode the path. My reasoning is:

The warning in getResource() should be on all of those methods.

For the example you above, my response is that applications shouldn't do that as it is passing unsanitized user provided data. Hence why I think the warning should be on all of those methods.

If a URL such as the example above was passed to a RequestDispatcher I'd expect it to be decoded, normalized and then mapped using the same rules as we defined for #424. In this case it would get rejected as suspicious unless the suspicious URI checks had been explicitly disabled.

Generally, I'd expect the same approach for all of these methods. Decoded the path, normalize it (including making relative paths absolute where relative paths are allowed) using the same rules as #424.

gregw commented 2 years ago

I think putting the warning on all the methods is a good first step. I'll do a PR.

I'm not sure there is a second step that works generally.

joakime commented 8 months ago

We should probably reopen this as we are getting reports that this clarification in the 6.1 javadoc is incomplete.

From https://www.eclipse.org/lists/servlet-dev/msg00634.html

Hi,

github.com/jakartaee/servlet/issues/453

clarifies the path parameter in ServletContext and ServletRequest.

ServletContext#getRequestDispatcher(java.lang.String) and ServletRequest#getRequestDispatcher(java.lang.String)

However, the PR github.com/jakartaee/servlet/pull/487 does not have an update for ServletRequest#getRequestDispatcher(java.lang.String)

Is it an oversight?

Thanks, Phu Dinh

pmd1nh commented 8 months ago

@markt-asf @gregw

pmd1nh commented 8 months ago

I reopened this issue for clarification of

ServletRequest#getRequestDispatcher(java.lang.String)

markt-asf commented 7 months ago

Fixed by #580

gregw commented 5 months ago

@markt-asf @stuartwdouglas I think we have a bit of a contradiction in getContextPath(). The javadoc current says both:

The container does not decode this string. and The path will be canonicalized as per Servlet 3.5

But step 5. of canonicalization in 3.5 is decode!

So is this now a decoded string, or an encoded string (as it strangely always was before?)

markt-asf commented 5 months ago

You have re-opened the wrong issue. That text was only reformatted by the fix to this issue. The original change was made to address #18.