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
SAML-Toolkits · Jan 26, 2015 · 4564638 · 4564638
1 parent 84c3a6f
commit 4564638
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 13 deletions.
diff --git a/Gemfile b/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
diff --git a/lib/onelogin/ruby-saml/response.rb b/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
 

diff --git a/test/response_test.rb b/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
diff --git a/test/responses/response_eval.xml b/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>
diff --git a/test/test_helper.rb b/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