You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "maskit (via GitHub)" <gi...@apache.org> on 2024/02/13 16:31:37 UTC
[PR] Fix a bug in parsing chunked messages [trafficserver]
maskit opened a new pull request, #11073:
URL: https://github.com/apache/trafficserver/pull/11073
This is for #10477 and #10580.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1503445686
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
Just making sure I understand: why is `;` an expected character? I don't ever recall seeing a `;` after the chunk header size.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1503740433
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
The semicolon is for `chunk-ext`.
```
chunk = chunk-size [ chunk-ext ] CRLF
chunk-data CRLF
chunk-ext = *( BWS ";" BWS chunk-ext-name
[ BWS "=" BWS chunk-ext-val ] )
```
https://www.rfc-editor.org/rfc/rfc9112#name-chunk-extensions
`BWS` is Bad WhiteSpace, and it's basically optional whitespace.
https://www.rfc-editor.org/rfc/rfc9110.html#section-5.6.3-5
As for LF, I forgot to loosen the rule here. I pushed a commit to allow LF.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#issuecomment-2033188655
Cherry-picked to v9.2.x
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1488555510
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
Taking a quick look at this, it seems like it would accept "100 junk \n" as the chunk size of a 256 byte chunk.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1505364666
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
Sounds good. Thank you for the explanation.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1488633783
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
Yeah, a malformed chunk extension would pass the check. I ignored almost everything that follows a chunk size, but it should not allow reading random data as a chunk size.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#issuecomment-1966901126
[approve ci autest]
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#issuecomment-1942048560
[approve ci autest]
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#discussion_r1503453995
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -157,8 +157,9 @@ ChunkedHandler::read_size()
}
} else {
// We are done parsing size
- if (num_digits == 0 || running_sum < 0) {
- // Bogus chunk size
+ if ((num_digits == 0 || running_sum < 0) || /* Bogus chunk size */
+ (!ParseRules::is_cr(*tmp) && !ParseRules::is_ws(*tmp) && *tmp != ';') /* Unexpected character */
+ ) {
Review Comment:
Oh, should we also not be surprised by a LF? We seem to be lenient on that elsewhere in this loop.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a bug in parsing chunked messages [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #11073:
URL: https://github.com/apache/trafficserver/pull/11073#issuecomment-1973317873
Cherry-picked to v10.0.x
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org