From 4e02435c2701eb72cf2e6107a26315bd1a6181e1 Mon Sep 17 00:00:00 2001 From: Arkadiy Tetelman Date: Wed, 31 Aug 2022 00:30:53 -0700 Subject: [PATCH] Handle net-http changes --- .github/workflows/build-test.yml | 4 +-- Dockerfile | 2 +- README.md | 2 +- lib/ssrf_filter.rb | 1 + lib/ssrf_filter/patch/resolv.rb | 44 +++++++++++++++++++++++++++++ lib/ssrf_filter/patch/ssl_socket.rb | 5 ++-- lib/ssrf_filter/ssrf_filter.rb | 14 +++++---- spec/lib/patch/resolv_spec.rb | 33 ++++++++++++++++++++++ spec/lib/ssrf_filter_spec.rb | 22 +++++++++------ spec/spec_helper.rb | 1 + 10 files changed, 109 insertions(+), 19 deletions(-) create mode 100644 lib/ssrf_filter/patch/resolv.rb create mode 100644 spec/lib/patch/resolv_spec.rb diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 9f3e7d5..2932d1e 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -1,6 +1,6 @@ name: Build-test -on: +on: push: workflow_call: @@ -8,7 +8,7 @@ jobs: build-test: strategy: matrix: - ruby-version: [2.6.0, 2.7.0, 3.0.0, 3.1.0] + ruby-version: [2.6.0, 2.7.0, 3.0.0, 3.1.0, head] runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 diff --git a/Dockerfile b/Dockerfile index 33785dd..3472c33 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM ruby:2.6.0 +FROM ruby:3.0.0 RUN apt update && apt-get install -y vim diff --git a/README.md b/README.md index dfd6386..45b36f4 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ This gem provides a safe and easy way to fetch content from user-submitted urls. - handles URIs/IPv4/IPv6, redirects, DNS, etc, correctly - has 0 runtime dependencies - has a comprehensive test suite (100% code coverage) -- is tested against ruby `2.6`, `2.7`, `3.0`, and `3.1` +- is tested against ruby `2.6`, `2.7`, `3.0`, `3.1`, and `ruby-head` ### Quick start diff --git a/lib/ssrf_filter.rb b/lib/ssrf_filter.rb index 98e4814..31e4983 100644 --- a/lib/ssrf_filter.rb +++ b/lib/ssrf_filter.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'ssrf_filter/patch/resolv' require 'ssrf_filter/patch/ssl_socket' require 'ssrf_filter/ssrf_filter' require 'ssrf_filter/version' diff --git a/lib/ssrf_filter/patch/resolv.rb b/lib/ssrf_filter/patch/resolv.rb new file mode 100644 index 0000000..2a0c170 --- /dev/null +++ b/lib/ssrf_filter/patch/resolv.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'resolv' + +class SsrfFilter + module Patch + module Resolv + # As described in ssl_socket.rb, we want to patch ruby's http connection code to allow us to make outbound network + # requests while ensuring that both: + # 1) we're connecting to a public / non-private ip address + # 2) https connections continue to work + # + # This used to work fine prior to this change in ruby's net/http library: + # https://github.com/ruby/net-http/pull/36 + # After this changed was introduced our patch no longer works - we need to set the hostname to the correct + # value on the SSLSocket (`s.hostname = ssl_host_address`), but that code path no longer executes due to the + # modification in the linked pull request. + # + # To work around this we introduce the patch below, which forces our ip address string to not match against the + # Resolv IPv4/IPv6 regular expressions. This is ugly and cumbersome but I didn't see any better path. + class PatchedRegexp < Regexp + def ===(other) + if ::Thread.current.key?(::SsrfFilter::FIBER_ADDRESS_KEY) && + other.object_id.equal?(::Thread.current[::SsrfFilter::FIBER_ADDRESS_KEY].object_id) + false + else + super(other) + end + end + end + + def self.apply! + return if instance_variable_defined?(:@patched_resolv) + + @patched_resolv = true + + old_ipv4 = ::Resolv::IPv4.send(:remove_const, :Regex) + old_ipv6 = ::Resolv::IPv6.send(:remove_const, :Regex) + ::Resolv::IPv4.const_set(:Regex, PatchedRegexp.new(old_ipv4)) + ::Resolv::IPv6.const_set(:Regex, PatchedRegexp.new(old_ipv6)) + end + end + end +end diff --git a/lib/ssrf_filter/patch/ssl_socket.rb b/lib/ssrf_filter/patch/ssl_socket.rb index 7eae69e..db8dcad 100644 --- a/lib/ssrf_filter/patch/ssl_socket.rb +++ b/lib/ssrf_filter/patch/ssl_socket.rb @@ -35,13 +35,14 @@ def self.apply! ::OpenSSL::SSL::SSLSocket.class_eval do original_post_connection_check = instance_method(:post_connection_check) define_method(:post_connection_check) do |hostname| - original_post_connection_check.bind(self).call(::Thread.current[::SsrfFilter::FIBER_LOCAL_KEY] || hostname) + original_post_connection_check.bind(self).call(::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY] || + hostname) end if method_defined?(:hostname=) original_hostname = instance_method(:hostname=) define_method(:hostname=) do |hostname| - original_hostname.bind(self).call(::Thread.current[::SsrfFilter::FIBER_LOCAL_KEY] || hostname) + original_hostname.bind(self).call(::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY] || hostname) end end end diff --git a/lib/ssrf_filter/ssrf_filter.rb b/lib/ssrf_filter/ssrf_filter.rb index 9523206..9791768 100644 --- a/lib/ssrf_filter/ssrf_filter.rb +++ b/lib/ssrf_filter/ssrf_filter.rb @@ -83,7 +83,8 @@ def self.prefixlen_from_ipaddr(ipaddr) patch: ::Net::HTTP::Patch }.freeze - FIBER_LOCAL_KEY = :__ssrf_filter_hostname + FIBER_HOSTNAME_KEY = :__ssrf_filter_hostname + FIBER_ADDRESS_KEY = :__ssrf_filter_address class Error < ::StandardError end @@ -106,6 +107,7 @@ class CRLFInjection < Error %i[get put post delete head patch].each do |method| define_singleton_method(method) do |url, options = {}, &block| ::SsrfFilter::Patch::SSLSocket.apply! + ::SsrfFilter::Patch::Resolv.apply! original_url = url scheme_whitelist = options[:scheme_whitelist] || DEFAULT_SCHEME_WHITELIST @@ -187,7 +189,7 @@ def self.fetch_once(uri, ip, verb, options, &block) http_options = options[:http_options] || {} http_options[:use_ssl] = (uri.scheme == 'https') - with_forced_hostname(hostname) do + with_forced_hostname(hostname, ip) do ::Net::HTTP.start(uri.hostname, uri.port, **http_options) do |http| http.request(request) do |response| case response @@ -219,11 +221,13 @@ def self.validate_request(request) end private_class_method :validate_request - def self.with_forced_hostname(hostname, &_block) - ::Thread.current[FIBER_LOCAL_KEY] = hostname + def self.with_forced_hostname(hostname, ip, &_block) + ::Thread.current[FIBER_HOSTNAME_KEY] = hostname + ::Thread.current[FIBER_ADDRESS_KEY] = ip yield ensure - ::Thread.current[FIBER_LOCAL_KEY] = nil + ::Thread.current[FIBER_HOSTNAME_KEY] = nil + ::Thread.current[FIBER_ADDRESS_KEY] = nil end private_class_method :with_forced_hostname end diff --git a/spec/lib/patch/resolv_spec.rb b/spec/lib/patch/resolv_spec.rb new file mode 100644 index 0000000..d457ad9 --- /dev/null +++ b/spec/lib/patch/resolv_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +describe ::SsrfFilter::Patch::Resolv do + describe 'apply' do + before do + if described_class.instance_variable_defined?(:@patched_resolv) + described_class.remove_instance_variable(:@patched_resolv) + end + end + + it 'only patches once' do + expect(::Resolv::IPv4).to receive(:remove_const).once.and_call_original + expect(::Resolv::IPv6).to receive(:remove_const).once.and_call_original + described_class.apply! + described_class.apply! + end + end + + describe ::SsrfFilter::Patch::Resolv::PatchedRegexp do + it 'forces the ip regex to not match the supplied address' do + # rubocop:disable Style/CaseEquality + ipaddress1 = '1.2.3.4' + ipaddress2 = '5.6.7.8' + SsrfFilter.send(:with_forced_hostname, nil, ipaddress1) do + expect(described_class.new(Resolv::IPv4::Regex) === ipaddress1).to be false + expect(described_class.new(Resolv::IPv4::Regex) === ipaddress2).to be true + end + expect(described_class.new(Resolv::IPv4::Regex) === ipaddress1).to be true + expect(described_class.new(Resolv::IPv4::Regex) === ipaddress2).to be true + # rubocop:enable Style/CaseEquality + end + end +end diff --git a/spec/lib/ssrf_filter_spec.rb b/spec/lib/ssrf_filter_spec.rb index ad0f3e1..57a6455 100644 --- a/spec/lib/ssrf_filter_spec.rb +++ b/spec/lib/ssrf_filter_spec.rb @@ -145,22 +145,28 @@ describe 'with_forced_hostname' do it 'sets the value for the block and clear it afterwards' do - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to be_nil - described_class.with_forced_hostname('test') do - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to eq('test') + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil + described_class.with_forced_hostname('test', '1.2.3.4') do + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test') + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to eq('1.2.3.4') end - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil end it 'clears the value even if an exception is raised' do - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil expect do - described_class.with_forced_hostname('test') do - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to eq('test') + described_class.with_forced_hostname('test', '1.2.3.4') do + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test') + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to eq('1.2.3.4') raise StandardError end end.to raise_error(StandardError) - expect(Thread.current[described_class::FIBER_LOCAL_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil + expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 8db8052..05cf935 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'pry-byebug' require 'simplecov' require 'simplecov-lcov' SimpleCov.start do