diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 9b7cce7..b4cdbec 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -1,15 +1,14 @@ name: Build-test on: - push: - pull_request_target: + pull_request: workflow_call: jobs: build-test: strategy: matrix: - ruby-version: [2.6.0, 2.7.0, 3.0.0, 3.1.0, 3.2.0, head] + ruby-version: [2.7.0, 3.0.0, 3.1.0, 3.2.0, 3.3.0, head] runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 diff --git a/.rspec b/.rspec index b9caf0b..21b145e 100644 --- a/.rspec +++ b/.rspec @@ -1,4 +1,5 @@ --color ---order random --warning +--order random --require spec_helper +--format documentation \ No newline at end of file diff --git a/.rubocop.yml b/.rubocop.yml index d006e09..1d74c16 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -7,8 +7,8 @@ require: AllCops: NewCops: enable -Gemspec/RequiredRubyVersion: - Enabled: false +Gemspec/DevelopmentDependencies: + EnforcedStyle: gemspec Metrics/AbcSize: Enabled: false @@ -52,15 +52,15 @@ Layout/SpaceInsideHashLiteralBraces: RSpec/BeforeAfterAll: Enabled: false +RSpec/IndexedLet: + Enabled: false + RSpec/MultipleExpectations: Enabled: false RSpec/ExampleLength: Max: 40 -RSpec/FilePath: - SpecSuffixOnly: true - RSpec/MessageSpies: EnforcedStyle: receive diff --git a/Dockerfile b/Dockerfile index f21bfed..03d2567 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,6 +5,5 @@ RUN apt update && apt-get install -y vim tmux tig WORKDIR app COPY Gemfile ssrf_filter.gemspec . COPY lib/ssrf_filter/version.rb lib/ssrf_filter/version.rb -RUN bundle update +RUN bundle install ENV CI=1 -COPY . . diff --git a/lib/ssrf_filter.rb b/lib/ssrf_filter.rb index 98e4814..94e3434 100644 --- a/lib/ssrf_filter.rb +++ b/lib/ssrf_filter.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true -require 'ssrf_filter/patch/ssl_socket' require 'ssrf_filter/ssrf_filter' require 'ssrf_filter/version' diff --git a/lib/ssrf_filter/patch/ssl_socket.rb b/lib/ssrf_filter/patch/ssl_socket.rb deleted file mode 100644 index 7ea42d3..0000000 --- a/lib/ssrf_filter/patch/ssl_socket.rb +++ /dev/null @@ -1,66 +0,0 @@ -# frozen_string_literal: true - -class SsrfFilter - module Patch - module SSLSocket - def self.apply! - return if instance_variable_defined?(:@patched_ssl_socket) - - @patched_ssl_socket = true - - ::OpenSSL::SSL::SSLSocket.class_eval do - # When fetching a url we'd like to have the following workflow: - # 1) resolve the hostname www.example.com, and choose a public ip address to connect to - # 2) connect to that specific ip address, to prevent things like DNS TOCTTOU bugs or other trickery - # - # Ideally this would happen by the ruby http library giving us control over DNS resolution, - # but it doesn't. Instead, when making the request we set the uri.hostname to the chosen ip address, - # and send a 'Host' header of the original hostname, i.e. connect to 'http://93.184.216.34/' and send - # a 'Host: www.example.com' header. - # - # This works for the http case, http://www.example.com. For the https case, this causes certificate - # validation failures, since the server certificate for https://www.example.com will not have a - # Subject Alternate Name for 93.184.216.34. - # - # Thus we perform the monkey-patch below, modifying SSLSocket's `post_connection_check(hostname)` - # and `hostname=(hostname)` methods: - # If our fiber local variable is set, use that for the hostname instead, otherwise behave as usual. - # The only time the variable will be set is if you are executing inside a `with_forced_hostname` block, - # which is used in ssrf_filter.rb. - # - # An alternative approach could be to pass in our own OpenSSL::X509::Store with a custom - # `verify_callback` to the ::Net::HTTP.start call, but this would require reimplementing certification - # validation, which is dangerous. This way we can piggyback on the existing validation and simply pretend - # that we connected to the desired hostname. - - 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_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_HOSTNAME_KEY] || hostname) - end - end - - # This patch is the successor to https://github.com/arkadiyt/ssrf_filter/pull/54 - # Due to some changes in Ruby's net/http library (namely https://github.com/ruby/net-http/pull/36), - # the SSLSocket in the request was no longer getting `s.hostname` set. - # The original fix tried to monkey-patch the Regexp class to cause the original code path to execute, - # but this caused other problems (like https://github.com/arkadiyt/ssrf_filter/issues/61) - # This fix attempts a different approach to set the hostname on the socket - original_initialize = instance_method(:initialize) - define_method(:initialize) do |*args| - original_initialize.bind(self).call(*args) - if ::Thread.current.key?(::SsrfFilter::FIBER_HOSTNAME_KEY) - self.hostname = ::Thread.current[::SsrfFilter::FIBER_HOSTNAME_KEY] - end - end - end - end - end - end -end diff --git a/lib/ssrf_filter/ssrf_filter.rb b/lib/ssrf_filter/ssrf_filter.rb index 91b23ca..a0eed8c 100644 --- a/lib/ssrf_filter/ssrf_filter.rb +++ b/lib/ssrf_filter/ssrf_filter.rb @@ -10,7 +10,7 @@ def self.prefixlen_from_ipaddr(ipaddr) mask_addr = ipaddr.instance_variable_get('@mask_addr') raise ArgumentError, 'Invalid mask' if mask_addr.zero? - while (mask_addr & 0x1).zero? + while mask_addr.nobits?(0x1) mask_addr >>= 1 end @@ -84,8 +84,6 @@ def self.prefixlen_from_ipaddr(ipaddr) patch: ::Net::HTTP::Patch }.freeze - FIBER_HOSTNAME_KEY = :__ssrf_filter_hostname - class Error < ::StandardError end @@ -106,8 +104,6 @@ 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! - original_url = url scheme_whitelist = options.fetch(:scheme_whitelist, DEFAULT_SCHEME_WHITELIST) resolver = options.fetch(:resolver, DEFAULT_RESOLVER) @@ -156,16 +152,16 @@ def self.ipaddr_has_mask?(ipaddr) end private_class_method :ipaddr_has_mask? - def self.host_header(hostname, uri) + def self.normalized_hostname(uri) # Attach port for non-default as per RFC2616 if (uri.port == 80 && uri.scheme == 'http') || (uri.port == 443 && uri.scheme == 'https') - hostname + uri.hostname else - "#{hostname}:#{uri.port}" + "#{uri.hostname}:#{uri.port}" end end - private_class_method :host_header + private_class_method :normalized_hostname def self.fetch_once(uri, ip, verb, options, &block) if options[:params] @@ -174,11 +170,8 @@ def self.fetch_once(uri, ip, verb, options, &block) uri.query = ::URI.encode_www_form(params) end - hostname = uri.hostname - uri.hostname = ip - request = VERB_MAP[verb].new(uri) - request['host'] = host_header(hostname, uri) + request['host'] = normalized_hostname(uri) Array(options[:headers]).each do |header, value| request[header] = value @@ -189,24 +182,24 @@ def self.fetch_once(uri, ip, verb, options, &block) options[:request_proc].call(request) if options[:request_proc].respond_to?(:call) validate_request(request) - http_options = options[:http_options] || {} - http_options[:use_ssl] = (uri.scheme == 'https') + http_options = (options[:http_options] || {}).merge( + use_ssl: uri.scheme == 'https', + ipaddr: ip + ) - with_forced_hostname(hostname) do - ::Net::HTTP.start(uri.hostname, uri.port, **http_options) do |http| - response = http.request(request) do |res| - block&.call(res) - end - case response - when ::Net::HTTPRedirection - url = response['location'] - # Handle relative redirects - url = "#{uri.scheme}://#{hostname}:#{uri.port}#{url}" if url.start_with?('/') - else - url = nil - end - return response, url + ::Net::HTTP.start(uri.hostname, uri.port, **http_options) do |http| + response = http.request(request) do |res| + block&.call(res) + end + case response + when ::Net::HTTPRedirection + url = response['location'] + # Handle relative redirects + url = "#{uri.scheme}://#{normalized_hostname(uri)}#{url}" if url.start_with?('/') + else + url = nil end + return response, url end end private_class_method :fetch_once @@ -223,12 +216,4 @@ def self.validate_request(request) end end private_class_method :validate_request - - def self.with_forced_hostname(hostname, &_block) - ::Thread.current[FIBER_HOSTNAME_KEY] = hostname - yield - ensure - ::Thread.current[FIBER_HOSTNAME_KEY] = nil - end - private_class_method :with_forced_hostname end diff --git a/spec/lib/patch/ssl_socket_spec.rb b/spec/lib/patch/ssl_socket_spec.rb deleted file mode 100644 index fc5d7fb..0000000 --- a/spec/lib/patch/ssl_socket_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -describe ::SsrfFilter::Patch::SSLSocket do - before do - if described_class.instance_variable_defined?(:@patched_ssl_socket) - described_class.remove_instance_variable(:@patched_ssl_socket) - end - end - - it 'only patches once' do - expect(::OpenSSL::SSL::SSLSocket).to receive(:class_eval).once.and_call_original - described_class.apply! - described_class.apply! - end -end diff --git a/spec/lib/ssrf_filter_spec.rb b/spec/lib/ssrf_filter_spec.rb index 897ae8f..9f94412 100644 --- a/spec/lib/ssrf_filter_spec.rb +++ b/spec/lib/ssrf_filter_spec.rb @@ -47,8 +47,7 @@ end it 'returns true for unknown ip families' do - allow(public_ipv4).to receive(:ipv4?).and_return(false) - allow(public_ipv4).to receive(:ipv6?).and_return(false) + allow(public_ipv4).to receive_messages(ipv4?: false, ipv6?: false) expect(described_class.unsafe_ip_address?(public_ipv4)).to be(true) end end @@ -79,7 +78,7 @@ describe 'fetch_once' do it 'sets the host header' do - stub_request(:post, "https://#{public_ipv4}").with(headers: {host: 'www.example.com'}) + stub_request(:post, 'https://www.example.com').with(headers: {host: 'www.example.com'}) .to_return(status: 200, body: 'response body') response, url = described_class.fetch_once(URI('https://www.example.com'), public_ipv4.to_s, :post, {}) expect(response.code).to eq('200') @@ -88,7 +87,7 @@ end it 'does not send the port in the host header for default ports (http)' do - stub_request(:post, "http://#{public_ipv4}").with(headers: {host: 'www.example.com'}) + stub_request(:post, 'http://www.example.com').with(headers: {host: 'www.example.com'}) .to_return(status: 200, body: 'response body') response, url = described_class.fetch_once(URI('http://www.example.com'), public_ipv4.to_s, :post, {}) expect(response.code).to eq('200') @@ -97,8 +96,7 @@ end it 'sends the port in the host header for non-default ports' do - stub_request(:post, "https://#{public_ipv4}:80").with(headers: {host: 'www.example.com:80'}) - .to_return(status: 200, body: 'response body') + stub_request(:post, 'https://www.example.com:80').to_return(status: 200, body: 'response body') response, url = described_class.fetch_once(URI('https://www.example.com:80'), public_ipv4.to_s, :post, {}) expect(response.code).to eq('200') expect(response.body).to eq('response body') @@ -106,7 +104,7 @@ end it 'passes headers, params, and blocks' do - stub_request(:get, "https://#{public_ipv4}/?key=value").with(headers: + stub_request(:get, 'https://www.example.com/?key=value').with(headers: {host: 'www.example.com', header: 'value', header2: 'value2'}).to_return(status: 200, body: 'response body') options = { headers: {'header' => 'value'}, @@ -123,8 +121,8 @@ end it 'merges params' do - stub_request(:get, "https://#{public_ipv4}/?key=value&key2=value2") - .with(headers: {host: 'www.example.com'}).to_return(status: 200, body: 'response body') + stub_request(:get, 'https://www.example.com/?key=value&key2=value2') + .to_return(status: 200, body: 'response body') uri = URI('https://www.example.com/?key=value') response, url = described_class.fetch_once(uri, public_ipv4.to_s, :get, params: {'key2' => 'value2'}) expect(response.code).to eq('200') @@ -132,38 +130,17 @@ expect(url).to be_nil end - it 'does not use tls for http urls', only: true do - expect(::Net::HTTP).to receive(:start).with(public_ipv4.to_s, 80, use_ssl: false) + it 'does not use tls for http urls' do + expect(Net::HTTP).to receive(:start).with('www.example.com', 80, hash_including(use_ssl: false)) described_class.fetch_once(URI('http://www.example.com'), public_ipv4.to_s, :get, {}) end it 'uses tls for https urls' do - expect(::Net::HTTP).to receive(:start).with(public_ipv4.to_s, 443, use_ssl: true) + expect(Net::HTTP).to receive(:start).with('www.example.com', 443, hash_including(use_ssl: true)) described_class.fetch_once(URI('https://www.example.com'), public_ipv4.to_s, :get, {}) end end - describe 'with_forced_hostname' do - it 'sets the value for the block and clear it afterwards' do - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil - described_class.with_forced_hostname('test') do - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test') - end - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil - end - - it 'clears the value even if an exception is raised' do - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil - expect do - described_class.with_forced_hostname('test') do - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to eq('test') - raise StandardError - end - end.to raise_error(StandardError) - expect(Thread.current[described_class::FIBER_HOSTNAME_KEY]).to be_nil - end - end - describe 'validate_request' do it 'disallows header names with newlines and carriage returns' do expect do @@ -191,8 +168,8 @@ end describe 'integration tests' do - # To test the SSLSocket patching logic (and hit 100% code coverage), we need to make a real connection to a - # TLS-enabled server. To do this we create a private key and certificate, spin up a web server in + # To hit 100% code coverage, we need to make a real connection to a TLS-enabled server. + # To do this we create a private key and certificate, spin up a web server in # a thread (serving traffic on localhost), and make a request to the server. This requires several things: # 1) creating a custom trust store with our certificate and using that for validation # 2) allowing (non-mocked) network connections @@ -244,7 +221,7 @@ def inject_custom_trust_store(*certificates) store.add_cert(certificate) end - expect(::Net::HTTP).to receive(:start).exactly(certificates.length).times + expect(Net::HTTP).to receive(:start).exactly(certificates.length).times .and_wrap_original do |orig, *args, &block| args.last[:cert_store] = store # Inject our custom trust store orig.call(*args, &block) @@ -452,22 +429,17 @@ def inject_custom_trust_store(*certificates) end it 'fails if there are too many redirects' do - stub_request(:get, "https://#{public_ipv4}").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: private_ipv4}) - resolver = proc { [public_ipv4] } + stub_request(:get, 'https://www.example.com').to_return(status: 301, headers: {location: 'https://example2.com'}) expect do - described_class.get('https://www.example.com', resolver: resolver, max_redirects: 0) + described_class.get('https://www.example.com', max_redirects: 0) end.to raise_error(described_class::TooManyRedirects) end it 'returns the last response if there are too many redirects and unfollowed redirects are allowed' do - stub_request(:get, "https://#{public_ipv4}").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: 'https://www.example2.com'}) - resolver = proc { [public_ipv4] } + stub_request(:get, 'https://www.example.com').to_return(status: 301, headers: {location: 'https://www.example2.com'}) response = described_class.get( 'https://www.example.com', - resolver: resolver, allow_unfollowed_redirects: true, max_redirects: 0 ) @@ -476,23 +448,15 @@ def inject_custom_trust_store(*certificates) end it 'fails if the redirected url is not in the scheme whitelist' do - stub_request(:put, "https://#{public_ipv4}").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: 'ftp://www.example.com'}) - resolver = proc { [public_ipv4] } + stub_request(:put, 'https://www.example.com').to_return(status: 301, headers: {location: 'ftp://www.example.com'}) expect do - described_class.put('https://www.example.com', resolver: resolver) + described_class.put('https://www.example.com') end.to raise_error(described_class::InvalidUriScheme) end it 'fails if the redirected url has no public ip address' do - stub_request(:delete, "https://#{public_ipv4}").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: 'https://www.example2.com'}) - resolver = proc do |hostname| - [{ - 'www.example.com' => public_ipv4, - 'www.example2.com' => private_ipv6 - }[hostname]] - end + stub_request(:delete, 'https://www.example.com').to_return(status: 301, headers: {location: 'https://www.example2.com'}) + resolver = proc { [private_ipv6] } expect do described_class.delete('https://www.example.com', resolver: resolver) end.to raise_error(described_class::PrivateIPAddress) @@ -512,32 +476,18 @@ def inject_custom_trust_store(*certificates) end it 'follows redirects and succeed on a public hostname' do - stub_request(:post, "https://#{public_ipv4}/path?key=value").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: 'https://www.example2.com/path2?key2=value2'}) - stub_request(:post, "https://[#{public_ipv6}]/path2?key2=value2") - .with(headers: {host: 'www.example2.com'}).to_return(status: 200, body: 'response body') - resolver = proc do |hostname| - [{ - 'www.example.com' => public_ipv4, - 'www.example2.com' => public_ipv6 - }[hostname]] - end - response = described_class.post('https://www.example.com/path?key=value', resolver: resolver) + stub_request(:post, 'https://www.example.com/path?key=value').to_return(status: 301, headers: {location: 'https://www.example2.com/path2?key2=value2'}) + stub_request(:post, 'https://www.example2.com/path2?key2=value2').to_return(status: 200, body: 'response body') + response = described_class.post('https://www.example.com/path?key=value') expect(response.code).to eq('200') expect(response.body).to eq('response body') end it 'follows relative redirects and succeed' do - stub_request(:post, "https://#{public_ipv4}/path?key=value").with(headers: {host: 'www.example.com'}) - .to_return(status: 301, headers: {location: '/path2?key2=value2'}) - stub_request(:post, "https://#{public_ipv4}/path2?key2=value2") - .with(headers: {host: 'www.example.com'}).to_return(status: 200, body: 'response body') - resolver = proc do |hostname| - [{ - 'www.example.com' => public_ipv4 - }[hostname]] - end - response = described_class.post('https://www.example.com/path?key=value', resolver: resolver) + stub_request(:post, 'https://www.example.com/path?key=value').to_return(status: 301, + headers: {location: '/path2?key2=value2'}) + stub_request(:post, 'https://www.example.com/path2?key2=value2').to_return(status: 200, body: 'response body') + response = described_class.post('https://www.example.com/path?key=value') expect(response.code).to eq('200') expect(response.body).to eq('response body') end diff --git a/ssrf_filter.gemspec b/ssrf_filter.gemspec index 476e2c6..62ffaa5 100644 --- a/ssrf_filter.gemspec +++ b/ssrf_filter.gemspec @@ -8,7 +8,7 @@ Gem::Specification.new do |gem| gem.platform = Gem::Platform::RUBY gem.version = SsrfFilter::VERSION gem.authors = ['Arkadiy Tetelman'] - gem.required_ruby_version = '>= 2.6.0' + gem.required_ruby_version = '>= 2.7.0' gem.summary = 'A gem that makes it easy to prevent server side request forgery (SSRF) attacks' gem.description = gem.summary gem.homepage = 'https://github.com/arkadiyt/ssrf_filter' @@ -16,13 +16,14 @@ Gem::Specification.new do |gem| gem.files = Dir['lib/**/*.rb'] gem.metadata = {'rubygems_mfa_required' => 'true'} - gem.add_development_dependency('bundler-audit', '~> 0.9.1') + gem.add_development_dependency('base64', '~> 0.2.0') # For ruby >= 3.4 + gem.add_development_dependency('bundler-audit', '~> 0.9.2') gem.add_development_dependency('pry-byebug') - gem.add_development_dependency('rspec', '~> 3.12.0') - gem.add_development_dependency('rubocop', '~> 1.35.0') - gem.add_development_dependency('rubocop-rspec', '~> 2.12.1') + gem.add_development_dependency('rspec', '~> 3.13.0') + gem.add_development_dependency('rubocop', '~> 1.68.0') + gem.add_development_dependency('rubocop-rspec', '~> 3.2.0') gem.add_development_dependency('simplecov', '~> 0.22.0') gem.add_development_dependency('simplecov-lcov', '~> 0.8.0') - gem.add_development_dependency('webmock', '>= 3.18.0') + gem.add_development_dependency('webmock', '>= 3.24.0') gem.add_development_dependency('webrick') end