Re: [GitHub] [tomcat] alpire opened a new pull request #176: CoyoteAdapter: fix out-of-bounds read in checkNormalize
On 29/06/2019 02:26, GitBox wrote: > alpire opened a new pull request #176: CoyoteAdapter: fix out-of-bounds read > in checkNormalize > URL: https://github.com/apache/tomcat/pull/176 > > >On malformed requests, checkNormalize would throw an > ArrayIndexOutOfBoundsException leading to a 500 response. This change fixes > checkNormalize to return false instead of throwing exception on those inputs, > and adds a few tests to check the new functionality. That there are URI sequences that can trigger this is certainly something that we need to fix. On a slightly different topic, this got me thinking. checkNormalize() is a test that runs on every request. It is essentially there to ensure that whatever character set has been specified for the Connector is "safe". In this case "safe" means when the bytes are converted to characters we don't get any unexpected "." or "/" characters in the result that make the character version of the URI non-normalized. Rather than run this test on every request, we could: - pre-calculate the character sets we consider to be safe - throw an exception if the user attempts to configure the Connector to use one I think we could safely exclude any character set where any of the following were not true: - 0x00 <-> '\u' - 0x2e <-> '.' - 0x2f <-> '/' - 0x5c <-> '\' We could create a unit test that maintains/checks the list of "safe" character set canonical names. After creating the character set in Connector.setURIEncoding(), if the canonical name of the resulting character set is not in the safe list, throw an exception. Then remove checkNormalize() and replace with a comment that explains why the conversion is known to be safe. There are several loops through the URI in checkNormalize(). I think - I'd need to test to confirm - that removing them would provide a measurable performance benefit. Thoughts? Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #176: CoyoteAdapter: fix out-of-bounds read in checkNormalize
markt-asf commented on a change in pull request #176: CoyoteAdapter: fix
out-of-bounds read in checkNormalize
URL: https://github.com/apache/tomcat/pull/176#discussion_r298806534
##
File path: test/org/apache/catalina/connector/TestCoyoteAdapter.java
##
@@ -326,6 +326,7 @@ public boolean isResponseBodyOK() {
@Test
public void testNormalize01() {
doTestNormalize("/foo/../bar", "/bar");
+doTestNormalize("..", null);
}
Review comment:
We aim for one test per method. This would be a candidate for
`testNormalize02()`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #176: CoyoteAdapter: fix out-of-bounds read in checkNormalize
markt-asf commented on a change in pull request #176: CoyoteAdapter: fix
out-of-bounds read in checkNormalize
URL: https://github.com/apache/tomcat/pull/176#discussion_r298806436
##
File path: java/org/apache/catalina/connector/CoyoteAdapter.java
##
@@ -1252,6 +1252,11 @@ public static boolean checkNormalize(MessageBytes
uriMB) {
int pos = 0;
+// An empty URL is not acceptable
+if (start == end) {
+return false;
+}
+
// Check for '\' and 0
Review comment:
I believe the above return would represent unreachable code. Given this is
on the code path every request must pass through, we do not want to add any
unnecessary tests however trivial. If you can find an input to
`CoyoteAdapter.postParseRequest()` that would result in this code being
executed then that would likely be sufficient justification to add this test.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #176: CoyoteAdapter: fix out-of-bounds read in checkNormalize
markt-asf commented on a change in pull request #176: CoyoteAdapter: fix
out-of-bounds read in checkNormalize
URL: https://github.com/apache/tomcat/pull/176#discussion_r298806623
##
File path: test/org/apache/catalina/connector/TestCoyoteAdapter.java
##
@@ -344,6 +345,29 @@ private void doTestNormalize(String input, String
expected) {
}
}
+@Test
+public void testCheckNormalize() {
+doTestCheckNormalize("/url", true);
+
+doTestCheckNormalize("", false);
+doTestCheckNormalize("..", false);
+doTestCheckNormalize("/.", false);
+doTestCheckNormalize("/..", false);
+doTestCheckNormalize("/./", false);
+doTestCheckNormalize("//", false);
+doTestCheckNormalize("/../", false);
+doTestCheckNormalize("\\", false);
+doTestCheckNormalize("\0", false);
+}
Review comment:
One test per method here as well. `testCheckNormalize01()`,
`testCheckNormalize02()` etc.
While I am not against direct testing of any value that could be passed to
`checkNormalize()`, I think the test would be more useful if the appropriate
input was passed to `normalize()`, `convertURI()` and then `checkNormalize()`.
I appreciate `checkNormalize()` is a public method and could - in theory -
receive any input but in this instance the testing needs to focus on what is
possible given Tomcat's usage of the method.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #176: CoyoteAdapter: fix out-of-bounds read in checkNormalize
markt-asf commented on a change in pull request #176: CoyoteAdapter: fix
out-of-bounds read in checkNormalize
URL: https://github.com/apache/tomcat/pull/176#discussion_r298806528
##
File path: java/org/apache/catalina/connector/CoyoteAdapter.java
##
@@ -1271,6 +1276,11 @@ public static boolean checkNormalize(MessageBytes
uriMB) {
}
}
+// The URL must start with '/'
+if (c[start] != '/') {
+return false;
+}
+
// Check for ending with "/." or "/.."
Review comment:
The same goes for this test.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 63532] New: Wrong interpretation of EndPointConfig object life cycle and session.getOpenSession method in web socket
https://bz.apache.org/bugzilla/show_bug.cgi?id=63532 Bug ID: 63532 Summary: Wrong interpretation of EndPointConfig object life cycle and session.getOpenSession method in web socket Product: Tomcat 9 Version: unspecified Hardware: Other OS: Mac OS X 10.3 Status: NEW Severity: normal Priority: P2 Component: WebSocket Assignee: dev@tomcat.apache.org Reporter: sparshupadh...@gmail.com Target Milestone: - New EndpointConfig Object is being passed as a parameter in ServerEndpoint for each client request. As far as I know it should be Singleton and all instances of ServerEndpoint should share the same object. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
