Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject HTTP message with duplicate Content-Length header fields #637

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dennis-tseng99
Copy link
Contributor

If multiple headers occur, usually the last header would have authority; however the section 3.3.2 of RFC 7230 states that:

If a message is received that has multiple Content-Length header fields with field-values consisting of the same decimal value, ....., then the recipient MUST either reject the message as invalid.

@mikebeaton
Copy link
Contributor

If it's okay to jump in, this:

"... or replace the duplicated field-values with a single valid Content-Length field containing that decimal value prior to determining the message body length or forwarding the message."

seems relevant too; and AFAICT implies that, at least in the case where the multiple fields all have the same value, this can be treated as one field of that value. So, can be accepted. Your suggestion (again, if I'm not misreading) seems to reject the message only if at least one content-length value is the duplicated, but accept the last one if they are all different. The RFC doesn't even seem to cover the case of more than one content-length value, where the values are different. So the existing code using the last received size (if there are multiple) actually looks correct for the case the RFC mentions, and acceptable otherwise.

@dennis-tseng99
Copy link
Contributor Author

Thank @mikebeaton.

in the case where the multiple fields all have the same value, this can be treated as one field of that value. So, can be accepted.

Unfortunately, our httpboot.c doesn't deal with one header with multiple field values, like "Content-Length: 42, 42, 42, ....". Our httpboot.c assumes there is only one field value behind the Content-Length header, like "Content-Length: 42". So it cannot be accepted.

Your suggestion (again, if I'm not misreading) seems to reject the message only if at least one content-length value is the duplicated,

Yes, according to RFC:
"If a message is received that has multiple Content-Length header
fields with field-values consisting of the same decimal value"

but accept the last one if they are all different.

If there are 2 headers, for example, "Content-Length: 42" and "Content-Length: 52", then our shim httpboot.c will accept the last one which is "Content-Length": 52". My patch didn't modify it. Is this similar to CVE-2021-40346 ?

The RFC doesn't even seem to cover the case of more than one content-length value, where the values are different.

Actually, it covers:
"If a message is received without Transfer-Encoding and with
either multiple Content-Length header fields having differing
field-values or or a single Content-Length header field having an
invalid value, then the message framing is invalid and the
recipient MUST treat it as an unrecoverable error."

My patch didn't cover it, either. Maybe my patch should reject all messages with multiple "Content-Length" headers.

@mikebeaton
Copy link
Contributor

Actually, it covers:
"If a message is received without Transfer-Encoding and with
either multiple Content-Length header fields having differing
field-values or or a single Content-Length header field having an
invalid value, then the message framing is invalid and the
recipient MUST treat it as an unrecoverable error."

My patch didn't cover it, either. Maybe my patch should reject all messages with multiple "Content-Length" headers.

That idea does seem more consistent. But the alternative of allowing multiple values if they are the same, but rejecting if any different value is found still also seems correct to me. (Unlike your current suggestion, if I haven't misread it, of rejecting if any duplicate value is found.)

@dennis-tseng99
Copy link
Contributor Author

Thank suggestion from @mikebeaton. Changed to:
Reject message with different values in multiple Content-Length header fields.

@mikebeaton
Copy link
Contributor

Thank you, that seems like a reasonable, valid update to me - FWIW! - and it indeed appears to be required by the parts of the HTTP standard that you've linked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants