SafeWebhook - Take 2

.codeclimate.yml

@@ -9,6 +9,9 @@ engines:
     enabled: true
   rubocop:
     enabled: true
+    exclude_fingerprints:
+      # Using #=== intentionally to do subnet masking
+      - d1afe90be49c43e85a76bfa58f637804
 ratings:
   paths:
   - "**.rb"

lib/cc/fixed_resolv.rb

@@ -0,0 +1,29 @@
+require "resolv-replace"
+
+module CC
+  class FixedResolv < Resolv::DNS
+    def self.enable!
+      new.tap do |instance|
+        Resolv::DefaultResolver.replace_resolvers([instance])
+      end
+    end
+
+    def initialize
+      @addresses = {}
+    end
+
+    def setaddress(name, address)
+      addresses[name] = address
+    end
+
+    def each_address(name)
+      if addresses.key?(name)
+        yield addresses.fetch(name)
+      end
+    end
+
+    private
+
+    attr_reader :addresses
+  end
+end

lib/cc/service/http.rb

@@ -1,5 +1,6 @@
 require "active_support/concern"
 require "cc/service/response_check"
+require "cc/service/safe_webhook"
 
 module CC::Service::HTTP
   extend ActiveSupport::Concern
@@ -38,10 +39,8 @@ def service_post_with_redirects(url, body = nil, headers = nil, &block)
   end
 
   def raw_get(url = nil, params = nil, headers = nil)
-    http.get do |req|
-      req.url(url) if url
+    http_method(:get, url, nil, headers) do |req|
       req.params.update(params) if params
-      req.headers.update(headers) if headers
       yield req if block_given?
     end
   end
@@ -54,6 +53,8 @@ def raw_post(url = nil, body = nil, headers = nil)
   def http_method(method, url = nil, body = nil, headers = nil)
     block = Proc.new if block_given?
 
+    CC::Service::SafeWebhook.ensure_safe!(url)
+
     http.send(method) do |req|
       req.url(url) if url
       req.headers.update(headers) if headers

lib/cc/service/safe_webhook.rb

@@ -0,0 +1,69 @@
+require "ipaddr"
+require "uri"
+
+require "cc/fixed_resolv"
+
+module CC
+  class Service
+    class SafeWebhook
+      InternalWebhookError = Class.new(StandardError)
+
+      PRIVATE_ADDRESS_SUBNETS = [
+        IPAddr.new("10.0.0.0/8"),
+        IPAddr.new("172.16.0.0/12"),
+        IPAddr.new("192.168.0.0/16"),
+        IPAddr.new("fd00::/8"),
+        IPAddr.new("127.0.0.1"),
+        IPAddr.new("0:0:0:0:0:0:0:1"),
+      ].freeze
+
+      def self.ensure_safe!(url)
+        instance = new(url)
+        instance.ensure_safe!
+      end
+
+      def self.getaddress(host)
+        @dns ||= Resolv::DNS.new
+        @dns.getaddress(host)
+      end
+
+      def self.setaddress(host, address)
+        @fixed_resolv ||= CC::FixedResolv.enable!
+        @fixed_resolv.setaddress(host, address)
+      end
+
+      def initialize(url)
+        @url = url
+      end
+
+      def ensure_safe!
+        uri = URI.parse(url)
+
+        if !allow_internal_webhooks? && internal?(uri.host)
+          raise InternalWebhookError, "#{url.inspect} maps to an internal address"
+        end
+      end
+
+      private
+
+      attr_reader :url
+
+      def internal?(host)
+        address = self.class.getaddress(host)
+
+        self.class.setaddress(host, address)
+
+        PRIVATE_ADDRESS_SUBNETS.any? do |subnet|
+          subnet === IPAddr.new(address.to_s)
+        end
+      rescue Resolv::ResolvError
+        true # localhost
+      end
+
+      def allow_internal_webhooks?
+        var = ENV["CODECLIMATE_ALLOW_INTERNAL_WEBHOOKS"] || ""
+        var == "1" || var == "true"
+      end
+    end
+  end
+end

spec/cc/service/safe_webhook_spec.rb

