Skip to content

Commit dc98c04

Browse files
committed
[backport]: fix: patch http-proxy to prevent request smuggling in rewrites (#65) (#67)
* fix: patch http-proxy to prevent request smuggling in rewrites (#65) * namespace temp files (#50) * fix: patch http-proxy to prevent request smuggling in rewrites * ncc (cherry picked from commit e9a0b6dd0a27d07c0526b1d8d1b71c13ad4951dd) * block hop-by-hop TE rewrite smuggling bypass (#70) * block hop-by-hop TE rewrite smuggling bypass * more hardening * chore: minimize lockfile delta for http-proxy patch
1 parent 9023c0a commit dc98c04

File tree

7 files changed

+353
-9
lines changed

7 files changed

+353
-9
lines changed

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@
338338
"webpack-sources@3.2.3": "patches/webpack-sources@3.2.3.patch",
339339
"stacktrace-parser@0.1.10": "patches/stacktrace-parser@0.1.10.patch",
340340
"@types/node@20.17.6": "patches/@types__node@20.17.6.patch",
341+
"http-proxy@1.18.1": "patches/http-proxy@1.18.1.patch",
341342
"taskr@1.1.0": "patches/taskr@1.1.0.patch"
342343
}
343344
}

packages/next/src/compiled/http-proxy/index.js

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

