From e16c2e9edbdfc350c10f44af7787ce0a71c010d8 Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Fri, 29 Jan 2021 17:53:06 +0100 Subject: [PATCH 1/8] Add validation of HTTP status line, header keys and values --- CHANGES/4818.feature | 1 + CONTRIBUTORS.txt | 1 + aiohttp/_http_writer.pyx | 13 +++++++++++++ aiohttp/http_writer.py | 19 +++++++++++++++++-- tests/test_http_writer.py | 18 ++++++++++++++++++ tests/test_web_response.py | 8 ++------ 6 files changed, 52 insertions(+), 8 deletions(-) create mode 100644 CHANGES/4818.feature diff --git a/CHANGES/4818.feature b/CHANGES/4818.feature new file mode 100644 index 00000000000..1279b5457c8 --- /dev/null +++ b/CHANGES/4818.feature @@ -0,0 +1 @@ +Add validation of HTTP status line, header keys and header values to prevent header injection. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 20f110dea92..489f7324989 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -113,6 +113,7 @@ Felix Yan Fernanda Guimarães FichteFoll Florian Scheffler +Franciszek Magiera Frederik Gladhorn Frederik Peter Aalund Gabriel Tremblay diff --git a/aiohttp/_http_writer.pyx b/aiohttp/_http_writer.pyx index 84b42fa1c35..37b5fa183b0 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 _check_string(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,11 @@ def _serialize_headers(str status_line, headers): _init_writer(&writer) + _check_string(status_line) + for key, val in headers.items(): + _check_string(to_str(key)) + _check_string(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..e7552183d9b 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -171,11 +171,26 @@ async def drain(self) -> None: await self._protocol._drain_helper() +def _check_string(string: str) -> str: + 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." + ) + else: + return string + + def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: line = ( - status_line + _check_string(status_line) + "\r\n" - + "".join([k + ": " + v + "\r\n" for k, v in headers.items()]) + + "".join( + [ + _check_string(k) + ": " + _check_string(v) + "\r\n" + for k, v in headers.items() + ] + ) ) return line.encode("utf-8") + b"\r\n" diff --git a/tests/test_http_writer.py b/tests/test_http_writer.py index ab62ffc31b5..07d3522d8e0 100644 --- a/tests/test_http_writer.py +++ b/tests/test_http_writer.py @@ -5,6 +5,7 @@ from unittest import mock import pytest +from multidict import CIMultiDict from aiohttp import http from aiohttp.test_utils import make_mocked_coro @@ -272,3 +273,20 @@ async def test_drain_no_transport(protocol: Any, transport: Any, loop: Any) -> N msg._protocol.transport = None await msg.drain() assert not protocol._drain_helper.called + + +async def test_write_headers_prevents_injection( + protocol: Any, transport: Any, loop: Any +) -> None: + msg = http.StreamWriter(protocol, loop) + wrong_status_line = "HTTP/1.1 200 OK\r\nSet-Cookie: abc=123" + headers = CIMultiDict({"Content-Length": "256"}) + with pytest.raises(ValueError): + await msg.write_headers(wrong_status_line, headers) + 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 a80184029a5..659aa1b9b41 100644 --- a/tests/test_web_response.py +++ b/tests/test_web_response.py @@ -14,6 +14,7 @@ from re_assert import Matches from aiohttp import HttpVersion, HttpVersion10, HttpVersion11, hdrs +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 @@ -58,12 +59,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""): From 7ae98362dd4be37f11805a0ba6cc8905e28d18a8 Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Sun, 15 Aug 2021 20:18:20 +0200 Subject: [PATCH 2/8] Apply review comments --- CONTRIBUTORS.txt | 2 +- aiohttp/http_writer.py | 17 ++++------------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 489f7324989..05aa13c9685 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -113,7 +113,7 @@ Felix Yan Fernanda Guimarães FichteFoll Florian Scheffler -Franciszek Magiera +Franek Magiera Frederik Gladhorn Frederik Peter Aalund Gabriel Tremblay diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index e7552183d9b..3fe7cff37f6 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -177,22 +177,13 @@ def _check_string(string: str) -> str: "Newline or carriage return character detected in HTTP status message or " "header. This is a potential security issue." ) - else: - return string + return string def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - line = ( - _check_string(status_line) - + "\r\n" - + "".join( - [ - _check_string(k) + ": " + _check_string(v) + "\r\n" - for k, v in headers.items() - ] - ) - ) - return line.encode("utf-8") + b"\r\n" + headers = (_check_string(k) + ": " + _check_string(v) for k, v in headers.items()) + line = _check_string(status_line) + "\r\n" + "\r\n".join(headers) + "\r\n" + return line.encode("utf-8") _serialize_headers = _py_serialize_headers From 9c80465931ea50ffb5aef326f1c61d24e7bb8a0b Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Mon, 16 Aug 2021 20:01:53 +0200 Subject: [PATCH 3/8] Rename _check_string to _safe_header and remove validation for the status_line --- aiohttp/_http_writer.pyx | 7 +++---- aiohttp/http_writer.py | 6 +++--- tests/test_http_writer.py | 4 ---- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/aiohttp/_http_writer.pyx b/aiohttp/_http_writer.pyx index 37b5fa183b0..eff85219586 100644 --- a/aiohttp/_http_writer.pyx +++ b/aiohttp/_http_writer.pyx @@ -111,7 +111,7 @@ cdef str to_str(object s): return str(s) -cdef void _check_string(str string) except *: +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 " @@ -127,10 +127,9 @@ def _serialize_headers(str status_line, headers): _init_writer(&writer) - _check_string(status_line) for key, val in headers.items(): - _check_string(to_str(key)) - _check_string(to_str(val)) + _safe_header(to_str(key)) + _safe_header(to_str(val)) try: if _write_str(&writer, status_line) < 0: diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index 3fe7cff37f6..29830585342 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -171,7 +171,7 @@ async def drain(self) -> None: await self._protocol._drain_helper() -def _check_string(string: str) -> str: +def _safe_header(string: str) -> str: if "\r" in string or "\n" in string: raise ValueError( "Newline or carriage return character detected in HTTP status message or " @@ -181,8 +181,8 @@ def _check_string(string: str) -> str: def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - headers = (_check_string(k) + ": " + _check_string(v) for k, v in headers.items()) - line = _check_string(status_line) + "\r\n" + "\r\n".join(headers) + "\r\n" + headers = (_safe_header(k) + ": " + _safe_header(v) for k, v in headers.items()) + line = status_line + "\r\n" + "\r\n".join(headers) + "\r\n" return line.encode("utf-8") diff --git a/tests/test_http_writer.py b/tests/test_http_writer.py index 07d3522d8e0..3fb5531ca1d 100644 --- a/tests/test_http_writer.py +++ b/tests/test_http_writer.py @@ -279,10 +279,6 @@ async def test_write_headers_prevents_injection( protocol: Any, transport: Any, loop: Any ) -> None: msg = http.StreamWriter(protocol, loop) - wrong_status_line = "HTTP/1.1 200 OK\r\nSet-Cookie: abc=123" - headers = CIMultiDict({"Content-Length": "256"}) - with pytest.raises(ValueError): - await msg.write_headers(wrong_status_line, headers) status_line = "HTTP/1.1 200 OK" wrong_headers = CIMultiDict({"Set-Cookie: abc=123\r\nContent-Length": "256"}) with pytest.raises(ValueError): From f9770f4225ec792993b60553cb0f7cbe9d4656f2 Mon Sep 17 00:00:00 2001 From: Franek Magiera Date: Mon, 16 Aug 2021 21:07:45 +0200 Subject: [PATCH 4/8] Update aiohttp/http_writer.py Co-authored-by: Sam Bull --- aiohttp/http_writer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index 29830585342..3b7a1a35400 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -181,8 +181,8 @@ def _safe_header(string: str) -> str: def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - headers = (_safe_header(k) + ": " + _safe_header(v) for k, v in headers.items()) - line = status_line + "\r\n" + "\r\n".join(headers) + "\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" return line.encode("utf-8") From 691f20daee14a0af6fdd6f9183d9efb0bcf6ce12 Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Mon, 16 Aug 2021 23:10:57 +0200 Subject: [PATCH 5/8] Modify changelog message --- CHANGES/4818.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/4818.feature b/CHANGES/4818.feature index 1279b5457c8..158e4ebae84 100644 --- a/CHANGES/4818.feature +++ b/CHANGES/4818.feature @@ -1 +1 @@ -Add validation of HTTP status line, header keys and header values to prevent header injection. +Add validation of HTTP header keys and values to prevent header injection. From 4d2eb7616c89dd48b2002da635148d6cac387207 Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Mon, 16 Aug 2021 23:45:32 +0200 Subject: [PATCH 6/8] Refactor headers join --- aiohttp/http_writer.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index 3b7a1a35400..509bad8e9a3 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -181,9 +181,11 @@ def _safe_header(string: str) -> str: def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - 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" - return line.encode("utf-8") + headers_strings = [ + _safe_header(k) + ": " + _safe_header(v) + "\r\n" for k, v in headers.items() + ] + line = status_line + "\r\n" + "".join(headers_strings) + return line.encode("utf-8") + b"\r\n" _serialize_headers = _py_serialize_headers From ce3dc76d0ff3f13537edf922ba85350199dd51a9 Mon Sep 17 00:00:00 2001 From: franekmagiera Date: Tue, 17 Aug 2021 00:04:18 +0200 Subject: [PATCH 7/8] Refactor headers serialization back to the broken down version and add the second CRLF sign after the headers --- aiohttp/http_writer.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index 509bad8e9a3..61886aa7785 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -181,11 +181,9 @@ def _safe_header(string: str) -> str: def _py_serialize_headers(status_line: str, headers: "CIMultiDict[str]") -> bytes: - headers_strings = [ - _safe_header(k) + ": " + _safe_header(v) + "\r\n" for k, v in headers.items() - ] - line = status_line + "\r\n" + "".join(headers_strings) - 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 From 66ec8d5c6d43dc03d5981e7dd3c95ced61172809 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 4 Sep 2021 16:41:53 +0100 Subject: [PATCH 8/8] Update aiohttp/http_writer.py --- aiohttp/http_writer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py index 61886aa7785..428a7929b1a 100644 --- a/aiohttp/http_writer.py +++ b/aiohttp/http_writer.py @@ -174,8 +174,8 @@ async def drain(self) -> None: def _safe_header(string: str) -> str: 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." + "Newline or carriage return detected in headers. " + "Potential header injection attack." ) return string