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