@@ -0,0 +1,41 @@
+require "spec_helper"
+
+class CC::Service
+  describe SafeWebhook do
+    describe ".ensure_safe!" do
+      it "does not allow internal URLs" do
+        %w[ 127.0.0.1 192.168.0.1 10.0.1.18 ].each do |address|
+          stub_resolv("github.com", address)
+
+          expect do
+            SafeWebhook.ensure_safe!("https://github.com/api/v1/user")
+          end.to raise_error(SafeWebhook::InternalWebhookError)
+        end
+      end
+
+      it "allows internal URLs when configured to do so" do
+        allow(ENV).to receive(:[]).
+          with("CODECLIMATE_ALLOW_INTERNAL_WEBHOOKS").
+          and_return("1")
+
+        stub_resolv("github.com", "10.0.1.18")
+
+        SafeWebhook.ensure_safe!("https://github.com/api/v1/user")
+      end
+
+      it "allows non-internal URLs" do
+        stub_resolv("github.com", "1.1.1.2")
+
+        SafeWebhook.ensure_safe!("https://github.com/api/v1/user")
+      end
+
+      it "ensures future dns queries get the same answer" do
+        stub_resolv("github.com", "1.1.1.3")
+
+        SafeWebhook.ensure_safe!("https://github.com/api/v1/user")
+
+        expect(Resolv.getaddress("github.com").to_s).to eq "1.1.1.3"
+      end
+    end
+  end
+end

spec/cc/service_spec.rb

@@ -32,42 +32,42 @@
   it "post success" do
     stub_http("/my/test/url", [200, {}, '{"ok": true, "thing": "123"}'])
 
-    response = service_post("/my/test/url", { token: "1234" }.to_json, {}) do |inner_response|
+    response = service_post("http://example.com/my/test/url", { token: "1234" }.to_json, {}) do |inner_response|
       body = JSON.parse(inner_response.body)
       { thing: body["thing"] }
     end
 
     expect(response[:ok]).to eq(true)
     expect(response[:params]).to eq('{"token":"1234"}')
-    expect(response[:endpoint_url]).to eq("/my/test/url")
+    expect(response[:endpoint_url]).to eq("http://example.com/my/test/url")
     expect(response[:status]).to eq(200)
   end
 
   it "post redirect success" do
-    stub_http("/my/test/url", [307, { "Location" => "/my/redirect/url" }, '{"ok": false, "redirect": true}'])
+    stub_http("/my/test/url", [307, { "Location" => "http://example.com/my/redirect/url" }, '{"ok": false, "redirect": true}'])
     stub_http("/my/redirect/url", [200, {}, '{"ok": true, "thing": "123"}'])
 
-    response = service_post_with_redirects("/my/test/url", { token: "1234" }.to_json, {}) do |inner_response|
+    response = service_post_with_redirects("http://example.com/my/test/url", { token: "1234" }.to_json, {}) do |inner_response|
       body = JSON.parse(inner_response.body)
       { thing: body["thing"] }
     end
 
     expect(response[:ok]).to eq(true)
     expect(response[:params]).to eq('{"token":"1234"}')
-    expect(response[:endpoint_url]).to eq("/my/test/url")
+    expect(response[:endpoint_url]).to eq("http://example.com/my/test/url")
     expect(response[:status]).to eq(200)
   end
 
   it "post http failure" do
     stub_http("/my/wrong/url", [404, {}, ""])
 
-    expect { service_post("/my/wrong/url", { token: "1234" }.to_json, {}) }.to raise_error(CC::Service::HTTPError)
+    expect { service_post("http://example.com/my/wrong/url", { token: "1234" }.to_json, {}) }.to raise_error(CC::Service::HTTPError)
   end
 
   it "post some other failure" do
     stub_http("/my/wrong/url") { raise ArgumentError, "lol" }
 
-    expect { service_post("/my/wrong/url", { token: "1234" }.to_json, {}) }.to raise_error(ArgumentError)
+    expect { service_post("http://example.com/my/wrong/url", { token: "1234" }.to_json, {}) }.to raise_error(ArgumentError)
   end
 
   it "services" do

spec/spec_helper.rb

@@ -36,4 +36,8 @@
   # This setting enables warnings. It's recommended, but in some cases may
   # be too noisy due to issues in dependencies.
   config.warnings = true
+  config.before do
+    # Disable actual DNS resolution during specs by default
+    stub_resolv(anything, "1.1.1.1")
+  end
 end

spec/support/resolv_helpers.rb

@@ -0,0 +1,10 @@
+module ResolvHelpers
+  def stub_resolv(name, address)
+    allow(CC::Service::SafeWebhook).to receive(:getaddress).
+      with(name).and_return(Resolv::IPv4.create(address))
+  end
+end
+
+RSpec.configure do |conf|
+  conf.include(ResolvHelpers)
+end