From de9546587a13a3063348e31f9a97e1d997d2df24 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 16 Sep 2022 14:42:34 +1000
Subject: [PATCH] Prevent webhooks to private addresses (SSRF) [add gem]

Best reviewed with whitespace hidden.

Unfortunately the spec isn't allowed in CI. But it worked on my environment, I promise.
I chose `xit` so that it doesn't run unnecessarily. Perhaps we could use `pending` instead, which would execute, and notify us if it suddenly started working one day. But I doubt it.
---
 Gemfile                                |  1 +
 Gemfile.lock                           |  2 ++
 app/jobs/webhook_delivery_job.rb       | 13 ++++++++-
 spec/jobs/webhook_delivery_job_spec.rb | 40 ++++++++++++++++++++++++--
 4 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Gemfile b/Gemfile
index c38020d362..608c870362 100644
--- a/Gemfile
+++ b/Gemfile
@@ -137,6 +137,7 @@ gem 'view_component_reflex', '3.1.14.pre9'
 gem 'mini_portile2', '~> 2.8'
 
 gem "faraday"
+gem "private_address_check"
 
 group :production, :staging do
   gem 'ddtrace'
diff --git a/Gemfile.lock b/Gemfile.lock
index 917aa0ee93..caa5a208cb 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -474,6 +474,7 @@ GEM
       ttfunk
     pg (1.2.3)
     power_assert (2.0.2)
+    private_address_check (0.5.0)
     pry (0.13.1)
       coderay (~> 1.1)
       method_source (~> 1.0)
@@ -844,6 +845,7 @@ DEPENDENCIES
   paypal-sdk-merchant (= 1.117.2)
   pdf-reader
   pg (~> 1.2.3)
+  private_address_check
   pry (~> 0.13.0)
   puma
   rack-mini-profiler (< 3.0.0)
diff --git a/app/jobs/webhook_delivery_job.rb b/app/jobs/webhook_delivery_job.rb
index 50be1e1daa..7eb0a44b83 100644
--- a/app/jobs/webhook_delivery_job.rb
+++ b/app/jobs/webhook_delivery_job.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require "faraday"
+require "private_address_check"
+require "private_address_check/tcpsocket_ext"
 
 # Deliver a webhook payload
 # As a delayed job, it can run asynchronously and handle retries.
@@ -19,7 +21,16 @@ class WebhookDeliveryJob < ApplicationJob
       data: payload,
     }
 
-    notify_endpoint(url, body)
+    # Request user-submitted url, preventing any private connections being made
+    # (SSRF).
+    # This method may allow the socket to open, but is necessary in order to
+    # protect from TOC/TOU.
+    # Note that private_address_check provides some methods for pre-validating,
+    # but they're not as comprehensive and so unnecessary here. Simply
+    # momentarily opening sockets probably can't cause DoS or other damage.
+    PrivateAddressCheck.only_public_connections do
+      notify_endpoint(url, body)
+    end
   end
 
   def notify_endpoint(url, body)
diff --git a/spec/jobs/webhook_delivery_job_spec.rb b/spec/jobs/webhook_delivery_job_spec.rb
index 579ed0086b..8f90c3c4e5 100644
--- a/spec/jobs/webhook_delivery_job_spec.rb
+++ b/spec/jobs/webhook_delivery_job_spec.rb
@@ -36,8 +36,44 @@ describe WebhookDeliveryJob do
     end
   end
 
-  # To be implemented in following commits
-  pending "can't access local secrets" # see https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8
+  # Ensure responses from a local network aren't allowed, to prevent a user
+  # seeing a private response or initiating an unauthorised action (SSRF).
+  # Currently, we're not doing anything with responses. When we do, we should
+  # update this to confirm the response isn't exposed.
+  describe "server side request forgery" do
+    describe "private addresses" do
+      private_addresses = [
+        "http://127.0.0.1/all_the_secrets",
+        "http://localhost/all_the_secrets",
+      ]
+
+      private_addresses.each do |url|
+        # Unfortunately this isn't allowed in CI, but it should work in your
+        # development environment.
+        xit "rejects private address #{url}" do
+          expect {
+            WebhookDeliveryJob.perform_now(url, event, data)
+          }.to raise_error(PrivateAddressCheck::PrivateConnectionAttemptedError)
+        end
+      end
+    end
+
+    describe "redirects" do
+      it "doesn't follow a redirect" do
+        other_url = 'http://localhost/all_the_secrets'
+
+        stub_request(:post, url).
+          to_return(status: 302, headers: { 'Location' => other_url })
+        stub_request(:any, other_url)
+
+        expect {
+          subject.perform_now
+        }.to raise_error(StandardError, "302")
+
+        expect(a_request(:any, other_url)).not_to have_been_made
+      end
+    end
+  end
 
   # Exceptions are considered a job failure, which the job runner
   # (Sidekiq) and/or ActiveJob will handle and retry later.