Merge pull request #82 from sharetribe/fix-insecure-open

Fix insecure open

CHANGELOG.md

@@ -20,6 +20,10 @@ This file follows the best practices from [keepachangelog.com](http://keepachang
 
 ### Security
 
+- [Critical] Fix OS command injection vulnerability in installations of
+  Sharetribe Go that do not set explicitly the `sns_notification_token`
+  configuration variable (which is unset by default).
+
 ## [10.2.0] - 2021-11-18
 
 ### Added

app/controllers/amazon_bounces_controller.rb

@@ -9,8 +9,11 @@ def notification
     amz_message_type = request.headers['x-amz-sns-message-type']
 
     if amz_message_type.to_s.downcase == 'subscriptionconfirmation'
-      send_subscription_confirmation request.raw_post
-      head :ok and return
+      if send_subscription_confirmation request.raw_post
+        head :ok and return
+      else
+        head :bad_request and return
+      end
     end
 
     if amz_message_type.to_s.downcase == 'notification'
@@ -34,10 +37,17 @@ def notification
   private
 
   def send_subscription_confirmation(request_body)
-    require 'open-uri'
     json = JSON.parse(request_body)
-    subscribe_url = json['SubscribeURL']
-    open(subscribe_url) # rubocop:disable Security/Open
+    subscribe_url = URI.parse(json['SubscribeURL'])
+    if ['http', 'https'].include?(subscribe_url.scheme)
+      subscribe_url.open
+    end
+  rescue URI::InvalidURIError => e
+    logger.info "SES send_subscription_confirmation: URI::InvalidURIError: #{e.message}"
+    return
+  rescue StandardError => e
+    logger.info "SES send_subscription_confirmation: #{e.message}"
+    return
   end
 
   def handle_bounces(msg)
@@ -62,7 +72,7 @@ def handle_complaints(msg)
   end
 
   def check_sns_token
-    if APP_CONFIG.sns_notification_token != params['sns_notification_token']
+    if APP_CONFIG.sns_notification_token.nil? || APP_CONFIG.sns_notification_token != params['sns_notification_token']
       return head :ok
     end
   end

spec/requests/amazon_bounces_spec.rb

@@ -1,13 +1,85 @@
 require 'spec_helper'
+require 'tempfile'
 
 describe "Amazon Bounces", type: :request do
 
   before(:each) do
     @community = FactoryGirl.create(:community, :domain => "market.custom.org")
   end
 
-  describe "test notifications" do
+  describe "subscription confirmations" do
+    context "sns_notification_token is unset" do
+      before(:all) do
+        @sns_notification_token = APP_CONFIG.sns_notification_token
+        APP_CONFIG.sns_notification_token = nil
+      end
+
+      after(:all) do
+        APP_CONFIG.sns_notification_token = @sns_notification_token
+      end
+
+      it 'does not open subscription notification url' do
+        incoming_data = JSON.parse('{
+            "SubscribeURL":"https://subscribe.example.com/confirm"
+          }')
+
+        stub_request(:get, "https://subscribe.example.com/confirm").to_return(status: 200, body: "", headers: {})
+
+        post "https://market.custom.org/bounces?sns_notification_token=#{APP_CONFIG.sns_notification_token}", params: incoming_data.to_json.to_s, headers: { 'x-amz-sns-message-type' => 'subscriptionconfirmation'}
+
+        expect(response.status).to eq(200)
+
+        assert_not_requested :get, "https://subscribe.example.com/confirm"
+      end
+    end
+
+    context "sns_notification_token is set" do
+      it 'opens valid subscription notification url' do
+        incoming_data = JSON.parse('{
+            "SubscribeURL":"https://subscribe.example.com/confirm"
+          }')
+
+        stub_request(:get, "https://subscribe.example.com/confirm").to_return(status: 200, body: "", headers: {})
 
+        post "https://market.custom.org/bounces?sns_notification_token=#{APP_CONFIG.sns_notification_token}", params: incoming_data.to_json.to_s, headers: { 'x-amz-sns-message-type' => 'subscriptionconfirmation'}
+        expect(response.status).to eq(200)
+        assert_requested :get, "https://subscribe.example.com/confirm"
+      end
+
+      it 'does not open non-http schemes' do
+        incoming_data = JSON.parse('{
+            "SubscribeURL":"file://foo"
+          }')
+
+        post "https://market.custom.org/bounces?sns_notification_token=#{APP_CONFIG.sns_notification_token}", params: incoming_data.to_json.to_s, headers: { 'x-amz-sns-message-type' => 'subscriptionconfirmation'}
+        expect(response.status).to eq(400)
+      end
+
+      context "code_execution" do
+        before(:all) do
+          @tmp_file = Tempfile.new("ses_test")
+        end
+
+        after(:all) do
+          @tmp_file.close!
+        end
+
+        it 'does not allow code execution' do
+          incoming_data = JSON.parse("{
+              \"SubscribeURL\":\"| echo foo >  #{@tmp_file.path}\"
+            }")
+
+          post "https://market.custom.org/bounces?sns_notification_token=#{APP_CONFIG.sns_notification_token}", params: incoming_data.to_json.to_s, headers: { 'x-amz-sns-message-type' => 'subscriptionconfirmation'}
+          expect(response.status).to eq(400)
+          # Need to allow some time for the execution to happen, it seems
+          sleep 2
+          expect(@tmp_file.size()).to eq(0)
+        end
+      end
+    end
+  end
+
+  describe "test notifications" do
     it "when notificationType is bounce" do
       # Prepare
       @person = FactoryGirl.create(:person, id: "123abc", min_days_between_community_updates: 4)