1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
|
From 1e6e9d23cac59689760558dcb6634285e694b04c Mon Sep 17 00:00:00 2001
From: Glyph <glyph@twistedmatrix.com>
Date: Tue Sep 12 11:32:55 2023 -0700
Subject: [PATCH] 11976 stop processing pipelined HTTP/1.1 requests that are
received together (#11979)
CVE: CVE-2023-46137
Upstream-Status: Backport [https://github.com/twisted/twisted/commit/1e6e9d23cac59689760558dcb6634285e694b04c]
Signed-off-by: Soumya Sambu <soumya.sambu@windriver.com>
---
src/twisted/web/http.py | 32 +++++++--
src/twisted/web/newsfragments/11976.bugfix | 7 ++
src/twisted/web/test/test_web.py | 81 +++++++++++++++++++++-
3 files changed, 114 insertions(+), 6 deletions(-)
create mode 100644 src/twisted/web/newsfragments/11976.bugfix
diff --git a/src/twisted/web/http.py b/src/twisted/web/http.py
index 96a1335..b99480f 100644
--- a/src/twisted/web/http.py
+++ b/src/twisted/web/http.py
@@ -2366,14 +2366,38 @@ class HTTPChannel(basic.LineReceiver, policies.TimeoutMixin):
self._handlingRequest = True
+ # We go into raw mode here even though we will be receiving lines next
+ # in the protocol; however, this data will be buffered and then passed
+ # back to line mode in the setLineMode call in requestDone.
+ self.setRawMode()
+
req = self.requests[-1]
req.requestReceived(command, path, version)
- def dataReceived(self, data):
+ def rawDataReceived(self, data: bytes) -> None:
"""
- Data was received from the network. Process it.
+ This is called when this HTTP/1.1 parser is in raw mode rather than
+ line mode.
+
+ It may be in raw mode for one of two reasons:
+
+ 1. All the headers of a request have been received and this
+ L{HTTPChannel} is currently receiving its body.
+
+ 2. The full content of a request has been received and is currently
+ being processed asynchronously, and this L{HTTPChannel} is
+ buffering the data of all subsequent requests to be parsed
+ later.
+
+ In the second state, the data will be played back later.
+
+ @note: This isn't really a public API, and should be invoked only by
+ L{LineReceiver}'s line parsing logic. If you wish to drive an
+ L{HTTPChannel} from a custom data source, call C{dataReceived} on
+ it directly.
+
+ @see: L{LineReceive.rawDataReceived}
"""
- # If we're currently handling a request, buffer this data.
if self._handlingRequest:
self._dataBuffer.append(data)
if (
@@ -2385,9 +2409,7 @@ class HTTPChannel(basic.LineReceiver, policies.TimeoutMixin):
# ready. See docstring for _optimisticEagerReadSize above.
self._networkProducer.pauseProducing()
return
- return basic.LineReceiver.dataReceived(self, data)
- def rawDataReceived(self, data):
self.resetTimeout()
try:
diff --git a/src/twisted/web/newsfragments/11976.bugfix b/src/twisted/web/newsfragments/11976.bugfix
new file mode 100644
index 0000000..8ac292b
--- /dev/null
+++ b/src/twisted/web/newsfragments/11976.bugfix
@@ -0,0 +1,7 @@
+In Twisted 16.3.0, we changed twisted.web to stop dispatching HTTP/1.1
+pipelined requests to application code. There was a bug in this change which
+still allowed clients which could send multiple full HTTP requests in a single
+TCP segment to trigger asynchronous processing of later requests, which could
+lead to out-of-order responses. This has now been corrected and twisted.web
+should never process a pipelined request over HTTP/1.1 until the previous
+request has fully completed.
diff --git a/src/twisted/web/test/test_web.py b/src/twisted/web/test/test_web.py
index 3eb35a9..b2b2ad7 100644
--- a/src/twisted/web/test/test_web.py
+++ b/src/twisted/web/test/test_web.py
@@ -8,6 +8,7 @@ Tests for various parts of L{twisted.web}.
import os
import zlib
from io import BytesIO
+from typing import List
from zope.interface import implementer
from zope.interface.verify import verifyObject
@@ -17,10 +18,13 @@ from twisted.internet.address import IPv4Address, IPv6Address
from twisted.internet.task import Clock
from twisted.logger import LogLevel, globalLogPublisher
from twisted.python import failure, reflect
+from twisted.python.compat import iterbytes
from twisted.python.filepath import FilePath
-from twisted.test.proto_helpers import EventLoggingObserver
+from twisted.test.proto_helpers import EventLoggingObserver, StringTransport
from twisted.trial import unittest
from twisted.web import error, http, iweb, resource, server
+from twisted.web.resource import Resource
+from twisted.web.server import NOT_DONE_YET, Request, Site
from twisted.web.static import Data
from twisted.web.test.requesthelper import DummyChannel, DummyRequest
from ._util import assertIsFilesystemTemporary
@@ -1849,3 +1853,78 @@ class ExplicitHTTPFactoryReactor(unittest.TestCase):
factory = http.HTTPFactory()
self.assertIs(factory.reactor, reactor)
+
+
+class QueueResource(Resource):
+ """
+ Add all requests to an internal queue,
+ without responding to the requests.
+ You can access the requests from the queue and handle their response.
+ """
+
+ isLeaf = True
+
+ def __init__(self) -> None:
+ super().__init__()
+ self.dispatchedRequests: List[Request] = []
+
+ def render_GET(self, request: Request) -> int:
+ self.dispatchedRequests.append(request)
+ return NOT_DONE_YET
+
+
+class TestRFC9112Section932(unittest.TestCase):
+ """
+ Verify that HTTP/1.1 request ordering is preserved.
+ """
+
+ def test_multipleRequestsInOneSegment(self) -> None:
+ """
+ Twisted MUST NOT respond to a second HTTP/1.1 request while the first
+ is still pending.
+ """
+ qr = QueueResource()
+ site = Site(qr)
+ proto = site.buildProtocol(None)
+ serverTransport = StringTransport()
+ proto.makeConnection(serverTransport)
+ proto.dataReceived(
+ b"GET /first HTTP/1.1\r\nHost: a\r\n\r\n"
+ b"GET /second HTTP/1.1\r\nHost: a\r\n\r\n"
+ )
+ # The TCP data contains 2 requests,
+ # but only 1 request was dispatched,
+ # as the first request was not yet finalized.
+ self.assertEqual(len(qr.dispatchedRequests), 1)
+ # The first request is finalized and the
+ # second request is dispatched right away.
+ qr.dispatchedRequests[0].finish()
+ self.assertEqual(len(qr.dispatchedRequests), 2)
+
+ def test_multipleRequestsInDifferentSegments(self) -> None:
+ """
+ Twisted MUST NOT respond to a second HTTP/1.1 request while the first
+ is still pending, even if the second request is received in a separate
+ TCP package.
+ """
+ qr = QueueResource()
+ site = Site(qr)
+ proto = site.buildProtocol(None)
+ serverTransport = StringTransport()
+ proto.makeConnection(serverTransport)
+ raw_data = (
+ b"GET /first HTTP/1.1\r\nHost: a\r\n\r\n"
+ b"GET /second HTTP/1.1\r\nHost: a\r\n\r\n"
+ )
+ # Just go byte by byte for the extreme case in which each byte is
+ # received in a separate TCP package.
+ for chunk in iterbytes(raw_data):
+ proto.dataReceived(chunk)
+ # The TCP data contains 2 requests,
+ # but only 1 request was dispatched,
+ # as the first request was not yet finalized.
+ self.assertEqual(len(qr.dispatchedRequests), 1)
+ # The first request is finalized and the
+ # second request is dispatched right away.
+ qr.dispatchedRequests[0].finish()
+ self.assertEqual(len(qr.dispatchedRequests), 2)
--
2.40.0
|
