Merge pull request #183 from onelogin/AUTH-255

AUTH-255: Sanitize document.signed_element_id via a prepared statement
Conflicts:
	Gemfile
	test/response_test.rb

Author: Lordnibbler

Gemfile

@@ -1,15 +1,27 @@
+#
+# Please keep this file alphabetized and organized
+#
 source 'http://rubygems.org'
 
 gemspec
 
 group :test do
-  gem "ruby-debug", "~> 0.10.4", :require => nil, :platforms => :ruby_18
-  gem "debugger",   "~> 1.1",    :require => nil, :platforms => :ruby_19
-  gem "shoulda",    "~> 2.11"
-  gem "rake",       "~> 10"
-  gem "mocha",      "~> 0.14"
-  gem "nokogiri",   "~> 1.5.0"
-  gem "timecop",    "<= 0.6.0"
-  gem "systemu",    "~> 2"
-  gem "rspec",      "~> 2"
+  if RUBY_VERSION < '1.9'
+    gem 'nokogiri',   '~> 1.5.0'
+    gem 'ruby-debug', '~> 0.10.4'
+  elsif RUBY_VERSION < '2.0'
+    gem 'debugger-linecache', '~> 1.2.0'
+    gem 'debugger', '~> 1.6.4'
+  elsif RUBY_VERSION < '2.1'
+    gem 'byebug',   '~> 2.1.1'
+  else
+    gem 'pry-byebug'
+  end
+
+  gem 'mocha',     '~> 0.14',  :require => false
+  gem 'rake',      '~> 10'
+  gem 'shoulda',   '~> 2.11'
+  gem 'systemu',   '~> 2'
+  gem 'test-unit', '~> 3'
+  gem 'timecop',   '<= 0.6.0'
 end

lib/onelogin/ruby-saml/response.rb

@@ -151,8 +151,18 @@ def validate_response_state(soft = true)
       end
 
       def xpath_first_from_signed_assertion(subelt=nil)
-        node = REXML::XPath.first(document, "/p:Response/a:Assertion[@ID='#{document.signed_element_id}']#{subelt}", { "p" => PROTOCOL, "a" => ASSERTION })
-        node ||= REXML::XPath.first(document, "/p:Response[@ID='#{document.signed_element_id}']/a:Assertion#{subelt}", { "p" => PROTOCOL, "a" => ASSERTION })
+        node = REXML::XPath.first(
+            document,
+            "/p:Response/a:Assertion[@ID=$id]#{subelt}",
+            { "p" => PROTOCOL, "a" => ASSERTION },
+            { 'id' => document.signed_element_id }
+        )
+        node ||= REXML::XPath.first(
+            document,
+            "/p:Response[@ID=$id]/a:Assertion#{subelt}",
+            { "p" => PROTOCOL, "a" => ASSERTION },
+            { 'id' => document.signed_element_id }
+        )
         node
       end
 

test/response_test.rb

@@ -254,5 +254,13 @@ class RubySamlTest < Test::Unit::TestCase
       end
     end
 
+    context '#xpath_first_from_signed_assertion' do
+      should 'not allow arbitrary code execution' do
+        malicious_response_document = fixture('response_eval', false)
+        response = OneLogin::RubySaml::Response.new(malicious_response_document)
+        response.send(:xpath_first_from_signed_assertion)
+        assert_equal($evalled, nil)
+      end
+    end
   end
 end

test/responses/response_eval.xml

@@ -0,0 +1,7 @@
+<saml:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:protocol">
+  <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+    <ds:SignedInfo>
+      <ds:Reference URI="#x'] or eval('$evalled = true') or /[@ID='v"/>
+    </ds:SignedInfo>
+  </ds:Signature>
+</saml:Response>

test/test_helper.rb

@@ -1,7 +1,6 @@
 require 'rubygems'
 require 'test/unit'
 require 'shoulda'
-require 'ruby-debug'
 require 'mocha/setup'
 require 'timecop'
 
@@ -63,7 +62,7 @@ def wrapped_response_2
   def signature_fingerprint_1
     @signature_fingerprint1 ||= "C5:19:85:D9:47:F1:BE:57:08:20:25:05:08:46:EB:27:F6:CA:B7:83"
   end
-  
+
   def signature_1
     @signature1 ||= File.read(File.join(File.dirname(__FILE__), 'certificates', 'certificate1'))
   end