Skip to content

Commit

Permalink
curl_httpclient,http1connection: Prohibit CR and LF in headers
Browse files Browse the repository at this point in the history
libcurl does not check for CR and LF in headers, making this the
application's responsibility. However, Tornado's other HTTP interfaces
check for linefeeds so we should do the same here so that switching
between the simple and curl http clients does not introduce header
injection vulnerabilties.

http1connection previously checked only for LF in headers (alone or in a
CRLF pair). It now prohibits bare CR as well, following the requirement
in RFC 9112.
  • Loading branch information
bdarnell committed Jun 5, 2024
1 parent 0efa9a4 commit b0ffc58
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 10 deletions.
20 changes: 12 additions & 8 deletions tornado/curl_httpclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import functools
import logging
import pycurl
import re
import threading
import time
from io import BytesIO
Expand All @@ -44,6 +45,8 @@

curl_log = logging.getLogger("tornado.curl_httpclient")

CR_OR_LF_RE = re.compile(b"\r|\n")


class CurlAsyncHTTPClient(AsyncHTTPClient):
def initialize( # type: ignore
Expand Down Expand Up @@ -347,14 +350,15 @@ def _curl_setup_request(
if "Pragma" not in request.headers:
request.headers["Pragma"] = ""

curl.setopt(
pycurl.HTTPHEADER,
[
b"%s: %s"
% (native_str(k).encode("ASCII"), native_str(v).encode("ISO8859-1"))
for k, v in request.headers.get_all()
],
)
encoded_headers = [
b"%s: %s"
% (native_str(k).encode("ASCII"), native_str(v).encode("ISO8859-1"))
for k, v in request.headers.get_all()
]
for line in encoded_headers:
if CR_OR_LF_RE.search(line):
raise ValueError("Illegal characters in header (CR or LF): %r" % line)
curl.setopt(pycurl.HTTPHEADER, encoded_headers)

curl.setopt(
pycurl.HEADERFUNCTION,
Expand Down
6 changes: 4 additions & 2 deletions tornado/http1connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@

from typing import cast, Optional, Type, Awaitable, Callable, Union, Tuple

CR_OR_LF_RE = re.compile(b"\r|\n")


class _QuietException(Exception):
def __init__(self) -> None:
Expand Down Expand Up @@ -453,8 +455,8 @@ def write_headers(
)
lines.extend(line.encode("latin1") for line in header_lines)
for line in lines:
if b"\n" in line:
raise ValueError("Newline in header: " + repr(line))
if CR_OR_LF_RE.search(line):
raise ValueError("Illegal characters (CR or LF) in header: %r" % line)
future = None
if self.stream.closed():
future = self._write_future = Future()
Expand Down
16 changes: 16 additions & 0 deletions tornado/test/httpclient_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,22 @@ def test_error_after_cancel(self):
if el.logged_stack:
break

def test_header_crlf(self):
# Ensure that the client doesn't allow CRLF injection in headers. RFC 9112 section 2.2
# prohibits a bare CR specifically and "a recipient MAY recognize a single LF as a line
# terminator" so we check each character separately as well as the (redundant) CRLF pair.
for header, name in [
("foo\rbar:", "cr"),
("foo\nbar:", "lf"),
("foo\r\nbar:", "crlf"),
]:
with self.subTest(name=name, position="value"):
with self.assertRaises(ValueError):
self.fetch("/hello", headers={"foo": header})
with self.subTest(name=name, position="key"):
with self.assertRaises(ValueError):
self.fetch("/hello", headers={header: "foo"})


class RequestProxyTest(unittest.TestCase):
def test_request_set(self):
Expand Down

0 comments on commit b0ffc58

Please sign in to comment.