You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2021/04/28 17:28:28 UTC
[GitHub] [tomcat] markt-asf opened a new pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
markt-asf opened a new pull request #417:
URL: https://github.com/apache/tomcat/pull/417
Potential fix for https://bz.apache.org/bugzilla/show_bug.cgi?id=65272
Needs careful review, hence using a PR.
If you spot any potential ways an invalid HTTP request line or header could be:
- accepted as valid
- rejected later than necessary
- rejected for the 'wrong' reason
or any other potential parsing error please report it. For bonus points provide a test case that demonstrates the issue.
As always, anything that might have security implications for a current Tomcat release should go to security@a.o rather than here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622458605
##########
File path: webapps/docs/changelog.xml
##########
@@ -143,6 +143,12 @@
request line, ensure that all the available data is included in the
error message. (markt)
</fix>
+ <fix>
+ <bug>65272</bug>: Restore the optional HTTP feature that allows
+ <code>LF</code> to be treated as a line terminator as well as the
Review comment:
This is specifically for a line-terminator for request-line and headers, correct? Is it worth mentioning that specifically?
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -550,10 +550,15 @@ boolean parseRequestLine(boolean keptAlive, int connectionTimeout, int keepAlive
prevChr = chr;
chr = byteBuffer.get();
if (chr == Constants.CR) {
- // Possible end of request line. Need LF next.
+ // Possible end of request line. Need LF next else invalid.
} else if (prevChr == Constants.CR && chr == Constants.LF) {
+ // CRLF is the standard line terminator
end = pos - 1;
parsingRequestLineEol = true;
+ } else if (chr == Constants.LF) {
Review comment:
Is it worth "fingerprinting" a message to determine if it is self-consistent? That is, a message is allowable with LF-terminated request/headers but _only_ if *all* of its headers and the request line are terminated with LF (only)? Or is that being unnecessarily picky and complicating the code?
##########
File path: test/org/apache/coyote/http11/TestHttp11InputBuffer.java
##########
@@ -687,24 +691,6 @@ public void testInvalidEndOfRequestLine01() {
}
- @Test
Review comment:
I would leave this test in, but change its nature (i.e. require that the response code is 200). Or is your assertion that the "correct" test is below in `test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java `?
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -841,7 +846,8 @@ private HeaderParseStatus parseHeader() throws IOException {
if (chr == Constants.CR && prevChr != Constants.CR) {
Review comment:
This appears to essentially ignore (all?) CR characters in headers. I'm not sure that's what we want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
rmaucher commented on pull request #417:
URL: https://github.com/apache/tomcat/pull/417#issuecomment-831755178
+1 looking good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622818008
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -550,10 +550,15 @@ boolean parseRequestLine(boolean keptAlive, int connectionTimeout, int keepAlive
prevChr = chr;
chr = byteBuffer.get();
if (chr == Constants.CR) {
- // Possible end of request line. Need LF next.
+ // Possible end of request line. Need LF next else invalid.
} else if (prevChr == Constants.CR && chr == Constants.LF) {
+ // CRLF is the standard line terminator
end = pos - 1;
parsingRequestLineEol = true;
+ } else if (chr == Constants.LF) {
Review comment:
It would make the code a little more complex and there is nothing in the spec to suggest that self-consistency is required. If we were going to make the behaviour configurable I can see how this would fit into an option that allow varying degrees of LF use. Eg: none, consistent, any.
My current thinking is to essentially hard-code the any option unless someone identifies a good reaaon not to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622818008
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -550,10 +550,15 @@ boolean parseRequestLine(boolean keptAlive, int connectionTimeout, int keepAlive
prevChr = chr;
chr = byteBuffer.get();
if (chr == Constants.CR) {
- // Possible end of request line. Need LF next.
+ // Possible end of request line. Need LF next else invalid.
} else if (prevChr == Constants.CR && chr == Constants.LF) {
+ // CRLF is the standard line terminator
end = pos - 1;
parsingRequestLineEol = true;
+ } else if (chr == Constants.LF) {
Review comment:
It would make the code a little more complex and there is nothing in the spec to suggest that self-consistency is required. If we were going to make the behaviour configurable I can see how this would fit into an option that allow varying degrees of LF use. Eg: none, consistent, any.
My current thinking is to essentially hard-code the any option unless someone identifies a good reason not to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622821247
##########
File path: java/org/apache/coyote/http11/Http11InputBuffer.java
##########
@@ -841,7 +846,8 @@ private HeaderParseStatus parseHeader() throws IOException {
if (chr == Constants.CR && prevChr != Constants.CR) {
Review comment:
It shouldn't. What it should do is detect if a line starts with CRLF or LF and if it does set the HeaderParseStatus.DONE state. Anything else is passed through to be parsed as the start of the header name. That particular line is handling the case where the lines starts with CR which may be the start of a CRLF sequence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r625561783
##########
File path: test/org/apache/coyote/http11/TestHttp11InputBuffer.java
##########
@@ -687,24 +691,6 @@ public void testInvalidEndOfRequestLine01() {
}
- @Test
Review comment:
Yes, the correct test is below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #417:
URL: https://github.com/apache/tomcat/pull/417#discussion_r622815587
##########
File path: webapps/docs/changelog.xml
##########
@@ -143,6 +143,12 @@
request line, ensure that all the available data is included in the
error message. (markt)
</fix>
+ <fix>
+ <bug>65272</bug>: Restore the optional HTTP feature that allows
+ <code>LF</code> to be treated as a line terminator as well as the
Review comment:
Can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf merged pull request #417: Fix BZ 65272. Restore the use of LF as an HTTP line terminator
Posted by GitBox <gi...@apache.org>.
markt-asf merged pull request #417:
URL: https://github.com/apache/tomcat/pull/417
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org