Bug 53469 - possible bug in Response.normalize(CharChunk cc)
Summary: possible bug in Response.normalize(CharChunk cc)
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Servlet & JSP API (show other bugs)
Version: 7.0.28
Hardware: PC All
: P2 normal with 4 votes (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 12:45 UTC by Thyzz
Modified: 2014-02-17 13:47 UTC (History)
3 users (show)



Attachments
Sample app source (10.34 KB, application/x-zip-compressed)
2012-07-09 13:51 UTC, Thyzz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thyzz 2012-06-26 12:45:09 UTC
The Response.normalize(CharChunk cc) introduced in 7.0.28 introduced a bug.
See Bug ID 53062

The URL that is being encoded is:
../../resources/org.apache.wicket.markup.html.WicketEventReference/wicket-event.js?w:lm=1340711670
which causes a IllegalArgumentException in this method

java.lang.IllegalArgumentException
	at org.apache.catalina.connector.Response.normalize(Response.java:1799)
	at org.apache.catalina.connector.Response.toAbsolute(Response.java:1732)
	at org.apache.catalina.connector.Response.encodeURL(Response.java:1242)
	at org.apache.catalina.connector.ResponseFacade.encodeURL(ResponseFacade.java:406)
	at org.apache.wicket.protocol.http.WebResponse.encodeURL(WebResponse.java:149)
	at org.apache.wicket.protocol.http.request.WebRequestCodingStrategy.encode(WebRequestCodingStrategy.java:387)

Running the same application in tomcat 7.0.27 and 6.0.18 works without issue
Comment 1 Mark Thomas 2012-06-27 19:24:01 UTC
Your relative URL is not valid. The absolute URL will look something like:

http://host:port/something/../../resources/etc

That relative URL is not valid since you can't go up a level once you get to the root of the host.

Previous versions of Tomcat did not include this check. Tomcat 7.0.28 onwards does.
Comment 2 mgrigorov 2012-07-09 12:20:07 UTC
Hi Mark,

I see there is no test case attached and one can only speculate but why do you think that the absolute url will have just 'something' in the servlet path ?

It could be 'something/else/here/../../resources/etc' and in this case it will be OK.

The reporter filed an issue in Wicket Jira too: https://issues.apache.org/jira/browse/WICKET-4645 and the pasted urls seem OK to me.

@Jens: can you attach a simple application that demonstrates the failure so it should be easier for Tomcat devs to debug it.
Comment 3 Mark Thomas 2012-07-09 12:41:39 UTC
Because I tested it and included the tests in the Tomcat unit tests

http://host:port/something/../../resources/etc
fails

http://host:port/soemthing/else/../../resources/etc
works
Comment 4 mgrigorov 2012-07-09 12:52:27 UTC
OK, thanks!

In this case I'll let the reporter follow up with a test case that demonstrates the problem with the urls provided in Wicket Jira.
Comment 5 Thyzz 2012-07-09 13:51:05 UTC
Created attachment 29043 [details]
Sample app source

According to Martin in https://issues.apache.org/jira/browse/WICKET-4645
This still seems to be a tomcat issue.

So here's a sample app source. I'll attach the war as well.
Comment 6 Thyzz 2012-07-09 14:02:34 UTC
Not allowed to upload the war file.

unzip the zip.
inside test run: mvn clean package
deploy the target/test-1.war to tomcat
hit it with http://localhost:8080/test-1, click the link and everything works 
do the same with http://localhost:8080/test-1/testpage/param1/value1, then click the link and check the tomcat log...
Comment 7 Mark Thomas 2012-07-09 14:39:00 UTC
Both the Tomcat access log and Firebug show that the request when clicking on the link displayed on http://localhost:8080/test-1/testpage/param1/value1 is for:

http://localhost:8080/test-1/?wicket:interface=:9:link::IBehaviorListener:0:2&random=0.9448873634935631

The application then attempts to generate a redirect from this page to:
http://localhost:8080/test-1/../../?wicket:interface=:9::::

That redirect is not valid.
Comment 8 papegaaij 2012-07-17 09:27:37 UTC
The problem lies in the fact that encodeURL rewrites the url to absolute. I think this should only be done in encodeRedirectURL. Wicket also calls encodeURL to encode urls that are rendered in hrefs on the response page. Tomcat rewriting them to absolute breaks this badly. This is best illustrated by an example:

Suppose you are at /a/b and you click a link to /a/b/c/d. This page contains a link to /1/2. Wicket renders all urls relative, so the link to /1/2 will be ../../../1/2 (it is relative to /a/b/c/d). This link will be passed to Response.encodeURL, which rewrites it to /a/../../../1/2, which obviously is wrong and fails.

If you go the other way: you are at /a/b/c/d and click a link to /a/b, which contains a link to /1/2, things go wrong in a different way. The link to /1/2, relative to /a/b, will be ../1/2. Tomcat will rewrite it to the absolute url /a/b/c/../1/2, which is normalized to /a/b/1/2. However, because I've got cookies enabled, isEncodeable returns false and the unencoded url is returned.
Comment 9 Mark Thomas 2012-07-20 20:40:33 UTC
(In reply to comment #8)
> The problem lies in the fact that encodeURL rewrites the url to absolute. I
> think this should only be done in encodeRedirectURL.

That is a fair point. We'll get that addressed for the next release. However...

> Wicket also calls
> encodeURL to encode urls that are rendered in hrefs on the response page.
> Tomcat rewriting them to absolute breaks this badly.

That suggests something else is going on that I'd like to get to the bottom of.

> Suppose you are at /a/b and you click a link to /a/b/c/d. This page contains
> a link to /1/2. Wicket renders all urls relative, so the link to /1/2 will
> be ../../../1/2 (it is relative to /a/b/c/d). This link will be passed to
> Response.encodeURL, which rewrites it to /a/../../../1/2, which obviously is
> wrong and fails.

Huh? If you click a link to /a/b/c/d and the response contains a relative link ../../../1/2 then the result should be /a/b/c/../../../1/2 == /1/2 (i.e. relative to the current request). Why does Tomcat/Wicket think it should be relative to the previous request for /a/b ?
Comment 10 papegaaij 2012-07-23 06:32:21 UTC
(In reply to comment #9)
> > Suppose you are at /a/b and you click a link to /a/b/c/d. This page contains
> > a link to /1/2. Wicket renders all urls relative, so the link to /1/2 will
> > be ../../../1/2 (it is relative to /a/b/c/d). This link will be passed to
> > Response.encodeURL, which rewrites it to /a/../../../1/2, which obviously is
> > wrong and fails.
> 
> Huh? If you click a link to /a/b/c/d and the response contains a relative
> link ../../../1/2 then the result should be /a/b/c/../../../1/2 == /1/2
> (i.e. relative to the current request). Why does Tomcat/Wicket think it
> should be relative to the previous request for /a/b ?

This has to do with the way Wicket renders links and handles requests. The link on /a/b, might look like ./b?2-1.ILinkListener-link. During processing of this link, user-code can send the user to /a/b/c/d. This causes the page for /a/b/c/d to be rendered as part of the request on /a/b?2-1.ILinkListener-link. The result of this request is a redirect to /a/b/c/d. The GET on this url simply fetches the rendered page from a buffer.
Comment 11 Mark Thomas 2012-07-24 16:47:53 UTC
OK. I see what the problem is here. It isn't quite as clear cut as comment #8 suggests.

To determine if the URL should be encoded, Tomcat attempts the following checks:
a) is it not an intra-document reference
b) is there a current session
c) are cookies being used
d) is URL re-writing permitted
e) does the absolute URL match down to the context path

