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

Clarify normalization of methods taking paths #453

Closed
gregw opened this issue Nov 30, 2021 · 9 comments · Fixed by #487
Closed

Clarify normalization of methods taking paths #453

gregw opened this issue Nov 30, 2021 · 9 comments · Fixed by #487

Comments

@gregw
Copy link
Contributor

gregw commented Nov 30, 2021

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:

  • ServletContext#getRequestDispatcher(java.lang.String)
  • ServletContent#getRealPath(java.lang.String)
  • ServletContent#getResource(java.lang.String)
  • ServletContent#getResourcePaths(java.lang.String)
  • ServletContent#getResourceAsStream(java.lang.String)
  • ServletRequest#getRequestDispatcher(java.lang.String)
@gregw
Copy link
Contributor Author

gregw commented Nov 30, 2021

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

@markt-asf
Copy link
Contributor

I don't think we had consensus.

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

  • the servlet spec says that path can include a query string;
  • the query string is delimited by '?';
  • '?' is a valid character in the path (if it is encoded);
  • users may want to use '?' in the path;
  • therefore users need to be able to provide an encoded path;
  • therefore the RequestDispatcher needs to decode the path.

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
Copy link
Contributor Author

gregw commented Nov 30, 2021

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
Copy link

joakime commented Feb 14, 2024

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
Copy link

pmd1nh commented Feb 23, 2024

@markt-asf @gregw

@pmd1nh pmd1nh reopened this Feb 26, 2024
@pmd1nh
Copy link

pmd1nh commented Feb 26, 2024

I reopened this issue for clarification of

ServletRequest#getRequestDispatcher(java.lang.String)

@markt-asf
Copy link
Contributor

Fixed by #580

@gregw
Copy link
Contributor Author

gregw commented May 23, 2024

@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?)

@gregw gregw reopened this May 23, 2024
@markt-asf
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants