From 71d7f7f5ab4b9eb3be99f140ece0ced23220cc86 Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Sat, 13 Jun 2020 14:21:56 -0700 Subject: [PATCH 1/2] Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed. Motivation: Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See #230. Modifications: - Added a method to HTTPBin to redirect to the specified target. - Added failing tests that perform redirects from a regular server to a socket-based server and vice versa. Result: Failing tests that show that the existing redirect checks were inadequate. --- .../HTTPClientTestUtils.swift | 6 ++ .../HTTPClientTests.swift | 89 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index e5df9cfc5..6beba8938 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -495,6 +495,12 @@ internal final class HttpBinHandler: ChannelInboundHandler { headers.add(name: "Location", value: "/redirect/infinite1") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return + case "/redirect/target": + var headers = self.responseHeaders + let targetURL = req.headers["X-Target-Redirect-URL"].first ?? "" + headers.add(name: "Location", value: targetURL) + self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) + return case "/percent%20encoded": if req.method != .GET { self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed)) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ec8eb92fe..8c14e23af 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -347,6 +347,95 @@ class HTTPClientTests: XCTestCase { response = try localClient.get(url: self.defaultHTTPBinURLPrefix + "redirect/https?port=\(httpsBin.port)").wait() XCTAssertEqual(response.status, .ok) + + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpSocketPath in + XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpsSocketPath in + let socketHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(httpSocketPath)) + let socketHTTPSBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(httpsSocketPath)) + defer { + XCTAssertNoThrow(try socketHTTPBin.shutdown()) + XCTAssertNoThrow(try socketHTTPSBin.shutdown()) + } + + // From HTTP or HTTPS to HTTP+UNIX should fail to redirect + var targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + var request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + var response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + // From HTTP or HTTPS to HTTPS+UNIX should also fail to redirect + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.headers.first(name: "Location"), targetURL) + + // ... while HTTP+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed + targetURL = self.defaultHTTPBinURLPrefix + "ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https://localhost:\(httpsBin.port)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + // ... and HTTPS+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed + targetURL = self.defaultHTTPBinURLPrefix + "ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https://localhost:\(httpsBin.port)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + + targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok" + request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil) + + response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + }) + }) } func testHttpHostRedirect() { From 9db6474255ffaa7865acdf12ed3f3b5ca081dcd6 Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Sat, 13 Jun 2020 14:37:49 -0700 Subject: [PATCH 2/2] Fixed an issue where redirects to socket path-based servers from any server was always allowed. Motivation: An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme. Modifications: - Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do. - Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now. - Check the destination URL's scheme rather than the current URL's scheme when validating a redirect. Result: Closes #230 --- Sources/AsyncHTTPClient/HTTPHandler.swift | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index fb32f482c..95f79664b 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -107,8 +107,8 @@ extension HTTPClient { /// UNIX Domain Socket HTTP request. case unixSocket(_ scheme: UnixScheme) - private static var hostSchemes = ["http", "https"] - private static var unixSchemes = ["unix", "http+unix", "https+unix"] + private static var hostRestrictedSchemes: Set = ["http", "https"] + private static var allSupportedSchemes: Set = ["http", "https", "unix", "http+unix", "https+unix"] init(forScheme scheme: String) throws { switch scheme { @@ -158,12 +158,14 @@ extension HTTPClient { } } - func supports(scheme: String) -> Bool { + func supportsRedirects(to scheme: String?) -> Bool { + guard let scheme = scheme?.lowercased() else { return false } + switch self { case .host: - return Kind.hostSchemes.contains(scheme) + return Kind.hostRestrictedSchemes.contains(scheme) case .unixSocket: - return Kind.unixSchemes.contains(scheme) + return Kind.allSupportedSchemes.contains(scheme) } } } @@ -1049,7 +1051,7 @@ internal struct RedirectHandler { return nil } - guard self.request.kind.supports(scheme: self.request.scheme) else { + guard self.request.kind.supportsRedirects(to: url.scheme) else { return nil }