Backwards incompatibility with header parsing from 3.8.4 to 3.8.5

[mdegat01]

Describe the bug

I received home-assistant/supervisor#4454 in my project and the root cause turned out to be the update of aiohttp from 3.8.4 to 3.8.5. I was able to clearly reproduce it in a vanilla aiohttp server set up from the example here and having a script make a call providing a Host header. The same script works when the server is running on 3.8.4 and fails if I update to 3.8.5.

To Reproduce

1. Install aiohttp 3.8.4 and run the demo server shown here
2. Use the following script to make a call to it and receive the hello world response as expected

#!/bin/bash

SERVER=127.0.0.1
nc -i 1 ${SERVER} 8080 <<EOF
GET /world HTTP/1.1
Host: 127.0.0.1

EOF

3. Stop the server, update to 3.8.5 and start it again
4. Run the same script and receive this response instead:

HTTP/1.0 400 Bad Request
Content-Type: text/plain; charset=utf-8
Content-Length: 69
Date: Wed, 02 Aug 2023 19:28:52 GMT
Server: Python/3.11 aiohttp/3.8.5

Invalid header value char:

  b'Host: 127.0.0.1'
                   ^

Expected behavior

The same script should work on 3.8.4 and 3.8.5

Logs/tracebacks

Error handling request
Traceback (most recent call last):
  File "/Users/degam/.pyenv/versions/3.11.2/lib/python3.11/site-packages/aiohttp/web_protocol.py", line 332, in data_received
    messages, upgraded, tail = self._request_parser.feed_data(data)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "aiohttp/_http_parser.pyx", line 557, in aiohttp._http_parser.HttpParser.feed_data
aiohttp.http_exceptions.BadHttpMessage: 400, message:
  Invalid header value char:

    b'Host: 127.0.0.1'
                     ^

Python Version

$ python --version
Python 3.11.2

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.8.5
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author:
Author-email:
License: Apache 2
Location: /Users/degam/.pyenv/versions/3.11.2/lib/python3.11/site-packages
Requires: aiosignal, async-timeout, attrs, charset-normalizer, frozenlist, multidict, yarl
Required-by:

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.0.4
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /Users/degam/.pyenv/versions/3.11.2/lib/python3.11/site-packages
Requires:
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /Users/degam/.pyenv/versions/3.11.2/lib/python3.11/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

macOS

Also have been able to reproduce it on a variety of systems running the Home Assistant software which depends on aiohttp, as described in the issue I linked at the top.

Related component

Server

Additional context

No response

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[webknjaz]

This is expected result of fixing a request smuggling security vulnerability. Newer RFCs are stricter regarding what should be a header separator. It can't be just CR or LF anymore and must always be a CR LF pair.

[webknjaz]

@Dreamsorcerer should that byte-string marker leak like this? It's probably confusing to people..

[mdegat01]

So the original script that caused the linked issue at the top of my post also included unix2dos like this:

#!/bin/bash

SERVER=127.0.0.1
nc -i 1 ${SERVER} 8080 <<< unix2dos<<EOF
GET /world HTTP/1.1
Host: 127.0.0.1

EOF

I stripped that out for this issue report since I was trying to keep the steps to reproduce as simple as possible. I was able to reproduce it even with that in there but upon further testing it looks like the underlying problem is really that unix2dos isn't working as the author expected in that script and the line endings remain incorrect.

So thanks for the explanation and in agreement, this is not an aiohttp issue. Although if considering improving the error message I would appreciate that. It was quite confusing, I wasn't sure what that marker was pointing to or trying to tell me as it appeared to be pointing to the single quote beyond the end of the header.

[Dreamsorcerer]

I'm pretty sure in testing, I saw errors with \r in the output. I'll have to test this one out later and see why it's not including the bad character in the output...

[Dreamsorcerer]

OK, it's because I'm splitting on \n, to restrict it to the current line, but if the \n is the error then it's just outside where I split the data. I could leave the full data in the output, but that'd be pretty messy, or I could just readd the \n to the end of the string after splitting.

[Dreamsorcerer]

Actually, I can just split it on \r\n, which should then match the parser's logic.

[webknjaz]

I wonder if we could use https://docs.python.org/3/library/unicodedata.html#unicodedata.name for anything non-ASCII in the error representation.
It is possible to reference symbols by names like '\N{CARRIAGE RETURN}\N{LINE FEED}', so why not do a reverse? And we wouldn't have to keep it as a bytestring...
@Dreamsorcerer WDYT?

[Dreamsorcerer]

It is possible to reference symbols by names like '\N{CARRIAGE RETURN}\N{LINE FEED}', so why not do a reverse? And we wouldn't have to keep it as a bytestring...

Seems like it doesn't really work for any of the characters we'd particularly want it for (it also requires str, which means we need to encode it first and it might not be valid utf-8):

>>> unicodedata.name("\r")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: no such name
>>> unicodedata.name("\n")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: no such name
>>> unicodedata.name("\x01")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: no such name

It'd also become more complex to figure out the pointer positioning. I think bytes is pretty clear, a developer can look up the characters themselves if they are unfamiliar with it.

[webknjaz]

@Dreamsorcerer true. Though, it'd be nice to have something like this conceptually, maybe not through unicode but with code-points for everything non-ASCII.. I was wondering whether to task one of my mentees with this research, it doesn't seem too complicated.

[Dreamsorcerer]

Up to you, but I'm not sure it adds much, and there are a bunch of other tasks I think would be more useful to look at.

[webknjaz]

Maybe we could just do repr(bytes_thing)[2:-1] to get rid of the bytes-wrapper?

[Dreamsorcerer]

Yeah, if you think that's preferable. I was just thinking it would be clear what format it is being displayed in, so a user knows that \x01 is not 4 literal characters, for example.