patches/http-proxy@1.18.1.patch

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
diff --git a/lib/http-proxy/common.js b/lib/http-proxy/common.js
2+
index 6513e81d80d5250ea249ea833f819ece67897c7e..09143dd1fe4e67885f40ea916a6ea1ef3e3afa19 100644
3+
--- a/lib/http-proxy/common.js
4+
+++ b/lib/http-proxy/common.js
5+
@@ -1,9 +1,9 @@
6+
var common = exports,
7+
url = require('url'),
8+
- extend = require('util')._extend,
9+
required = require('requires-port');
10+
11+
var upgradeHeader = /(^|,)\s*upgrade\s*($|,)/i,
12+
+ hopByHopTransferEncodingHeader = /(^|,)\s*transfer-encoding\s*($|,)/i,
13+
isSSL = /^https|wss/;
14+
15+
/**
16+
@@ -40,10 +40,10 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
17+
);
18+
19+
outgoing.method = options.method || req.method;
20+
- outgoing.headers = extend({}, req.headers);
21+
+ outgoing.headers = Object.assign({}, req.headers);
22+
23+
if (options.headers){
24+
- extend(outgoing.headers, options.headers);
25+
+ Object.assign(outgoing.headers, options.headers);
26+
}
27+
28+
if (options.auth) {
29+
@@ -61,13 +61,22 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
30+
31+
outgoing.agent = options.agent || false;
32+
outgoing.localAddress = options.localAddress;
33+
+ outgoing.headers = outgoing.headers || {};
34+
+ var hasTransferEncodingHeader = Object.keys(outgoing.headers).some(function (header) {
35+
+ return header.toLowerCase() === 'transfer-encoding'
36+
+ && typeof outgoing.headers[header] !== 'undefined';
37+
+ });
38+
+
39+
+ if (hasTransferEncodingHeader
40+
+ || (typeof outgoing.headers.connection === 'string'
41+
+ && hopByHopTransferEncodingHeader.test(outgoing.headers.connection))
42+
+ ) { outgoing.headers.connection = 'close'; }
43+
44+
//
45+
// Remark: If we are false and not upgrading, set the connection: close. This is the right thing to do
46+
// as node core doesn't handle this COMPLETELY properly yet.
47+
//
48+
if (!outgoing.agent) {
49+
- outgoing.headers = outgoing.headers || {};
50+
if (typeof outgoing.headers.connection !== 'string'
51+
|| !upgradeHeader.test(outgoing.headers.connection)
52+
) { outgoing.headers.connection = 'close'; }
53+
diff --git a/lib/http-proxy/index.js b/lib/http-proxy/index.js
54+
index 977a4b3622b9eaac27689f06347ea4c5173a96cd..88b2d0fcfa03c3aafa47c7e6d38e64412c45a7cc 100644
55+
--- a/lib/http-proxy/index.js
56+
+++ b/lib/http-proxy/index.js
57+
@@ -1,5 +1,4 @@
58+
var httpProxy = module.exports,
59+
- extend = require('util')._extend,
60+
parse_url = require('url').parse,
61+
EE3 = require('eventemitter3'),
62+
http = require('http'),
63+
@@ -47,9 +46,9 @@ function createRightProxy(type) {
64+
args[cntr] !== res
65+
) {
66+
//Copy global options
67+
- requestOptions = extend({}, options);
68+
+ requestOptions = Object.assign({}, options);
69+
//Overwrite with request options
70+
- extend(requestOptions, args[cntr]);
71+
+ Object.assign(requestOptions, args[cntr]);
72+
73+
cntr--;
74+
}
75+
diff --git a/lib/http-proxy/passes/web-incoming.js b/lib/http-proxy/passes/web-incoming.js
76+
index 7ae735514190eea569c605fff7d27c045fe8d601..c7c25e7228b21c76b3c7115af82ddcbf13a8e3ec 100644
77+
--- a/lib/http-proxy/passes/web-incoming.js
78+
+++ b/lib/http-proxy/passes/web-incoming.js
79+
@@ -33,9 +33,9 @@ module.exports = {
80+
81+
deleteLength: function deleteLength(req, res, options) {
82+
if((req.method === 'DELETE' || req.method === 'OPTIONS')
83+
- && !req.headers['content-length']) {
84+
+ && typeof req.headers['content-length'] === 'undefined'
85+
+ && typeof req.headers['transfer-encoding'] === 'undefined') {
86+
req.headers['content-length'] = '0';
87+
- delete req.headers['transfer-encoding'];
88+
}
89+
},
90+

pnpm-lock.yaml

Lines changed: 7 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
/** @type {import('next').NextConfig} */
2+
const nextConfig = {
3+
async rewrites() {
4+
return [
5+
{
6+
source: '/rewrites/:path*',
7+
destination: `http://127.0.0.1:${process.env.TEST_INTERMEDIARY_PORT}/rewrites/:path*`,
8+
},
9+
]
10+
},
11+
}
12+
13+
module.exports = nextConfig
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export default function Page() {
2+
return <p>hello world</p>
3+
}
Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
import net from 'net'
2+
import http from 'http'
3+
import { createNext, NextInstance } from 'e2e-utils'
4+
import { findPort, retry } from 'next-test-utils'
5+
6+
describe('rewrite-request-smuggling', () => {
7+
if ((global as any).isNextDeploy) {
8+
it('should skip deploy', () => {})
9+
return
10+
}
11+
12+
let backend: http.Server
13+
let backendPort: number
14+
let intermediary: http.Server
15+
let intermediaryPort: number
16+
let next: NextInstance
17+
const backendRequests: string[] = []
18+
19+
async function sendSmugglingPayload({
20+
nextPort,
21+
connectionHeader,
22+
method = 'DELETE',
23+
rewritePath = '/rewrites/poc',
24+
}: {
25+
nextPort: number
26+
connectionHeader: string
27+
method?: 'DELETE' | 'OPTIONS'
28+
rewritePath?: string
29+
}) {
30+
const smuggledRequest = Buffer.from(
31+
`GET /secret HTTP/1.1\r\nHost: 127.0.0.1:${nextPort}\r\n\r\n`,
32+
'latin1'
33+
)
34+
const chunkSize = Buffer.from(
35+
`${smuggledRequest.length.toString(16).toUpperCase()}\r\n`,
36+
'latin1'
37+
)
38+
39+
const payload = Buffer.concat([
40+
Buffer.from(
41+
`${method} ${rewritePath} HTTP/1.1\r\nHost: 127.0.0.1:${nextPort}\r\nTransfer-Encoding: chunked\r\nConnection: ${connectionHeader}\r\n\r\n`,
42+
'latin1'
43+
),
44+
chunkSize,
45+
smuggledRequest,
46+
Buffer.from('\r\n0\r\n\r\n', 'latin1'),
47+
])
48+
49+
await new Promise<void>((resolve, reject) => {
50+
const socket = net.createConnection({
51+
host: '127.0.0.1',
52+
port: nextPort,
53+
})
54+
55+
socket.once('connect', () => {
56+
socket.write(payload)
57+
})
58+
socket.once('error', reject)
59+
socket.setTimeout(1000, () => socket.destroy())
60+
socket.once('close', () => resolve())
61+
})
62+
}
63+
64+
beforeAll(async () => {
65+
backendPort = await findPort()
66+
intermediaryPort = await findPort()
67+
68+
backend = http.createServer((req, res) => {
69+
backendRequests.push(`${req.method} ${req.url}`)
70+
71+
if (req.url?.startsWith('/rewrites/')) {
72+
res.statusCode = 200
73+
res.end('rewrite-ok')
74+
return
75+
}
76+
77+
if (req.url === '/secret') {
78+
res.statusCode = 200
79+
res.end('secret')
80+
return
81+
}
82+
83+
res.statusCode = 404
84+
res.end('not-found')
85+
})
86+
87+
intermediary = http.createServer((req, res) => {
88+
const connectionHeader = Array.isArray(req.headers['connection'])
89+
? req.headers['connection'].join(',')
90+
: req.headers['connection'] || ''
91+
const hopByHopHeaders = connectionHeader
92+
.split(',')
93+
.map((h) => h.trim().toLowerCase())
94+
.filter(Boolean)
95+
const stripTransferEncodingUnconditionally =
96+
req.url?.startsWith('/rewrites/non-rfc-strip') || false
97+
98+
const forwardHeaders: Record<string, string | string[]> = {}
99+
for (const [key, value] of Object.entries(req.headers)) {
100+
if (key === 'connection') continue
101+
if (stripTransferEncodingUnconditionally && key === 'transfer-encoding')
102+
continue
103+
if (hopByHopHeaders.includes(key)) continue
104+
if (value !== undefined) {
105+
forwardHeaders[key] = value
106+
}
107+
}
108+
forwardHeaders.connection = stripTransferEncodingUnconditionally
109+
? connectionHeader.toLowerCase().includes('close')
110+
? 'close'
111+
: 'keep-alive'
112+
: 'keep-alive'
113+
114+
const proxyReq = http.request(
115+
{
116+
hostname: '127.0.0.1',
117+
port: backendPort,
118+
method: req.method,
119+
path: req.url,
120+
headers: forwardHeaders,
121+
},
122+
(proxyRes) => {
123+
res.writeHead(proxyRes.statusCode || 500, proxyRes.headers)
124+
proxyRes.pipe(res)
125+
}
126+
)
127+
128+
proxyReq.on('error', () => {
129+
res.statusCode = 502
130+
res.end('Bad Gateway')
131+
})
132+
133+
req.pipe(proxyReq)
134+
})
135+
136+
await new Promise<void>((resolve, reject) => {
137+
backend.listen(backendPort, '127.0.0.1', resolve)
138+
backend.once('error', reject)
139+
})
140+
141+
await new Promise<void>((resolve, reject) => {
142+
intermediary.listen(intermediaryPort, '127.0.0.1', resolve)
143+
intermediary.once('error', reject)
144+
})
145+
146+
next = await createNext({
147+
files: __dirname,
148+
env: {
149+
TEST_INTERMEDIARY_PORT: String(intermediaryPort),
150+
},
151+
})
152+
})
153+
154+
afterAll(async () => {
155+
await next?.destroy()
156+
await new Promise<void>((resolve) => intermediary.close(() => resolve()))
157+
await new Promise<void>((resolve) => backend.close(() => resolve()))
158+
})
159+
160+
it('does not smuggle a second request when using keep-alive only', async () => {
161+
backendRequests.length = 0
162+
163+
const nextPort = Number(new URL(next.url).port)
164+
await sendSmugglingPayload({ nextPort, connectionHeader: 'keep-alive' })
165+
166+
await retry(async () => {
167+
expect(backendRequests).toContain('DELETE /rewrites/poc')
168+
})
169+
expect(backendRequests).not.toContain('GET /secret')
170+
})
171+
172+
it('does not smuggle a second request with keep-alive, upgrade', async () => {
173+
backendRequests.length = 0
174+
175+
const nextPort = Number(new URL(next.url).port)
176+
await sendSmugglingPayload({
177+
nextPort,
178+
connectionHeader: 'keep-alive, upgrade',
179+
})
180+
181+
await retry(async () => {
182+
expect(backendRequests).toContain('DELETE /rewrites/poc')
183+
})
184+
expect(backendRequests).not.toContain('GET /secret')
185+
})
186+
187+
it('does not smuggle a second request with Transfer-Encoding, upgrade', async () => {
188+
backendRequests.length = 0
189+
190+
const nextPort = Number(new URL(next.url).port)
191+
await sendSmugglingPayload({
192+
nextPort,
193+
connectionHeader: 'Transfer-Encoding, upgrade',
194+
})
195+
196+
await retry(async () => {
197+
expect(backendRequests).toContain('DELETE /rewrites/poc')
198+
})
199+
expect(backendRequests).not.toContain('GET /secret')
200+
})
201+
202+
it('does not smuggle a second request for OPTIONS with Transfer-Encoding, upgrade', async () => {
203+
backendRequests.length = 0
204+
205+
const nextPort = Number(new URL(next.url).port)
206+
await sendSmugglingPayload({
207+
nextPort,
208+
method: 'OPTIONS',
209+
connectionHeader: 'Transfer-Encoding, upgrade',
210+
})
211+
212+
await retry(async () => {
213+
expect(backendRequests).toContain('OPTIONS /rewrites/poc')
214+
})
215+
expect(backendRequests).not.toContain('GET /secret')
216+
})
217+
218+
it('does not smuggle a second request when an intermediary strips transfer-encoding unconditionally', async () => {
219+
backendRequests.length = 0
220+
221+
const nextPort = Number(new URL(next.url).port)
222+
await sendSmugglingPayload({
223+
nextPort,
224+
method: 'OPTIONS',
225+
rewritePath: '/rewrites/non-rfc-strip',
226+
connectionHeader: 'keep-alive, upgrade',
227+
})
228+
229+
await retry(async () => {
230+
expect(backendRequests).toContain('OPTIONS /rewrites/non-rfc-strip')
231+
})
232+
expect(backendRequests).not.toContain('GET /secret')
233+
})
234+
})

0 commit comments

Comments
 (0)