Merge pull request #183 from onelogin/AUTH-255

AUTH-255: Sanitize document.signed_element_id via a prepared statement

Author: Lordnibbler

Gemfile

@@ -1,21 +1,27 @@
+#
+# Please keep this file alphabetized and organized
+#
 source 'http://rubygems.org'
 
 gemspec
 
 group :test do
-  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"
+  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 "byebug",   "~> 2.1.1"
+    gem 'pry-byebug'
   end
-  gem "shoulda",    "~> 2.11"
-  gem "rake",       "~> 10"
-  gem "mocha",      "~> 0.14",  :require => false
-  gem "timecop",    "<= 0.6.0"
-  gem "systemu",    "~> 2"
-  gem "rspec",      "~> 2"
+
+  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

@@ -184,8 +184,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

@@ -332,7 +332,7 @@ class RubySamlTest < Test::Unit::TestCase
           OneLogin::RubySaml::Attributes.single_value_compatibility = false
           assert_equal nil, response.attributes[:attribute_not_exists]
           assert_equal nil, response.attributes.single(:attribute_not_exists)
-          assert_equal nil, response.attributes.multi(:attribute_not_exists)          
+          assert_equal nil, response.attributes.multi(:attribute_not_exists)
           OneLogin::RubySaml::Attributes.single_value_compatibility = true
         end
 
@@ -368,5 +368,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>