Different approach to monkey-patching regexp (#70)

Dockerfile

@@ -1,6 +1,6 @@
 FROM ruby:3.0.0
 
-RUN apt update && apt-get install -y vim
+RUN apt update && apt-get install -y vim tmux tig
 
 WORKDIR app
 COPY Gemfile ssrf_filter.gemspec .

lib/ssrf_filter.rb

@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require 'ssrf_filter/patch/resolv'
 require 'ssrf_filter/patch/ssl_socket'
 require 'ssrf_filter/ssrf_filter'
 require 'ssrf_filter/version'

lib/ssrf_filter/patch/ssl_socket.rb

@@ -3,36 +3,36 @@
 class SsrfFilter
  module Patch
  module SSLSocket
- # 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.
-
  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] ||
@@ -45,6 +45,20 @@ def self.apply!
  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

lib/ssrf_filter/ssrf_filter.rb

@@ -84,7 +84,6 @@ def self.prefixlen_from_ipaddr(ipaddr)
  }.freeze
 
  FIBER_HOSTNAME_KEY = :__ssrf_filter_hostname
- FIBER_ADDRESS_KEY = :__ssrf_filter_address
 
  class Error < ::StandardError
  end
@@ -107,7 +106,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!
- ::SsrfFilter::Patch::Resolv.apply!
 
  original_url = url
  scheme_whitelist = options[:scheme_whitelist] || DEFAULT_SCHEME_WHITELIST
@@ -189,7 +187,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, ip) do
+ 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)
@@ -221,13 +219,11 @@ def self.validate_request(request)
  end
  private_class_method :validate_request
 
- def self.with_forced_hostname(hostname, ip, &_block)
+ def self.with_forced_hostname(hostname, &_block)
  ::Thread.current[FIBER_HOSTNAME_KEY] = hostname
- ::Thread.current[FIBER_ADDRESS_KEY] = ip
  yield
  ensure
  ::Thread.current[FIBER_HOSTNAME_KEY] = nil
- ::Thread.current[FIBER_ADDRESS_KEY] = nil
  end
  private_class_method :with_forced_hostname
 end

spec/lib/ssrf_filter_spec.rb

@@ -146,27 +146,21 @@
  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
- expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
- described_class.with_forced_hostname('test', '1.2.3.4') do
+ described_class.with_forced_hostname('test') 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_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_HOSTNAME_KEY]).to be_nil
- expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
  expect do
- described_class.with_forced_hostname('test', '1.2.3.4') do
+ described_class.with_forced_hostname('test') 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_HOSTNAME_KEY]).to be_nil
- expect(Thread.current[described_class::FIBER_ADDRESS_KEY]).to be_nil
  end
  end