Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
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 :
On 14/10/2021 09:22, ma...@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
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
On 14/10/2021 12:37, Rémy Maucherat wrote: On Thu, Oct 14, 2021 at 11:40 AM Mark Thomas wrote: On 14/10/2021 10:25, Konstantin Kolinko wrote: 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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
On Thu, Oct 14, 2021 at 11:40 AM Mark Thomas wrote:
>
> On 14/10/2021 10:25, Konstantin Kolinko wrote:
> > чт, 14 окт. 2021 г. в 11:25, Mark Thomas :
> >>
> >> On 14/10/2021 09:22, ma...@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
> >>> 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-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
On 14/10/2021 10:25, Konstantin Kolinko wrote:
чт, 14 окт. 2021 г. в 11:25, Mark Thomas :
On 14/10/2021 09:22, ma...@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
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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
чт, 14 окт. 2021 г. в 11:25, Mark Thomas :
>
> On 14/10/2021 09:22, ma...@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
> > 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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Do not add a trailing / to a request URI during canonicalization.
On 14/10/2021 09:22, ma...@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 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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[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
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)
+
+Do not add a trailing / to a request URI during
+canonicalization. (markt)
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