The problem is with check e). Now Tomcat normalizes any absolute URLs (a perfectly valid thing to do) then absolute URLs that fail normalization now fail the encoding process.

Note that prior to the addition of normalization, check e) always passed for any relative URL. That is a bug that has gone unnoticed for many years. (With the addition of mormalization, it is now fixed).

Note also that while Tomcat normalizes the URL for check e) it uses the original URL to construct the returned value.

The failed normalization is easily caught in Tomcat but it means that any URLs for which normalization fails will not have the session ID encoded in the URL.

This then comes back to what Wicket is trying to do. From your description, during the generation of the response to /a/b Wicket is pre-generating the content for /a/b/c/d and /a/b/c/d may contain links with relative URLs. It is the encoding of these URLs that is failing since Tomcat believes there are relative to the current request/response for /a/b rather than /a/b/c/d. This does seem a little unusual and not something that was anticipated by either the Tomcat developers or the Servlet EG.

I'm not sure how to handle this. The Servlet specification does not state that relative URLs passed to encodeURL must be treated as relative to the current request/response but equally, what else could they be relative to? It is more clear cut for sendRedirect since Tomcat is required to return the absolute (and now normalized) URL.

I am tempted to say the Wicket should be wrapping the HttpServletRequest during the generation of /a/b/c/d to return the correct URL and wrapping the HttpServletResponse to link it to the wrapped request although I'm not sure how well that would go down with the Wicket developers.

