Repository navigation
RFC 9112: reject signed chunk sizes and Content-Length values - #729
Conversation
|
@arturobernalg I am strongly not in favor of this change. It adds an extra full scan over chunk length element. Either be pragmatic and test for leading char not being |
b2ddb51 to
e5d7861
Compare
@ok2c agreed. Updated. Removed the extra scan and added a leading-character check as suggested. The existing parsing and overflow handling remain unchanged. Please do another pass |
| final Header contentLengthHeader = message.getFirstHeader(HttpHeaders.CONTENT_LENGTH); | ||
| if (contentLengthHeader != null) { | ||
| final String s = contentLengthHeader.getValue(); | ||
| if (s != null && !s.isEmpty() && (s.charAt(0) == '+' || s.charAt(0) == '-')) { |
There was a problem hiding this comment.
s != null && !s.isEmpty() && (s.charAt(0) == '+' || s.charAt(0) == '-')
@arturobernalg Should we move this predicate to a support method in MessageSupport?
Long#parseLong accepts a leading '+' or '-'. The NIO chunk decoder also treated '-1' as 'no chunk head yet' and skipped the line, and failed with IllegalArgumentException on other negative values. Chunk sizes must be 1*HEXDIG and Content-Length 1*DIGIT. Reject an empty value and a leading sign before parsing, in both the blocking and non-blocking code paths.
e5d7861 to
746d4ca
Compare
Long#parseLongaccepts a leading+or-, which the grammar does not allow (chunk-size is1*HEXDIG, Content-Length is1*DIGIT).On master:
ChunkedInputStreamaccepts+5ChunkDecoderaccepts+5, silently skips a-1chunk head (it equals the internal "no chunk head" marker) and fails withIllegalArgumentExceptionon other negative valuesMessageSupport#getContentLengthaccepts+5All three now reject such values with
MalformedChunkCodingException/ProtocolException. Leading zeros are still accepted. Tests added for the blocking and non-blocking paths.