From 346779b490f72448b14c5928ffe17280a2b3ceb4 Mon Sep 17 00:00:00 2001 From: Mark Bumiller Date: Fri, 6 Oct 2023 16:29:23 -0400 Subject: [PATCH 1/3] Do not allow forwarding of authorization headers on redirect. There is now a flag `auth_on_redirect` that can be set if you need to pass authorization headers. This is similar to https://curl.se/docs/CVE-2018-1000007.html and https://nvd.nist.gov/vuln/detail/CVE-2021-31879 --- lib/down/net_http.rb | 9 +++++++-- test/net_http_test.rb | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/down/net_http.rb b/lib/down/net_http.rb index b61f18a..c099163 100644 --- a/lib/down/net_http.rb +++ b/lib/down/net_http.rb @@ -41,6 +41,7 @@ def download(url, *args, **options) headers = options.delete(:headers) uri_normalizer = options.delete(:uri_normalizer) extension = options.delete(:extension) + auth_on_redirect = options.delete(:auth_on_redirect) # Use open-uri's :content_lenth_proc or :progress_proc to raise an # exception early if the file is too large. @@ -91,7 +92,7 @@ def download(url, *args, **options) uri.password = nil end - open_uri_file = open_uri(uri, open_uri_options, follows_remaining: max_redirects) + open_uri_file = open_uri(uri, open_uri_options, follows_remaining: max_redirects, auth_on_redirect: ) # Handle the fact that open-uri returns StringIOs for small files. extname = extension ? ".#{extension}" : File.extname(open_uri_file.base_uri.path) @@ -141,7 +142,7 @@ def open(url, *args, **options) private # Calls open-uri's URI::HTTP#open method. Additionally handles redirects. - def open_uri(uri, options, follows_remaining:) + def open_uri(uri, options, follows_remaining:, auth_on_redirect:) uri.open(options) rescue OpenURI::HTTPRedirect => exception raise Down::TooManyRedirects, "too many redirects" if follows_remaining == 0 @@ -155,6 +156,10 @@ def open_uri(uri, options, follows_remaining:) raise ResponseError.new("Invalid Redirect URI: #{exception.uri}", response: response) end + # do not leak credentials on redirect + options.delete("Authorization") unless auth_on_redirect + options.delete(:http_basic_authentication) unless auth_on_redirect + # forward cookies on the redirect if !exception.io.meta["set-cookie"].to_s.empty? options["Cookie"] ||= '' diff --git a/test/net_http_test.rb b/test/net_http_test.rb index 9f80058..ca65925 100644 --- a/test/net_http_test.rb +++ b/test/net_http_test.rb @@ -125,6 +125,21 @@ # "Set-Cookie" header. end + it "removes Authorization header on redirects" do + tempfile = Down::NetHttp.download("#{$httpbin}/redirect/1", headers: {"Authorization" => "Basic dXNlcjpwYXNzd29yZA=="}) + assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"] + end + + it "removes Baisic Auth credentials header on redirects" do + tempfile = Down::NetHttp.download("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", ) + assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"] + end + + it "preserves Authorization header on redirect, when asked" do + tempfile = Down::NetHttp.download("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", auth_on_redirect:true ) + assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(tempfile.read)["headers"]["Authorization"] + end + # I don't know how to test that the proxy is actually used it "accepts proxy" do tempfile = Down::NetHttp.download("#{$httpbin}/bytes/100", proxy: $httpbin) From 8cf8f1a0b27511aca315f81571ace24ada70688c Mon Sep 17 00:00:00 2001 From: Mark Bumiller Date: Mon, 13 Nov 2023 10:16:42 -0500 Subject: [PATCH 2/3] do not forward headers for #open --- lib/down/net_http.rb | 16 +++++++++++----- test/net_http_test.rb | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/down/net_http.rb b/lib/down/net_http.rb index c099163..8f2980b 100644 --- a/lib/down/net_http.rb +++ b/lib/down/net_http.rb @@ -108,14 +108,15 @@ def download(url, *args, **options) def open(url, *args, **options) options = merge_options(@options, *args, **options) - max_redirects = options.delete(:max_redirects) - uri_normalizer = options.delete(:uri_normalizer) + max_redirects = options.delete(:max_redirects) + uri_normalizer = options.delete(:uri_normalizer) + auth_on_redirect = options.delete(:auth_on_redirect) uri = ensure_uri(normalize_uri(url, uri_normalizer: uri_normalizer)) # Create a Fiber that halts when response headers are received. request = Fiber.new do - net_http_request(uri, options, follows_remaining: max_redirects) do |response| + net_http_request(uri, options, follows_remaining: max_redirects, auth_on_redirect:) do |response| Fiber.yield response end end @@ -205,7 +206,7 @@ def ensure_tempfile(io, extension) end # Makes a Net::HTTP request and follows redirects. - def net_http_request(uri, options, follows_remaining:, &block) + def net_http_request(uri, options, follows_remaining:, auth_on_redirect:, &block) http, request = create_net_http(uri, options) begin @@ -236,10 +237,15 @@ def net_http_request(uri, options, follows_remaining:, &block) raise ResponseError.new("Invalid Redirect URI: #{response["Location"]}", response: response) end + # do not leak credentials on redirect + uri.user = nil unless auth_on_redirect + uri.password = nil unless auth_on_redirect + options[:headers].delete("Authorization") unless auth_on_redirect + # handle relative redirects location = uri + location if location.relative? - net_http_request(location, options, follows_remaining: follows_remaining - 1, &block) + net_http_request(location, options, follows_remaining: follows_remaining - 1, auth_on_redirect:, &block) end end diff --git a/test/net_http_test.rb b/test/net_http_test.rb index ca65925..5f29379 100644 --- a/test/net_http_test.rb +++ b/test/net_http_test.rb @@ -339,6 +339,21 @@ assert_equal "#{$httpbin}/get", JSON.parse(io.read)["url"] end + it "removes Authorization header on redirects" do + io = Down::NetHttp.open("#{$httpbin}/redirect/1", headers: {"Authorization" => "Basic dXNlcjpwYXNzd29yZA=="}) + assert_nil JSON.parse(io.read)["headers"]["Authorization"] + end + + it "removes Baisic Auth credentials header on redirects" do + io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", ) + assert_nil JSON.parse(io.read)["headers"]["Authorization"] + end + + it "preserves Authorization header on redirect, when asked" do + io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", auth_on_redirect:true ) + assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(io.read)["headers"]["Authorization"] + end + it "returns content in encoding specified by charset" do io = Down::NetHttp.open("#{$httpbin}/stream/10") assert_equal Encoding::BINARY, io.read.encoding From 00010c9d1c14ababec191e1813981ca2e2763dab Mon Sep 17 00:00:00 2001 From: Mark Bumiller Date: Mon, 13 Nov 2023 10:47:06 -0500 Subject: [PATCH 3/3] allow keeping BasicAuth credentials for relative redirects. This only works for #open as #download does not have the same informtion --- lib/down/net_http.rb | 9 ++++++--- test/net_http_test.rb | 11 ++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/down/net_http.rb b/lib/down/net_http.rb index 8f2980b..1efbd80 100644 --- a/lib/down/net_http.rb +++ b/lib/down/net_http.rb @@ -150,6 +150,7 @@ def open_uri(uri, options, follows_remaining:, auth_on_redirect:) # fail if redirect URI scheme is not http or https begin + same_host = uri.host.eql? exception.uri.host uri = ensure_uri(exception.uri) rescue Down::InvalidUrl response = rebuild_response_from_open_uri_exception(exception) @@ -238,12 +239,14 @@ def net_http_request(uri, options, follows_remaining:, auth_on_redirect:, &block end # do not leak credentials on redirect - uri.user = nil unless auth_on_redirect - uri.password = nil unless auth_on_redirect options[:headers].delete("Authorization") unless auth_on_redirect # handle relative redirects - location = uri + location if location.relative? + if location.relative? + location = uri + location + uri.user = nil unless auth_on_redirect + uri.password = nil unless auth_on_redirect + end net_http_request(location, options, follows_remaining: follows_remaining - 1, auth_on_redirect:, &block) end diff --git a/test/net_http_test.rb b/test/net_http_test.rb index 5f29379..31e1e5d 100644 --- a/test/net_http_test.rb +++ b/test/net_http_test.rb @@ -130,7 +130,7 @@ assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"] end - it "removes Baisic Auth credentials header on redirects" do + it "removes Basic Auth credentials header on redirects" do tempfile = Down::NetHttp.download("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", ) assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"] end @@ -344,11 +344,16 @@ assert_nil JSON.parse(io.read)["headers"]["Authorization"] end - it "removes Baisic Auth credentials header on redirects" do - io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", ) + it "removes Basic Auth credentials header on absolute redirects" do + io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/absolute-redirect/1", ) assert_nil JSON.parse(io.read)["headers"]["Authorization"] end + it "preserves Basic Auth credentials header on relative redirects" do + io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/relative-redirect/1", ) + assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(io.read)["headers"]["Authorization"] + end + it "preserves Authorization header on redirect, when asked" do io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", auth_on_redirect:true ) assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(io.read)["headers"]["Authorization"]