Land rapid7#2137, fixes and specs for Opt containers

wvu · wvu · commit d4933466912e · 2013-07-23T15:58:09.000-05:00
diff --git a/lib/msf/core/option_container.rb b/lib/msf/core/option_container.rb
@@ -173,6 +173,7 @@ def normalize(value)
 
 	def valid?(value=self.value)
 		value = normalize(value)
+    return false unless value.kind_of?(String) or value.kind_of?(NilClass)
 		return false if empty_required_value?(value)
 		return super
 	end
@@ -263,6 +264,7 @@ def type
 
 	def valid?(value=self.value)
 		return false if empty_required_value?(value)
+    return true if value.nil? and !required?
 
 		(value and self.enums.include?(value.to_s))
 	end
@@ -330,10 +332,17 @@ def type
 
 	def valid?(value)
 		return false if empty_required_value?(value)
+    return false unless value.kind_of?(String) or value.kind_of?(NilClass)
 
 		if (value != nil and value.empty? == false)
 			begin
-				::Rex::Socket.getaddress(value, true)
+				getaddr_result = ::Rex::Socket.getaddress(value, true)
+        # Covers a wierdcase where an incomplete ipv4 address will have it's
+        # missing octets filled in  with 0's. (e.g 192.168 become 192.0.0.168)
+        # which does not feel like a legit behaviour
+        if value =~ /^\d{1,3}(\.\d{1,3}){1,3}$/
+          return false unless value =~ Rex::Socket::MATCH_IPV4
+        end
 			rescue
 				return false
 			end
@@ -354,6 +363,7 @@ def type
 	end
 
 	def normalize(value)
+    return nil unless value.kind_of?(String)
 		if (value =~ /^file:(.*)/)
 			path = $1
 			return false if not File.exists?(path) or File.directory?(path)
@@ -373,9 +383,12 @@ def normalize(value)
 
 	def valid?(value)
 		return false if empty_required_value?(value)
+    return false unless value.kind_of?(String) or value.kind_of?(NilClass)
 
 		if (value != nil and value.empty? == false)
-			walker = Rex::Socket::RangeWalker.new(normalize(value))
+      normalized = normalize(value)
+      return false if normalized.nil?
+			walker = Rex::Socket::RangeWalker.new(normalized)
 			if (not walker or not walker.valid?)
 				return false
 			end
@@ -474,7 +487,7 @@ def valid?(value)
 			Regexp.compile(value)
 
 			return true
-		rescue RegexpError
+		rescue RegexpError, TypeError
 			return false
 		end
 	end
diff --git a/lib/rex/socket/range_walker.rb b/lib/rex/socket/range_walker.rb
@@ -95,9 +95,12 @@ def parse(parseme)
 				return false if ip_part.nil? or ip_part.empty? or mask_part.nil? or mask_part.empty?
 				return false if mask_part !~ /^[0-9]{1,2}$/ # Illegal mask -- numerals only
 				return false if mask_part.to_i > 32 # This too -- between 0 and 32.
+        if ip_part =~ /^\d{1,3}(\.\d{1,3}){1,3}$/
+          return false unless ip_part =~ Rex::Socket::MATCH_IPV4
+        end
 				begin
-					Rex::Socket.addr_atoi(ip_part) # This allows for "www.metasploit.com/24" which is fun.
-				rescue Resolv::ResolvError
+					Rex::Socket.getaddress(ip_part) # This allows for "www.metasploit.com/24" which is fun.
+				rescue Resolv::ResolvError, ::SocketError, Errno::ENOENT
 					return false # Can't resolve the ip_part, so bail.
 				end
 
diff --git a/spec/file_fixtures/short_address_list.txt b/spec/file_fixtures/short_address_list.txt
@@ -0,0 +1,5 @@
+192.168.1.1
+192.168.1.2
+192.168.1.3
+192.168.1.4
+192.168.1.5
diff --git a/spec/file_fixtures/string_list.txt b/spec/file_fixtures/string_list.txt
@@ -0,0 +1,3 @@
+foo
+bar
+baz
diff --git a/spec/lib/msf/core/options/opt_address_range_spec.rb b/spec/lib/msf/core/options/opt_address_range_spec.rb
@@ -11,27 +11,49 @@
 		{ :value => "192.0.2.0,1-255", :normalized => "192.0.2.0,1-255" },
 		{ :value => "192.0.2.*",       :normalized => "192.0.2.*" },
 		{ :value => "192.0.2.0-192.0.2.255", :normalized => "192.0.2.0-192.0.2.255" },
