From 0f08970de029ecea27ee740ef8dc79d1399adc71 Mon Sep 17 00:00:00 2001
From: Steve Kemp <steve@steve.org.uk>
Date: Mon, 19 Nov 2012 17:36:24 +0000
Subject:   Avoid using the shell for expansion when invoking curl - this fixes
   the potential security hole.

---
 SECURITY                  | 18 ++++++++++++------
 lib/custodian/webfetch.rb | 15 ++++++++++++++-
 2 files changed, 26 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 lib/custodian/webfetch.rb

diff --git a/SECURITY b/SECURITY
index 2faf51f..bbd44d0 100644
--- a/SECURITY
+++ b/SECURITY
@@ -30,17 +30,21 @@ So the following configuration file potentially allows a command to be executed
 
 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.
 
-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.
+For the moment we've solved the case of the ping-exploitation, by validating
+that hostnames passed to the multi-ping command match ^[a-z0-9.-]$ - both forms
+of input are validated:
 
-In the case of the ping-test we've done both levels of testing:
-
-  * Test the hostname is valid priorer to executing the shell.
+  * Ensuring the hostname is valid prior to executing the shell command.
 
   * Ensure the hostname is valid before adding the job to the queue.
 
 
+For HTTP-testing we're avoiding the shell by using the array-based invokation
+of the curl command.  We don't perform validation on the URL though, because
+that is a significantly harder jhob.
+
+
+
 
 General
 -------
@@ -55,6 +59,8 @@ Problem: We cannot sign the body without giving away our key details.
 Solution: Read /etc/custodian/salt, and store the checksum of all keys + values with that salt?
 
 
+
+
 TODO
 ----
 
diff --git a/lib/custodian/webfetch.rb b/lib/custodian/webfetch.rb
old mode 100644
new mode 100755
index 8948958..34052ca
--- a/lib/custodian/webfetch.rb
+++ b/lib/custodian/webfetch.rb
@@ -69,7 +69,20 @@ class WebFetch
     #
     # Shell out to curl (!!!) to do the fetch.
     #
-    system( "curl --max-time #{timeout} --silent --location --insecure --dump-header #{head} --out #{body} --silent #{@url}")
+    # Avoid using the actual shell to avoid a security risk
+    #
+    system( "curl",
+            "--max-time",
+            timeout.to_s,
+            "--silent",
+            "--location",
+            "--insecure",
+            "--dump-header",
+            head,
+            "--out",
+            body,
+            "--silent",
+            @url )
 
 
     #
-- 
cgit v1.2.1