As a minimum, Tomcat needs to be patched to catch a normalization failure and simply not encode the URL in that case.

Thoughts, feedback and corrections welcome.
Comment 12 papegaaij 2012-07-25 09:07:31 UTC
In my opinion, Tomcat should not convert relative URLs to absolute in encodeURL. That should only be done in encodeRedirectURL. encodeURL can still perform normalization, as long as it preserves relative URLs. For example, ./a/../b can be normalized to ./b, but it should not be converted into an absolute URL.
Comment 13 mgrigorov 2012-07-25 09:15:25 UTC
I agree with Emond.
By Servlet spec (actually the javadoc of javax.servlet.http.HttpServletResponse#sendRedirect) only #sendRedirect() should convert the url from relative to absolute. I think this is the place where URL normalization should happen.

Using HttpServletRequestWrapper in Wicket in an option but this will solve the problem only in Wicket. Every other framework or plain application may face the same problem.
Comment 14 Mark Thomas 2012-07-25 18:02:43 UTC
Folks, please re-read comment #11.

The output of encodeURL() is not and never will be normalized.

However, the Javadoc for encodeURL() allows/requires Tomcat to check if the session needs to be encoded in the provided URL. One of the checks Tomcat uses is whether or not the URL provided to encodeURL() is part of the web application. To do this correctly Tomcat has to construct a absolute, normalized URL to check whether the resulting URL is within the web application. This requires converting relative URLs to absolute and the only basis Tomcat has for doing this is the current request URL.

Wicket is doing "unusual" things in pre-generating content for a different URL than the current one. This is causing problems for relative URLs. I have yet to find any evidence that any other framework does this. At the moment this looks like a Wicket specific issue.

As previously stated, I will be changing Tomcat so that if the "is the URL part of the webapp" test fails (e.g. because normalization fails) the result will be that the session ID is not added to the URL. That may or may not be sufficient to fix this for Wicket. Some feedback on this would be appreciated.

In terms of further fixing, I am leaning towards this being a Wicket specific issue (and hence a Wicket problem to fix) but I am open to contrary arguments.
Comment 15 papegaaij 2012-07-26 07:05:11 UTC
Ok, things are getting more clear to me now. Isn't it better to assume that relative URLs always fall within the webapplication? The worst thing that could happen, is that a jsessionid gets appended where it was not needed, which I think is better than leave it out in cases where it should have been appended.

The specification is not clear at this point. For sendRedirect it is: "If the location is relative without a leading '/' the container interprets it as relative to the current request URI.". The absence of such a statement in the documentation of encodeURL indicates to me that it is not safe to make the assumption that relative URLs passed to encodeURL are relative to the current request. I think Tomcat should do the best it can without assuming anything that's not backed up by the documentation, even if it means that the outcome may be less than perfect.
Comment 16 Mark Thomas 2012-07-29 20:35:37 UTC
I have fixed the IAE in trunk and 7.0.x.

I am leaving this open while I wait for clarification from the Servlet EG as to how relative URLs passed to encodeURL should be treated.

See http://java.net/jira/browse/SERVLET_SPEC-43
Comment 17 wanshoupu 2012-08-20 22:29:40 UTC
(In reply to comment #16)
> I have fixed the IAE in trunk and 7.0.x.
> 
> I am leaving this open while I wait for clarification from the Servlet EG as
> to how relative URLs passed to encodeURL should be treated.
> 
> See http://java.net/jira/browse/SERVLET_SPEC-43

Tomcat needs to be patched to catch a normalization failure and simply not encode the URL in that case.

I totally agree with this solution. Has it been so fixed anywhere?
My webapplication generated something like this: 
https://localhost:3443/vcbs/../../../../../?wicket:interface=:18::::
which absolutely failed the 'within-server-root' test after normalization.

I look forward to this being patched in Tomcat.
Comment 18 mgrigorov 2012-08-27 08:27:25 UTC
(In reply to comment #16)
> I have fixed the IAE in trunk and 7.0.x.

Unfortunately with this fix apps that work only with jsessionid encoded in the url will still break. If JSESSIONID is in a cookie then all is OK because Tomcat wont even try to normalize in this case. But if Tomcat is configured to always encode it in the url and the normalization fails then with this fix the relative url will not have the jsessionid and next request will be considered as not authenticated and depending on the application configuration it will most probably lead to a redirect to the login page.

Unfortunately I don't see how to fix this in Wicket too. There is no way to inform the servlet container that there is a new base url which should be used for the resolving. The only solution is to configure the Wicket application to use REDIRECT_TO_RENDER strategy instead of REDIRECT_TO_BUFFER (the default one). With REDIRECT_TO_RENDER Wicket will make a http redirect before starting the render of the page responsible for /a/b/c/d.
Comment 19 mgrigorov 2012-08-27 11:55:28 UTC
I think I found the solution. Wicket can pass always an absolute url to the web container for encoding. Wicket knows the current base url and can make it absolute safely. This way the web container will only try to normalize it but this will do nothing since there are no '../' in the passed absolute url. After encoding it Wicket can make it relative again and write it in the response.
Comment 20 Mark Thomas 2012-09-16 18:22:11 UTC
Marking as NEEDINFO as I am waiting for a response from the Servlet EG.
Comment 21 Tino Kissig 2012-10-29 13:33:17 UTC
At our current system we're getting the IllegalArgumentException when Tomcat tries to normalize such a relative url for a redirect.

java.lang.IllegalArgumentException
	at org.apache.catalina.connector.Response.normalize(Response.java:1821)
	at org.apache.catalina.connector.Response.toAbsolute(Response.java:1741)
	at org.apache.catalina.connector.Response.encodeRedirectURL(Response.java:1210)
	at org.apache.catalina.connector.ResponseFacade.encodeRedirectURL(ResponseFacade.java:418)
	at javax.servlet.http.HttpServletResponseWrapper.encodeRedirectURL(HttpServletResponseWrapper.java:85)
	at javax.servlet.http.HttpServletResponseWrapper.encodeRedirectURL(HttpServletResponseWrapper.java:85)
	at org.apache.wicket.protocol.http.WebResponse.redirect(WebResponse.java:210)
	at org.apache.wicket.protocol.http.BufferedWebResponse.close(BufferedWebResponse.java:67)

Do you think that this is related to this bug or to bug 53062?
Comment 22 mgrigorov 2012-10-29 13:37:49 UTC
Wicket has a fix in versions 1.5.9 (not released yet) and 6.1.0.
Which version do you use ?
Comment 23 Tino Kissig 2012-10-29 13:39:24 UTC
(In reply to comment #22)
> Wicket has a fix in versions 1.5.9 (not released yet) and 6.1.0.
> Which version do you use ?

Unfortunately we still use 1.4.18 and don't have the resources yet to upgrade to 1.5.x or even 6.x.
Comment 24 Mark Thomas 2013-02-09 09:37:24 UTC
The Servlet EG has confirmed that Tomcat's current behaviour is correct. Relative URLs are relative to the current request. They are not assumed to be part of the web application.

I'm marking this as fixed as the IAE that originally triggered this issue has been fixed.