diff options
| -rw-r--r-- | SECURITY | 20 | ||||
| -rw-r--r-- | lib/custodian/parser.rb | 10 | ||||
| -rwxr-xr-x | lib/custodian/protocol-tests/ping.rb | 15 | ||||
| -rwxr-xr-x | t/test-parser.rb | 20 |
4 files changed, 58 insertions, 7 deletions
@@ -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 |