+    { :value => "file:#{File.expand_path('short_address_list.txt',FILE_FIXTURES_PATH)}", :normalized => '192.168.1.1 192.168.1.2 192.168.1.3 192.168.1.4 192.168.1.5'},
 	]
 	invalid_values = [
 		# Too many dots
 		{ :value => "192.0.2.0.0" },
 		{ :value => "192.0.2.0.0,1" },
 		{ :value => "192.0.2.0.0,1-2" },
-		{ :pending => "Redmine #7536", :value => "192.0.2.0.0/24" },
+		{ :value => "192.0.2.0.0/24" },
 		# Not enough dots
 		{ :value => "192.0.2" },
 		{ :value => "192.0.2,1" },
 		{ :value => "192.0.2,1-2" },
-		{ :pending => "Redmine #7536", :value => "192.0.2/24" },
+		{ :value => "192.0.2/24" },
 		# Can't mix ranges and CIDR
 		{ :value => "192.0.2.0,1/24" },
 		{ :value => "192.0.2.0-1/24" },
 		{ :value => "192.0.2.0,1-2/24" },
 		{ :value => "192.0.2.0/1-24" },
-		{ :value => "192.0.2.0-192.0.2.1-255", },
+		{ :value => "192.0.2.0-192.0.2.1-255" },
+    # Non-string values
+    { :value => true},
+    { :value => 5 },
+    { :value => []},
+    { :value => [1,2]},
+    { :value => {}},
 	]
 
-	it_behaves_like "an option", valid_values, invalid_values
+	it_behaves_like "an option", valid_values, invalid_values, 'addressrange'
+
+  let(:required_opt) {  Msf::OptAddressRange.new('RHOSTS', [true, 'The target addresses', '']) }
+
+  context 'the normalizer' do
+    it 'should handle a call for random IPs' do
+      random_addresses = required_opt.normalize('rand:5')
+      random_addresses.kind_of?(String).should == true
+      ips = random_addresses.split(' ')
+      ips.count.should == 5
+      ips.each do |ip|
+        (ip =~ Rex::Socket::MATCH_IPV4).should == 0
+      end
+    end
+  end
+
 end
 
 
diff --git a/spec/lib/msf/core/options/opt_address_spec.rb b/spec/lib/msf/core/options/opt_address_spec.rb
@@ -13,10 +13,19 @@
 		# Too many dots
 		{ :value => "192.0.2.0.0" },
 		# Not enough
-    { :pending => "Redmine #7537", :value => "192.0.2" }
+    { :value => "192.0.2" },
+    # Non-string values
+    { :value => true},
+    { :value => 5 },
+    { :value => []},
+    { :value => [1,2]},
+    { :value => {}},
 	]
 
-	it_behaves_like "an option", valid_values, invalid_values
+	it_behaves_like "an option", valid_values, invalid_values, 'address'
+
+
+
 end
 
 
diff --git a/spec/lib/msf/core/options/opt_bool_spec.rb b/spec/lib/msf/core/options/opt_bool_spec.rb
@@ -17,6 +17,7 @@
     { :value => "012"     },
     { :value => "123"     },
   ]
-  it_behaves_like "an option", valid_values, invalid_values
+  it_behaves_like "an option", valid_values, invalid_values, 'bool'
+
 end
 
diff --git a/spec/lib/msf/core/options/opt_enum_spec.rb b/spec/lib/msf/core/options/opt_enum_spec.rb
@@ -0,0 +1,23 @@
+# -*- coding:binary -*-
+
+require 'spec_helper'
+require 'msf/core/option_container'
+
+describe Msf::OptEnum do
+
+  it_behaves_like "an option", [], [], 'enum'
+
+  subject do
+    Msf::OptEnum.new('name',[true, 'A Boolean Value', 'Foo', ['Foo', 'Bar', 'Baz']])
+  end
+
+  context 'the validator' do
+    it 'should return false for a value not in the list' do
+      subject.valid?('Snap').should == false
+    end
+
+    it 'should return true for a value in the list' do
+      subject.valid?('Bar').should == true
+    end
+  end
+end
diff --git a/spec/lib/msf/core/options/opt_int_spec.rb b/spec/lib/msf/core/options/opt_int_spec.rb
@@ -21,7 +21,7 @@
     { :value => "FF",      },
   ]
 
-  it_behaves_like "an option", valid_values, invalid_values
+  it_behaves_like "an option", valid_values, invalid_values, 'integer'
 end
 
 
diff --git a/spec/lib/msf/core/options/opt_path_spec.rb b/spec/lib/msf/core/options/opt_path_spec.rb
@@ -15,6 +15,6 @@
     { :value => "$",    },
   ]
 
-  it_behaves_like "an option", valid_values, invalid_values
+  it_behaves_like "an option", valid_values, invalid_values, 'path'
 end
 
diff --git a/spec/lib/msf/core/options/opt_port_spec.rb b/spec/lib/msf/core/options/opt_port_spec.rb
@@ -16,6 +16,6 @@
     { :value => "65536",   },
   ]
 
-  it_behaves_like "an option", valid_values, invalid_values
+  it_behaves_like "an option", valid_values, invalid_values, 'port'
 end
 
