From 73e7f0ffb61e650575b226c5eb6e0b091c5935bc Mon Sep 17 00:00:00 2001 From: patrick brisbin Date: Wed, 28 Sep 2016 12:30:29 -0400 Subject: [PATCH] SafeWebhook This implements a check that outgoing webhooks aren't making connections to internal services. It does this by resolving the webhook's host and checking it against internal subnet masks. Crucially, it must also adjust the default `Resolv` behavior to ensure that the eventual connection gets the same IP address as the one we've verified as non-internal. All of this is override-able via ENV, since an on-prem installation will want to intentionally target internal services with their webhooks. --- .codeclimate.yml | 3 ++ lib/cc/fixed_resolv.rb | 29 ++++++++++++ lib/cc/service/http.rb | 7 +-- lib/cc/service/safe_webhook.rb | 69 ++++++++++++++++++++++++++++ spec/cc/service/safe_webhook_spec.rb | 41 +++++++++++++++++ spec/cc/service_spec.rb | 14 +++--- spec/spec_helper.rb | 4 ++ spec/support/resolv_helpers.rb | 10 ++++ 8 files changed, 167 insertions(+), 10 deletions(-) create mode 100644 lib/cc/fixed_resolv.rb create mode 100644 lib/cc/service/safe_webhook.rb create mode 100644 spec/cc/service/safe_webhook_spec.rb create mode 100644 spec/support/resolv_helpers.rb diff --git a/.codeclimate.yml b/.codeclimate.yml index b74b36e..7027358 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -9,6 +9,9 @@ engines: enabled: true rubocop: enabled: true + exclude_fingerprints: + # Using #=== intentionally to do subnet masking + - d1afe90be49c43e85a76bfa58f637804 ratings: paths: - "**.rb" diff --git a/lib/cc/fixed_resolv.rb b/lib/cc/fixed_resolv.rb new file mode 100644 index 0000000..7ce21bc --- /dev/null +++ b/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 diff --git a/lib/cc/service/http.rb b/lib/cc/service/http.rb index 4f7f27f..f0d163c 100644 --- a/lib/cc/service/http.rb +++ b/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 diff --git a/lib/cc/service/safe_webhook.rb b/lib/cc/service/safe_webhook.rb new file mode 100644 index 0000000..de0490b --- /dev/null +++ b/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 diff --git a/spec/cc/service/safe_webhook_spec.rb b/spec/cc/service/safe_webhook_spec.rb new file mode 100644 index 0000000..b7094da --- /dev/null +++ b/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 diff --git a/spec/cc/service_spec.rb b/spec/cc/service_spec.rb index 6cffebb..50fa135 100644 --- a/spec/cc/service_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 14e39a6..773cb4e 100644 --- a/spec/spec_helper.rb +++ b/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 diff --git a/spec/support/resolv_helpers.rb b/spec/support/resolv_helpers.rb new file mode 100644 index 0000000..b035ab3 --- /dev/null +++ b/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