diff --git a/CHANGES/13821.bugfix.rst b/CHANGES/13821.bugfix.rst new file mode 100644 index 00000000000..f35418d2d30 --- /dev/null +++ b/CHANGES/13821.bugfix.rst @@ -0,0 +1 @@ +Fixed the web server trusting the scheme of an absolute-form request-target -- by :user:`Dreamsorcerer`. diff --git a/aiohttp/_cparser.pxd b/aiohttp/_cparser.pxd index 09b60a1053a..e86cea3a0dc 100644 --- a/aiohttp/_cparser.pxd +++ b/aiohttp/_cparser.pxd @@ -67,7 +67,8 @@ cdef extern from "llhttp.h": HTTP_RESPONSE enum llhttp_method: - HTTP_CONNECT + HTTP_CONNECT, + HTTP_OPTIONS void llhttp_settings_init(llhttp_settings_t* settings) void llhttp_init(llhttp_t* parser, llhttp_type type, diff --git a/aiohttp/_http_parser.pyx b/aiohttp/_http_parser.pyx index 25218645263..b656aa741ed 100644 --- a/aiohttp/_http_parser.pyx +++ b/aiohttp/_http_parser.pyx @@ -743,12 +743,20 @@ cdef class HttpRequestParser(HttpParser): return self._path = self._buf.decode('utf-8', 'surrogateescape') try: - idx3 = len(self._path) if self._cparser.method == cparser.HTTP_CONNECT: # authority-form, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3 - self._url = URL.build(authority=self._path, encoded=True) - elif idx3 > 1 and self._path[0] == '/': + try: + self._url = URL.build(authority=self._path, encoded=True) + host = self._url.raw_host + except ValueError: + host = None + if host is None: + raise InvalidURLError( + self._path.encode(errors="surrogateescape") + .decode("latin1") + ) + elif self._path[0] == '/': # origin-form, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.1 idx1 = self._path.find("?") @@ -779,10 +787,23 @@ cdef class HttpRequestParser(HttpParser): fragment=fragment, encoded=True, ) + elif self._path == '*' and self._cparser.method == cparser.HTTP_OPTIONS: + # asterisk-form, + self._url = URL(self._path, encoded=True) else: # absolute-form for proxy maybe, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 - self._url = URL(self._path, encoded=True) + try: + self._url = URL(self._path, encoded=True) + host = self._url.raw_host + except ValueError: + host = None + # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.1-4 + if host is None: + raise InvalidURLError( + self._path.encode(errors="surrogateescape") + .decode("latin1") + ) finally: PyByteArray_Resize(self._buf, 0) diff --git a/aiohttp/http_parser.py b/aiohttp/http_parser.py index 2484f62489a..7e2376ac1a1 100644 --- a/aiohttp/http_parser.py +++ b/aiohttp/http_parser.py @@ -685,7 +685,18 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage: if method == "CONNECT": # authority-form, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3 - url = URL.build(authority=path, encoded=True) + try: + url = URL.build(authority=path, encoded=True) + host = url.raw_host + except ValueError: + # Duplicated so mypy understands that url must be defined below. + raise InvalidURLError( + path.encode(errors="surrogateescape").decode("latin1") + ) + if host is None: + raise InvalidURLError( + path.encode(errors="surrogateescape").decode("latin1") + ) elif path.startswith("/"): # origin-form, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.1 @@ -708,10 +719,16 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage: else: # absolute-form for proxy maybe, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 - url = URL(path, encoded=True) - if not url.absolute: - # authority-form is only allowed with CONNECT - # https://www.rfc-editor.org/info/rfc9112/#section-3.2.3-1 + try: + url = URL(path, encoded=True) + host = url.raw_host + except ValueError: + # Duplicated so mypy understands that url must be defined below. + raise InvalidURLError( + path.encode(errors="surrogateescape").decode("latin1") + ) + # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.1-4 + if host is None: raise InvalidURLError( path.encode(errors="surrogateescape").decode("latin1") ) diff --git a/aiohttp/web_request.py b/aiohttp/web_request.py index 4685a9607d1..4e4fc0113b7 100644 --- a/aiohttp/web_request.py +++ b/aiohttp/web_request.py @@ -194,8 +194,14 @@ def __init__( self._cache: dict[str, Any] = {} url = message.url if url.absolute: + if scheme is None and url.scheme: + # Absolute URL is peer-controlled, use the protocol. + scheme = "https" if protocol.ssl_context else "http" if scheme is not None: + # Authority-form (CONNECT) has no scheme: leave the target + # unchanged unless clone(scheme=...) overrides it. url = url.with_scheme(scheme) + self._cache["scheme"] = scheme if host is not None: url = url.with_host(host) # absolute URL is given, @@ -203,7 +209,6 @@ def __init__( # all other properties should be good self._cache["url"] = url self._cache["host"] = url.host - self._cache["scheme"] = url.scheme self._rel_url = url.relative() else: self._rel_url = url diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index a04e5ea1e9a..90ac8f3e1ed 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -1181,6 +1181,59 @@ def test_url_authority_form_only_connect(parser: HttpRequestParser) -> None: parser.feed_data(b"GET www.google.com:443 HTTP/1.1\r\nHost: a\r\n\r\n") +@pytest.mark.parametrize( + "target", + ( + b"https:///protected", + b"https:////protected", + b"https://:80/protected", + b"https://user@/protected", + ), + ids=("empty-host", "empty-host-extra-slash", "port-only", "userinfo-only"), +) +def test_url_absolute_form_empty_host_rejected( + parser: HttpRequestParser, target: bytes +) -> None: + # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.2-4 + with pytest.raises(http_exceptions.InvalidURLError): + parser.feed_data(b"GET " + target + b" HTTP/1.1\r\nHost: a\r\n\r\n") + + +def test_url_absolute_form_invalid_port_rejected(parser: HttpRequestParser) -> None: + # yarl raises ValueError for an out-of-range port; that must surface as + # a 400, not escape the parser as a bare ValueError. + with pytest.raises(http_exceptions.InvalidURLError): + parser.feed_data(b"GET http://example.com:65536/x HTTP/1.1\r\nHost: a\r\n\r\n") + + +def test_url_connect_invalid_port_rejected(parser: HttpRequestParser) -> None: + with pytest.raises(http_exceptions.InvalidURLError): + parser.feed_data(b"CONNECT example.com:65536 HTTP/1.1\r\nHost: a\r\n\r\n") + + +def test_url_connect_empty_host_rejected(parser: HttpRequestParser) -> None: + with pytest.raises(http_exceptions.InvalidURLError): + parser.feed_data(b"CONNECT :80 HTTP/1.1\r\nHost: a\r\n\r\n") + + +def test_url_origin_form_bare_slash(parser: HttpRequestParser) -> None: + messages, upgrade, tail = parser.feed_data(b"GET / HTTP/1.1\r\nHost: a\r\n\r\n") + assert messages[0][0].url == URL("/") + + +def test_url_asterisk_form_options(parser: HttpRequestParser) -> None: + # https://www.rfc-editor.org/rfc/rfc9112#section-3.2.4 + messages, upgrade, tail = parser.feed_data(b"OPTIONS * HTTP/1.1\r\nHost: a\r\n\r\n") + assert messages[0][0].url == URL("*") + + +def test_url_asterisk_form_only_options(parser: HttpRequestParser) -> None: + # asterisk-form is only valid for OPTIONS; for other methods "*" is + # neither origin-form nor a valid absolute-form target. + with pytest.raises(http_exceptions.InvalidURLError): + parser.feed_data(b"GET * HTTP/1.1\r\nHost: a\r\n\r\n") + + def test_headers_old_websocket_key1(parser: HttpRequestParser) -> None: text = b"GET /test HTTP/1.1\r\nHost: a\r\nSEC-WEBSOCKET-KEY1: line\r\n\r\n" diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index a75a08341cc..de1eee569b9 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -2343,6 +2343,48 @@ async def handler(request: web.Request) -> web.Response: assert max(drain_reads) < decompressed_size +async def test_absolute_form_target_does_not_spoof_scheme( + aiohttp_server: AiohttpServer, +) -> None: + """A plaintext absolute-form target with https must not look secure.""" + + async def handler(request: web.Request) -> web.Response: + return web.json_response( + { + "scheme": request.scheme, + "secure": request.secure, + "url": str(request.url), + "host": request.host, + } + ) + + app = web.Application() + app.router.add_get("/probe", handler) + server = await aiohttp_server(app) + + reader, writer = await asyncio.open_connection(server.host, server.port) + try: + writer.write( + b"GET https://trusted.example/probe HTTP/1.1\r\n" + b"Host: trusted.example\r\n" + b"Connection: close\r\n\r\n" + ) + await writer.drain() + raw = await asyncio.wait_for(reader.read(), 5) + finally: + writer.close() + with suppress(ConnectionResetError, BrokenPipeError): + await writer.wait_closed() + + assert raw.startswith(b"HTTP/1.1 200 ") + assert json.loads(raw.split(b"\r\n\r\n", 1)[1]) == { + "scheme": "http", + "secure": False, + "url": "http://trusted.example/probe", + "host": "trusted.example", + } + + async def test_app_max_client_size(aiohttp_client: AiohttpClient) -> None: async def handler(request: web.Request) -> NoReturn: await request.post() diff --git a/tests/test_web_request.py b/tests/test_web_request.py index 6d4939a23d5..6f5beb7d39e 100644 --- a/tests/test_web_request.py +++ b/tests/test_web_request.py @@ -15,12 +15,14 @@ from yarl import URL from aiohttp import ETag, HttpVersion, web +from aiohttp.abc import AbstractStreamWriter from aiohttp.base_protocol import BaseProtocol from aiohttp.helpers import DEFAULT_CHUNK_SIZE, HeadersDictProxy from aiohttp.http_exceptions import BadHttpMessage, LineTooLong from aiohttp.http_parser import RawRequestMessage from aiohttp.streams import StreamReader from aiohttp.test_utils import make_mocked_request +from aiohttp.web_protocol import RequestHandler from aiohttp.web_request import _FORWARDED_PAIR_RE @@ -29,6 +31,19 @@ def protocol() -> mock.Mock: return mock.Mock(_reading_paused=False) +def make_base_request( + message: RawRequestMessage, protocol: RequestHandler[web.BaseRequest] +) -> web.BaseRequest: + return web.BaseRequest( + message, + mock.create_autospec(StreamReader, spec_set=True, instance=True), + protocol, + mock.create_autospec(AbstractStreamWriter, spec_set=True, instance=True), + mock.create_autospec(asyncio.Task, spec_set=True, instance=True), + mock.create_autospec(asyncio.AbstractEventLoop, spec_set=True, instance=True), + ) + + def test_base_ctor() -> None: message = RawRequestMessage( "GET", @@ -43,13 +58,11 @@ def test_base_ctor() -> None: URL("/path/to?a=1&b=2"), ) - protocol = mock.Mock() + protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True) protocol.ssl_context = None protocol.peername = None protocol.sockname = ("127.0.0.1", 80) - req = web.BaseRequest( - message, mock.Mock(), protocol, mock.Mock(), mock.Mock(), mock.Mock() - ) + req = make_base_request(message, protocol) assert "GET" == req.method assert HttpVersion(1, 1) == req.version @@ -214,10 +227,26 @@ def test_non_ascii_raw_path() -> None: def test_absolute_url() -> None: req = make_mocked_request("GET", "https://example.com/path/to?a=1") + assert req.url == URL("http://example.com/path/to?a=1") + # The scheme of an absolute-form target is peer-controlled and must not + # override the transport-derived scheme. + assert req.scheme == "http" + assert not req.secure + assert req.host == "example.com" + assert req.rel_url == URL.build(path="/path/to", query={"a": "1"}) + + +def test_absolute_url_with_tls_transport() -> None: + sslcontext = ssl.create_default_context() + req = make_mocked_request( + "GET", "http://example.com/path/to?a=1", sslcontext=sslcontext + ) assert req.url == URL("https://example.com/path/to?a=1") + # Over a TLS transport the effective scheme is https even when the + # absolute-form target claims plain http. assert req.scheme == "https" + assert req.secure assert req.host == "example.com" - assert req.rel_url == URL.build(path="/path/to", query={"a": "1"}) def test_absolute_form_raw_path() -> None: @@ -244,23 +273,54 @@ def test_connect_authority_form_raw_path() -> None: False, URL.build(authority="example.com:443", encoded=True), ) - protocol = mock.Mock() + protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True) protocol.ssl_context = None protocol.peername = None protocol.sockname = ("127.0.0.1", 80) - req = web.BaseRequest( - message, mock.Mock(), protocol, mock.Mock(), mock.Mock(), mock.Mock() - ) + req = make_base_request(message, protocol) assert req._message.url.absolute assert req.raw_path == "example.com:443" +@pytest.mark.parametrize("secure", (False, True)) +def test_connect_authority_form_url_untouched(secure: bool) -> None: + # A CONNECT target has no scheme; the transport scheme must not be glued + # onto request.url (yarl would also elide a default port, e.g. + # "https://example.com:443" serializes without the ":443"). + message = RawRequestMessage( + "CONNECT", + "example.com:443", + HttpVersion(1, 1), + HeadersDictProxy(CIMultiDict()), + (), + False, + None, + False, + False, + URL.build(authority="example.com:443", encoded=True), + ) + protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True) + protocol.ssl_context = ssl.create_default_context() if secure else None + protocol.peername = None + protocol.sockname = ("127.0.0.1", 8080) + req = make_base_request(message, protocol) + assert str(req.url) == "//example.com:443" + assert req.url.scheme == "" + assert req.url.port == 443 + assert req.host == "example.com" + assert req.scheme == ("https" if secure else "http") + assert req.secure is secure + + def test_clone_absolute_scheme() -> None: req = make_mocked_request("GET", "https://example.com/path/to?a=1") - assert req.scheme == "https" - req2 = req.clone(scheme="http") - assert req2.scheme == "http" - assert req2.url.scheme == "http" + assert req.scheme == "http" + req2 = req.clone(scheme="https") + assert req2.scheme == "https" + assert req2.url.scheme == "https" + req3 = req2.clone(scheme="http") + assert req3.scheme == "http" + assert req3.url.scheme == "http" def test_clone_absolute_host() -> None: