You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2021/10/14 08:22:22 UTC
[tomcat] branch main updated: Do not add a trailing / to a request
URI during canonicalization.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 70d4e9b Do not add a trailing / to a request URI during canonicalization.
70d4e9b is described below
commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Oct 13 18:28:45 2021 +0100
Do not add a trailing / to a request URI during canonicalization.
This is part of the clarification in Servet 6.0 of the expected
canonicalization Servlet containers are expected to apply to request
URIs.
---
java/org/apache/catalina/connector/CoyoteAdapter.java | 9 ++++++++-
test/org/apache/catalina/connector/TestCoyoteAdapter.java | 10 +++++++++-
webapps/docs/changelog.xml | 4 ++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java
index 053874f..046cc4c 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -1149,6 +1149,7 @@ public class CoyoteAdapter implements Adapter {
final byte[] b = uriBC.getBytes();
final int start = uriBC.getStart();
int end = uriBC.getEnd();
+ boolean appendedSlash = false;
// An empty URL is not acceptable
if (start == end) {
@@ -1197,6 +1198,7 @@ public class CoyoteAdapter implements Adapter {
&& (b[end - 3] == (byte) '/'))) {
b[end] = (byte) '/';
end++;
+ appendedSlash = true;
}
}
@@ -1241,8 +1243,13 @@ public class CoyoteAdapter implements Adapter {
index = index2;
}
- return true;
+ // If a slash was appended to help normalize "/." or "/.." then remove
+ // any trailing "/" from the result unless the result is "/".
+ if (appendedSlash && end > 1 && b[end - 1]== '/') {
+ uriBC.setEnd(end -1);
+ }
+ return true;
}
diff --git a/test/org/apache/catalina/connector/TestCoyoteAdapter.java b/test/org/apache/catalina/connector/TestCoyoteAdapter.java
index 464ca90..72f26b8 100644
--- a/test/org/apache/catalina/connector/TestCoyoteAdapter.java
+++ b/test/org/apache/catalina/connector/TestCoyoteAdapter.java
@@ -328,10 +328,18 @@ public class TestCoyoteAdapter extends TomcatBaseTest {
doTestNormalize("/foo/../bar", "/bar");
}
+ @Test
+ public void testNormalize02() {
+ doTestNormalize("/foo/.", "/foo");
+ }
+
private void doTestNormalize(String input, String expected) {
MessageBytes mb = MessageBytes.newInstance();
byte[] b = input.getBytes(StandardCharsets.UTF_8);
- mb.setBytes(b, 0, b.length);
+ // Need to allow an extra byte in case '/' is appended during processing
+ byte[] b2 = new byte[b.length + 1];
+ System.arraycopy(b, 0, b2, 0, b.length);
+ mb.setBytes(b2, 0, b.length);
boolean result = CoyoteAdapter.normalize(mb, false);
mb.toString();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index fb6b2d0..2be62e9 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -122,6 +122,10 @@
aligns Apache Tomcat with recent changes in the Jakarta Servlet
specification project. (markt)
</add>
+ <fix>
+ Do not add a trailing <code>/</code> to a request URI during
+ canonicalization. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark, Konstantin,
On 10/14/21 05:40, Mark Thomas wrote:
> On 14/10/2021 10:25, Konstantin Kolinko wrote:
>> чт, 14 окт. 2021 г. в 11:25, Mark Thomas <ma...@apache.org>:
>>>
>>> On 14/10/2021 09:22, markt@apache.org wrote:
>>>> This is an automated email from the ASF dual-hosted git repository.
>>>>
>>>> markt pushed a commit to branch main
>>>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>>>
>>>>
>>>> The following commit(s) were added to refs/heads/main by this push:
>>>> new 70d4e9b Do not add a trailing / to a request URI during
>>>> canonicalization.
>>>> 70d4e9b is described below
>>>>
>>>> commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
>>>> Author: Mark Thomas <ma...@apache.org>
>>>> AuthorDate: Wed Oct 13 18:28:45 2021 +0100
>>>>
>>>> Do not add a trailing / to a request URI during canonicalization.
>>>>
>>>> This is part of the clarification in Servet 6.0 of the expected
>>>> canonicalization Servlet containers are expected to apply to
>>>> request
>>>> URIs.
>>>
>>> All,
>>>
>>> This is the first of several clarifications. The question is do we want
>>> to back-port this change to earlier versions?
>>>
>>> My current thinking is that we should as the current behaviour looks
>>> wrong. We add a trailing "/" to simplify the normalization algorithm but
>>> then don't remove it after we have completed normalization.
>>>
>>
>> Hi!
>>
>> I have not thought about this in detail.
>> Just several quick observations / quick thoughts.
>>
>> a. Generally, I like doing things correctly.
>>
>> b. Looking at test example:
>>
>>> doTestNormalize("/foo/.", "/foo");
>>
>> It can be seen that "foo" is a directory,
>> and thus I think it actually behaves as follows:
>> Old behaviour:
>> 1. Serve content of "/foo/"
>>
>> New behaviour:
>> 1. As "/foo" is a directory, Tomcat will likely won't serve it, but
>> will respond with a 302-redirect to "/foo/"
>> 2. Serve content of "/foo/".
>>
>> It is one more round-trip, but at least the browser will display a
>> correct normalized URL.
>
> The extra round-trip annoys me a little. But then I think if that is an
> issue for the user agent, submit a sensible URI in the first place.
>
>> c. I think that browsers usually normalize URLs before making a
>> request, though I am not 100% sure. If so, the non-normalized URLs
>> will come from elsewhere, not from a browser. (And so there will be no
>> difference for browsers).
>
> I can think a few possible sources:
>
> - reverse proxies with possibly inefficient/wrong configuration
>
> - attackers trying to exploit a reverse proxy / servlet container
> combination
>
> - application generate URIs where the URiu has been generated by
> concatenating various strings
>
>
>> d. If backporting, it would better be configurable.
>
> Yeah, I know. I'd like to avoid lots of new configuration options. Maybe
> a single new option "servlet6Canonicalization" for all the changes I am
> proposing (there are a few more commits to come)?
I agree that this needs to be an option for Tomcats 8 and 9. I'm fine
with a single switch to change from the old behavior to the new.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2021 12:37, Rémy Maucherat wrote:
> On Thu, Oct 14, 2021 at 11:40 AM Mark Thomas <ma...@apache.org> wrote:
>>
>> On 14/10/2021 10:25, Konstantin Kolinko wrote:
<snip/>
>>> d. If backporting, it would better be configurable.
>>
>> Yeah, I know. I'd like to avoid lots of new configuration options. Maybe
>> a single new option "servlet6Canonicalization" for all the changes I am
>> proposing (there are a few more commits to come)?
>
> These spec changes are great for consistency. However, I think it's a
> major problem to backport, it's way better to leave things in place
> and wait for that 9.x "branch" to force it on users. Ok for a general
> "servlet6" configuration option to allow users to try out if they are
> interested.
Ack. I'm going to wait for the Servlet 6 spec changes to be finalised
before looking at a backport (with a configuration option).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Oct 14, 2021 at 11:40 AM Mark Thomas <ma...@apache.org> wrote:
>
> On 14/10/2021 10:25, Konstantin Kolinko wrote:
> > чт, 14 окт. 2021 г. в 11:25, Mark Thomas <ma...@apache.org>:
> >>
> >> On 14/10/2021 09:22, markt@apache.org wrote:
> >>> This is an automated email from the ASF dual-hosted git repository.
> >>>
> >>> markt pushed a commit to branch main
> >>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >>>
> >>>
> >>> The following commit(s) were added to refs/heads/main by this push:
> >>> new 70d4e9b Do not add a trailing / to a request URI during canonicalization.
> >>> 70d4e9b is described below
> >>>
> >>> commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
> >>> Author: Mark Thomas <ma...@apache.org>
> >>> AuthorDate: Wed Oct 13 18:28:45 2021 +0100
> >>>
> >>> Do not add a trailing / to a request URI during canonicalization.
> >>>
> >>> This is part of the clarification in Servet 6.0 of the expected
> >>> canonicalization Servlet containers are expected to apply to request
> >>> URIs.
> >>
> >> All,
> >>
> >> This is the first of several clarifications. The question is do we want
> >> to back-port this change to earlier versions?
> >>
> >> My current thinking is that we should as the current behaviour looks
> >> wrong. We add a trailing "/" to simplify the normalization algorithm but
> >> then don't remove it after we have completed normalization.
> >>
> >
> > Hi!
> >
> > I have not thought about this in detail.
> > Just several quick observations / quick thoughts.
> >
> > a. Generally, I like doing things correctly.
> >
> > b. Looking at test example:
> >
> >> doTestNormalize("/foo/.", "/foo");
> >
> > It can be seen that "foo" is a directory,
> > and thus I think it actually behaves as follows:
> > Old behaviour:
> > 1. Serve content of "/foo/"
> >
> > New behaviour:
> > 1. As "/foo" is a directory, Tomcat will likely won't serve it, but
> > will respond with a 302-redirect to "/foo/"
> > 2. Serve content of "/foo/".
> >
> > It is one more round-trip, but at least the browser will display a
> > correct normalized URL.
>
> The extra round-trip annoys me a little. But then I think if that is an
> issue for the user agent, submit a sensible URI in the first place.
The idea of the behavior was to save the round trip, indeed.
> > c. I think that browsers usually normalize URLs before making a
> > request, though I am not 100% sure. If so, the non-normalized URLs
> > will come from elsewhere, not from a browser. (And so there will be no
> > difference for browsers).
>
> I can think a few possible sources:
>
> - reverse proxies with possibly inefficient/wrong configuration
>
> - attackers trying to exploit a reverse proxy / servlet container
> combination
>
> - application generate URIs where the URiu has been generated by
> concatenating various strings
>
>
> > d. If backporting, it would better be configurable.
>
> Yeah, I know. I'd like to avoid lots of new configuration options. Maybe
> a single new option "servlet6Canonicalization" for all the changes I am
> proposing (there are a few more commits to come)?
These spec changes are great for consistency. However, I think it's a
major problem to backport, it's way better to leave things in place
and wait for that 9.x "branch" to force it on users. Ok for a general
"servlet6" configuration option to allow users to try out if they are
interested.
Rémy
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2021 10:25, Konstantin Kolinko wrote:
> чт, 14 окт. 2021 г. в 11:25, Mark Thomas <ma...@apache.org>:
>>
>> On 14/10/2021 09:22, markt@apache.org wrote:
>>> This is an automated email from the ASF dual-hosted git repository.
>>>
>>> markt pushed a commit to branch main
>>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>>
>>>
>>> The following commit(s) were added to refs/heads/main by this push:
>>> new 70d4e9b Do not add a trailing / to a request URI during canonicalization.
>>> 70d4e9b is described below
>>>
>>> commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
>>> Author: Mark Thomas <ma...@apache.org>
>>> AuthorDate: Wed Oct 13 18:28:45 2021 +0100
>>>
>>> Do not add a trailing / to a request URI during canonicalization.
>>>
>>> This is part of the clarification in Servet 6.0 of the expected
>>> canonicalization Servlet containers are expected to apply to request
>>> URIs.
>>
>> All,
>>
>> This is the first of several clarifications. The question is do we want
>> to back-port this change to earlier versions?
>>
>> My current thinking is that we should as the current behaviour looks
>> wrong. We add a trailing "/" to simplify the normalization algorithm but
>> then don't remove it after we have completed normalization.
>>
>
> Hi!
>
> I have not thought about this in detail.
> Just several quick observations / quick thoughts.
>
> a. Generally, I like doing things correctly.
>
> b. Looking at test example:
>
>> doTestNormalize("/foo/.", "/foo");
>
> It can be seen that "foo" is a directory,
> and thus I think it actually behaves as follows:
> Old behaviour:
> 1. Serve content of "/foo/"
>
> New behaviour:
> 1. As "/foo" is a directory, Tomcat will likely won't serve it, but
> will respond with a 302-redirect to "/foo/"
> 2. Serve content of "/foo/".
>
> It is one more round-trip, but at least the browser will display a
> correct normalized URL.
The extra round-trip annoys me a little. But then I think if that is an
issue for the user agent, submit a sensible URI in the first place.
> c. I think that browsers usually normalize URLs before making a
> request, though I am not 100% sure. If so, the non-normalized URLs
> will come from elsewhere, not from a browser. (And so there will be no
> difference for browsers).
I can think a few possible sources:
- reverse proxies with possibly inefficient/wrong configuration
- attackers trying to exploit a reverse proxy / servlet container
combination
- application generate URIs where the URiu has been generated by
concatenating various strings
> d. If backporting, it would better be configurable.
Yeah, I know. I'd like to avoid lots of new configuration options. Maybe
a single new option "servlet6Canonicalization" for all the changes I am
proposing (there are a few more commits to come)?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Konstantin Kolinko <kn...@gmail.com>.
чт, 14 окт. 2021 г. в 11:25, Mark Thomas <ma...@apache.org>:
>
> On 14/10/2021 09:22, markt@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > markt pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> > new 70d4e9b Do not add a trailing / to a request URI during canonicalization.
> > 70d4e9b is described below
> >
> > commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
> > Author: Mark Thomas <ma...@apache.org>
> > AuthorDate: Wed Oct 13 18:28:45 2021 +0100
> >
> > Do not add a trailing / to a request URI during canonicalization.
> >
> > This is part of the clarification in Servet 6.0 of the expected
> > canonicalization Servlet containers are expected to apply to request
> > URIs.
>
> All,
>
> This is the first of several clarifications. The question is do we want
> to back-port this change to earlier versions?
>
> My current thinking is that we should as the current behaviour looks
> wrong. We add a trailing "/" to simplify the normalization algorithm but
> then don't remove it after we have completed normalization.
>
Hi!
I have not thought about this in detail.
Just several quick observations / quick thoughts.
a. Generally, I like doing things correctly.
b. Looking at test example:
> doTestNormalize("/foo/.", "/foo");
It can be seen that "foo" is a directory,
and thus I think it actually behaves as follows:
Old behaviour:
1. Serve content of "/foo/"
New behaviour:
1. As "/foo" is a directory, Tomcat will likely won't serve it, but
will respond with a 302-redirect to "/foo/"
2. Serve content of "/foo/".
It is one more round-trip, but at least the browser will display a
correct normalized URL.
c. I think that browsers usually normalize URLs before making a
request, though I am not 100% sure. If so, the non-normalized URLs
will come from elsewhere, not from a browser. (And so there will be no
difference for browsers).
d. If backporting, it would better be configurable.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a
request URI during canonicalization.
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2021 09:22, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 70d4e9b Do not add a trailing / to a request URI during canonicalization.
> 70d4e9b is described below
>
> commit 70d4e9ba0a81a1d782fa50695a18d23f2f1f179c
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Wed Oct 13 18:28:45 2021 +0100
>
> Do not add a trailing / to a request URI during canonicalization.
>
> This is part of the clarification in Servet 6.0 of the expected
> canonicalization Servlet containers are expected to apply to request
> URIs.
All,
This is the first of several clarifications. The question is do we want
to back-port this change to earlier versions?
My current thinking is that we should as the current behaviour looks
wrong. We add a trailing "/" to simplify the normalization algorithm but
then don't remove it after we have completed normalization.
Thoughts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org