Various cleanups in HttpParser (CVE-2023-40167) #10329
Conversation
Various cleanups in HttpParser Signed-off-by: gregw <gregw@webtide.com>
Various cleanups in HttpParser Signed-off-by: gregw <gregw@webtide.com>
jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTPTest.java
Outdated
Show resolved
Hide resolved
Various cleanups in HttpParser Signed-off-by: gregw <gregw@webtide.com>
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
| if (c < '0' || c > '9') | ||
| throw new BadMessageException("Invalid Content-Length Value", new NumberFormatException()); | ||
|
|
||
| value = Math.addExact(Math.multiplyExact(value, 10), c - '0'); |
There was a problem hiding this comment.
addExact and multiplyExact throw ArithmeticException, that we should catch and convert to BadMessageException.
There was a problem hiding this comment.
It is already caught in parseNext and converted to a BadMessageException there. We don't need to catch inside a catch
There was a problem hiding this comment.
@gregw before, NumberFormatException was caught here and converted, that's why I raised this comment.
Also, we can be more precise with the exception message saying "invalid content-length", rather than just an ArithmeticException with no message.
I'd prefer to catch and rethrow, but I can live as is.
There was a problem hiding this comment.
@sbordet I'd prefer to avoid the tiny cost of an extra try catch in critical path code to give a slightly better error message to a bad client.
update after review Signed-off-by: gregw <gregw@webtide.com>
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
| if (c < '0' || c > '9') | ||
| throw new BadMessageException("Invalid Content-Length Value", new NumberFormatException()); | ||
|
|
||
| value = Math.addExact(Math.multiplyExact(value, 10), c - '0'); |
There was a problem hiding this comment.
@gregw before, NumberFormatException was caught here and converted, that's why I raised this comment.
Also, we can be more precise with the exception message saying "invalid content-length", rather than just an ArithmeticException with no message.
I'd prefer to catch and rethrow, but I can live as is.
crime against formatting resolved Signed-off-by: gregw <gregw@webtide.com>
joakime
changed the title
Various cleanups in HttpParser: