From 0f7b20ff39f2f155813510dc25f7b46074c6d34a Mon Sep 17 00:00:00 2001
From: Steve Kemp <steve@steve.org.uk>
Date: Mon, 19 Nov 2012 14:07:01 +0000
Subject:   Ensure that hostnames used for ping-tests are valid - to avoid the
 security hole.

---
 SECURITY                             | 20 +++++++++++++-------
 lib/custodian/parser.rb              | 10 ++++++++++
 lib/custodian/protocol-tests/ping.rb | 15 +++++++++++++++
 t/test-parser.rb                     | 20 ++++++++++++++++++++
 4 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/SECURITY b/SECURITY
index dc1c90a..2faf51f 100644
--- a/SECURITY
+++ b/SECURITY
@@ -18,21 +18,27 @@ Two tests pass arguments from the configuration file to the shell:
     ping
     http/https
 
-The hostname used to ping, and the URL for web-tests, are both passed directly to the shell with no encoding or sanitizing.  The only issue is that the hostnames must match the following regular expression:
+The hostname used to ping, and the URL for web-tests, are both passed directly to the shell assuming they match the following regular expression:
 
 ^([^\s]+)\s+
 
-The following configuration file allows the specified command to be executed, as the user running the dequeue tool, via the shell:
+So the following configuration file potentially allows a command to be executed by our worker:
 
     $(/home/steve/hg/custodian/exploit.sh) must ping otherwise "Owned".
 
-Given that anybody who can talk to the beanstalkd server can submit JSON-encoded-jobs we have no solution here which involves sanity-checking the parsed-hostnames.  Instead we much either restrict submissions to signed ones, or we must remove the following from hostnames:
+    http://$(/tmp/exploit.sh)/ must run http with status 200 otherwise "Owned".
 
-    $( ... )  - Expansion.
-    ` .. `    - Backticks.
-    ; ..      - Sub-commands.
+Given that anybody who can talk to the beanstalkd server can submit JSON-encoded-jobs we cannot rely on catching this solely in the parser.
 
-That has not yet been done, but it is definitely on the map.
+For the moment we've solved the case of the ping-exploitation, because the
+valid hostnames passed there are [a-z0-9.-].  We've not yet sanitized URLs
+because that is a harder job.
+
+In the case of the ping-test we've done both levels of testing:
+
+  * Test the hostname is valid priorer to executing the shell.
+
+  * Ensure the hostname is valid before adding the job to the queue.
 
 
 
diff --git a/lib/custodian/parser.rb b/lib/custodian/parser.rb
index 77adc48..ea25c3a 100644
--- a/lib/custodian/parser.rb
+++ b/lib/custodian/parser.rb
@@ -373,6 +373,16 @@ class MonitorConfig
           :timeout     => @timeout
         }
 
+        #
+        # Sanity check the hostname for ping-tests, to
+        # avoid this security hole:
+        #
+        #   $(/tmp/exploit.sh) must run ping ..
+        #
+        if ( service == "ping" )
+          raise ArgumentError, "Invalid hostname for ping-test: #{host}" unless( host =~ /^([a-zA-Z0-9:\-\.]+)$/ )
+        end
+
 
         #
         #  Alert text will have a default, which may be overridden.
diff --git a/lib/custodian/protocol-tests/ping.rb b/lib/custodian/protocol-tests/ping.rb
index fed72d4..3243b6b 100755
--- a/lib/custodian/protocol-tests/ping.rb
+++ b/lib/custodian/protocol-tests/ping.rb
@@ -72,6 +72,21 @@ class PINGTest
     #  Get the hostname to test against.
     #
     host = @test_data['target_host']
+
+
+    #
+    # Sanity check the hostname for ping-tests, to
+    # avoid this security hole:
+    #
+    #   $(/tmp/exploit.sh) must run ping ..
+    #
+    raise ArgumentError, "Invalid hostname for ping-test: #{host}" unless( host =~ /^([a-zA-Z0-9:\-\.]+)$/ )
+
+
+
+    #
+    # Show the hostname.
+    #
     puts "ping testing host #{host}" if ( @test_data['verbose'] )
 
 
diff --git a/t/test-parser.rb b/t/test-parser.rb
index a037448..cb9a9cb 100755
--- a/t/test-parser.rb
+++ b/t/test-parser.rb
@@ -608,4 +608,24 @@ class TestParser < Test::Unit::TestCase
     end
   end
 
+
+  #
+  # Test the potential security-hole for ping-tests
+  #
+  def test_ping_security_hole
+
+
+    parser = MonitorConfig.new("/dev/null" )
+
+    assert_raise ArgumentError do
+      parser.parse_line( "$(/tmp/exploit) must ping ." )
+    end
+
+    assert_nothing_raised do
+      parser.parse_line( "test.example.vm.bytemark.co.uk must ping ." )
+    end
+
+  end
+
+
 end
-- 
cgit v1.2.1