From 3cae27ac9232b5b86b8837a9f540b8fc574489a0 Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Sat, 4 Sep 2021 16:14:04 +0000 Subject: [PATCH] Add validation of HTTP status line, header keys and values (#5452) (#5984) * Add validation of HTTP status line, header keys and values * Apply review comments * Rename _check_string to _safe_header and remove validation for the status_line * Update aiohttp/http_writer.py Co-authored-by: Sam Bull * Modify changelog message * Refactor headers join * Refactor headers serialization back to the broken down version and add the second CRLF sign after the headers * Update aiohttp/http_writer.py Co-authored-by: Sam Bull (cherry picked from commit a1158c5389854f9885c30e08b0020e2d7d8a290f) Co-authored-by: Franek Magiera --- CHANGES/4818.feature | 1 + CONTRIBUTORS.txt | 1 + aiohttp/_http_writer.pyx | 12 ++++++++++++ aiohttp/http_writer.py | 18 ++++++++++++------ tests/test_http_writer.py | 12 ++++++++++++ tests/test_web_response.py | 8 ++------ 6 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 CHANGES/4818.feature diff --git a/CHANGES/4818.feature b/CHANGES/4818.feature new file mode 100644 index 00000000000..158e4ebae84 --- /dev/null +++ b/CHANGES/4818.feature @@ -0,0 +1 @@ +Add validation of HTTP header keys and values to prevent header injection. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index ad58b745919..366e559e536 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -111,6 +111,7 @@ Felix Yan Fernanda Guimarães FichteFoll Florian Scheffler +Franek Magiera Frederik Gladhorn Frederik Peter Aalund Gabriel Tremblay diff --git a/aiohttp/_http_writer.pyx b/aiohttp/_http_writer.pyx index 84b42fa1c35..eff85219586 100644 --- a/aiohttp/_http_writer.pyx +++ b/aiohttp/_http_writer.pyx @@ -111,6 +111,14 @@ cdef str to_str(object s): return str(s) +cdef void _safe_header(str string) except *: + if "\r" in string or "\n" in string: + raise ValueError( + "Newline or carriage return character detected in HTTP status message or " + "header. This is a potential security issue." + ) + + def _serialize_headers(str status_line, headers): cdef Writer writer cdef object key @@ -119,6 +127,10 @@ def _serialize_headers(str status_line, headers): _init_writer(&writer) + for key, val in headers.items(): + _safe_header(to_str(key)) + _safe_header(to_str(val)) + try: if _write_str(&writer, status_line) < 0: raise diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index f859790efdd..428a7929b1a 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -171,13 +171,19 @@ async def drain(self) -> None: await self._protocol._drain_helper() +def _safe_header(string: str) -> str: + if "\r" in string or "\n" in string: + raise ValueError( + "Newline or carriage return detected in headers. " + "Potential header injection attack." + ) + return string + + def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - line = ( - status_line - + "\r\n" - + "".join([k + ": " + v + "\r\n" for k, v in headers.items()]) - ) - return line.encode("utf-8") + b"\r\n" + headers_gen = (_safe_header(k) + ": " + _safe_header(v) for k, v in headers.items()) + line = status_line + "\r\n" + "\r\n".join(headers_gen) + "\r\n\r\n" + return line.encode("utf-8") _serialize_headers = _py_serialize_headers diff --git a/tests/test_http_writer.py b/tests/test_http_writer.py index 6aca2ea2d9a..8ebcfc654a5 100644 --- a/tests/test_http_writer.py +++ b/tests/test_http_writer.py @@ -3,6 +3,7 @@ from unittest import mock import pytest +from multidict import CIMultiDict from aiohttp import http from aiohttp.test_utils import make_mocked_coro @@ -246,3 +247,14 @@ async def test_drain_no_transport(protocol, transport, loop) -> None: msg._protocol.transport = None await msg.drain() assert not protocol._drain_helper.called + + +async def test_write_headers_prevents_injection(protocol, transport, loop) -> None: + msg = http.StreamWriter(protocol, loop) + status_line = "HTTP/1.1 200 OK" + wrong_headers = CIMultiDict({"Set-Cookie: abc=123\r\nContent-Length": "256"}) + with pytest.raises(ValueError): + await msg.write_headers(status_line, wrong_headers) + wrong_headers = CIMultiDict({"Content-Length": "256\r\nSet-Cookie: abc=123"}) + with pytest.raises(ValueError): + await msg.write_headers(status_line, wrong_headers) diff --git a/tests/test_web_response.py b/tests/test_web_response.py index 4d4b63ca4d8..583f076dc90 100644 --- a/tests/test_web_response.py +++ b/tests/test_web_response.py @@ -12,6 +12,7 @@ from aiohttp import HttpVersion, HttpVersion10, HttpVersion11, hdrs from aiohttp.helpers import ETag +from aiohttp.http_writer import _serialize_headers from aiohttp.payload import BytesPayload from aiohttp.test_utils import make_mocked_coro, make_mocked_request from aiohttp.web import ContentCoding, Response, StreamResponse, json_response @@ -56,12 +57,7 @@ def write(chunk): buf.extend(chunk) async def write_headers(status_line, headers): - headers = ( - status_line - + "\r\n" - + "".join([k + ": " + v + "\r\n" for k, v in headers.items()]) - ) - headers = headers.encode("utf-8") + b"\r\n" + headers = _serialize_headers(status_line, headers) buf.extend(headers) async def write_eof(chunk=b""):