diff --git a/spec/lib/msf/core/options/opt_raw_spec.rb b/spec/lib/msf/core/options/opt_raw_spec.rb
@@ -0,0 +1,15 @@
+# -*- coding:binary -*-
+
+require 'spec_helper'
+require 'msf/core/option_container'
+
+describe Msf::OptRaw do
+
+  valid_values = [
+      { :value => 'foo',    :normalized => 'foo'     },
+      { :value => "file:#{File.expand_path('string_list.txt',FILE_FIXTURES_PATH)}",:normalized => "foo\nbar\nbaz" },
+  ]
+  invalid_values = []
+
+  it_behaves_like "an option", valid_values, invalid_values, 'raw'
+end
diff --git a/spec/lib/msf/core/options/opt_regexp_spec.rb b/spec/lib/msf/core/options/opt_regexp_spec.rb
@@ -0,0 +1,17 @@
+# -*- coding:binary -*-
+
+require 'spec_helper'
+require 'msf/core/option_container'
+
+describe Msf::OptRegexp do
+
+  valid_values = [
+      { :value => '^foo$',    :normalized => /^foo$/ },
+  ]
+  invalid_values = [
+      { :value => 123 },
+      { :value => 'foo('}
+  ]
+
+  it_behaves_like "an option", valid_values, invalid_values, 'regexp'
+end
diff --git a/spec/lib/msf/core/options/opt_string_spec.rb b/spec/lib/msf/core/options/opt_string_spec.rb
@@ -0,0 +1,21 @@
+# -*- coding:binary -*-
+
+require 'spec_helper'
+require 'msf/core/option_container'
+
+describe Msf::OptString do
+  valid_values = [
+      { :value => 'foo',    :normalized => 'foo'     },
+      { :value => "file:#{File.expand_path('string_list.txt',FILE_FIXTURES_PATH)}",:normalized => "foo\nbar\nbaz" },
+  ]
+  invalid_values = [
+      # Non-string values
+      { :value => true},
+      { :value => 5 },
+      { :value => []},
+      { :value => [1,2]},
+      { :value => {}},
+  ]
+
+  it_behaves_like "an option", valid_values, invalid_values, 'string'
+end
diff --git a/spec/lib/rex/socket/range_walker_spec.rb b/spec/lib/rex/socket/range_walker_spec.rb
@@ -28,6 +28,16 @@
     walker.should include("10.1.3.5")
   end
 
+  it 'should reject CIDR ranges with missing octets' do
+    walker = Rex::Socket::RangeWalker.new('192.168/24')
+    walker.should_not be_valid
+  end
+
+  it 'should reject a CIDR range with too many octets' do
+    walker = Rex::Socket::RangeWalker.new('192.168.1.2.0/24')
+    walker.should_not be_valid
+  end
+
   it "should default the lower bound of a range to 0" do
     walker = Rex::Socket::RangeWalker.new("10.1.3.-17")
     walker.should be_valid
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -3,6 +3,8 @@
 require 'bundler'
 Bundler.require(:default, :test, :db)
 
+FILE_FIXTURES_PATH = File.expand_path(File.dirname(__FILE__)) + "/file_fixtures/"
+
 # add project lib directory to load path
 spec_pathname = Pathname.new(__FILE__).dirname
 root_pathname = spec_pathname.join('..').expand_path
diff --git a/spec/support/shared/examples/options.rb b/spec/support/shared/examples/options.rb
@@ -1,10 +1,29 @@
 # -*- coding:binary -*-
 
-shared_examples_for "an option" do |valid_values, invalid_values|
+shared_examples_for "an option" do |valid_values, invalid_values, type|
   subject do
     described_class.new("name")
   end
 
+  let(:required) { described_class.new('name', [true, 'A description here'])}
+  let(:optional) { described_class.new('name', [false, 'A description here'])}
+
+  it "should return a type of #{type}"  do
+    subject.type.should == type
+  end
+
+  context 'when required' do
+    it 'should not be valid for nil' do
+      required.valid?(nil).should == false
+    end
+  end
+
+  context 'when not required' do
+    it 'it should be valid for nil' do
+      optional.valid?(nil).should == true
+    end
+  end
+
   context "with valid values" do
     valid_values.each do |vhash|
 			valid_value = vhash[:value]

Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@`
`17`	`17`	`{ :value => "012" },`
`18`	`18`	`{ :value => "123" },`
`19`	`19`	`]`
`20`		`- it_behaves_like "an option", valid_values, invalid_values`
	`20`	`+ it_behaves_like "an option", valid_values, invalid_values, 'bool'`
	`21`	`+`
`21`	`22`	`end`
`22`	`23`
Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@`
`21`	`21`	`{ :value => "FF", },`
`22`	`22`	`]`
`23`	`23`
`24`		`- it_behaves_like "an option", valid_values, invalid_values`
	`24`	`+ it_behaves_like "an option", valid_values, invalid_values, 'integer'`
`25`	`25`	`end`
`26`	`26`
`27`	`27`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,6 @@`
`15`	`15`	`{ :value => "$", },`
`16`	`16`	`]`
`17`	`17`
`18`		`- it_behaves_like "an option", valid_values, invalid_values`
	`18`	`+ it_behaves_like "an option", valid_values, invalid_values, 'path'`
`19`	`19`	`end`
`20`	`20`