fix: reject conflicting content-length headers#1317
Conversation
| if n == b"content-length": | ||
| try: | ||
| self._expected_content_length = int(v, 10) | ||
| content_lengths.append(int(v, 10)) |
There was a problem hiding this comment.
Could this lead to a DoS if a malicous payload keeps sending new differing content-length headers and this list keeps growing?
There was a problem hiding this comment.
No, I don't think so, it'll be discarded once _initialize_content_length() exits, but I think I have a better idea where only comparing the lengths can work as well, lemme push a fix
|
asking for a second opinion from @mhils: |
We reject them, because it has tremendous request smuggling potential. |
| if content_length is None: | ||
| content_length = parsed_content_length | ||
| elif parsed_content_length != content_length: | ||
| msg = "Conflicting content-length headers" |
There was a problem hiding this comment.
Please add both values into the error message here.
| if content_length is None: | ||
| return | ||
|
|
||
| self._expected_content_length = content_length |
There was a problem hiding this comment.
Could be simplified here: if content_length is None, then just assign it anyway, as _expected_content_length is going to be also None.
|
@Harshal96 please add a changelog entry to document the new behaviour - this is breaking change, as the previous code paths silently accepted the first found content-length header (ignoring all others), while the new behaviour will raise a ProtocolError if there is a second header with a different value. I am still considering if such a compliance check should better be part of the existing checks in |
|
@Kriechi the check is tied to stream body accounting, so maybe should not be a part of |
3 participants
Summary
content-lengthheader fields before setting the expected body lengthcontent-lengthfields when their parsed decimal values differFixes #1316
Tests