Fixes latest CVEs

Per: - SAML-Toolkits#247 - SAML-Toolkits@9853651
amoose · Aug 4, 2015 · 32bbd4e · 32bbd4e
1 parent 801ad02
commit 32bbd4e
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 9 deletions.
diff --git a/lib/onelogin/ruby-saml/saml_message.rb b/lib/onelogin/ruby-saml/saml_message.rb
@@ -25,10 +25,17 @@ def self.schema
       end
 
       def valid_saml?(document, soft = true)
-        xml = Nokogiri::XML(document.to_s)
+        begin
+          xml = Nokogiri::XML(document.to_s) do |config|
+            config.options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
+          end
+        rescue Exception => error
+          return false if soft
+          validation_error("XML load failed: #{error.message}")
+        end
 
         SamlMessage.schema.validate(xml).map do |error|
-          break false if soft
+          return false if soft
           validation_error("#{error.message}\n\n#{xml.to_s}")
         end
       end

diff --git a/lib/xml_security.rb b/lib/xml_security.rb
@@ -34,9 +34,12 @@
 module XMLSecurity
 
   class BaseDocument < REXML::Document
+    REXML::Document::entity_expansion_limit = 0
 
-    C14N            = "http://www.w3.org/2001/10/xml-exc-c14n#"
-    DSIG            = "http://www.w3.org/2000/09/xmldsig#"
+    C14N              = "http://www.w3.org/2001/10/xml-exc-c14n#"
+    DSIG              = "http://www.w3.org/2000/09/xmldsig#"
+    NOKOGIRI_OPTIONS  = Nokogiri::XML::ParseOptions::STRICT |
+                        Nokogiri::XML::ParseOptions::NONET
 
     def canon_algorithm(element)
       algorithm = element
@@ -98,7 +101,9 @@ class Document < BaseDocument
       #<Object />
     #</Signature>
     def sign_document(private_key, certificate, signature_method = SHA1, digest_method = SHA1)
-      noko = Nokogiri.parse(self.to_s)
+      noko = Nokogiri.parse(self.to_s) do |options|
+        options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
+      end
       canon_doc = noko.canonicalize(canon_algorithm(C14N))
 
       signature_element = REXML::Element.new("ds:Signature").add_namespace('ds', DSIG)
@@ -120,7 +125,10 @@ def sign_document(private_key, certificate, signature_method = SHA1, digest_meth
       reference_element.add_element("ds:DigestValue").text = compute_digest(canon_doc, algorithm(digest_method_element))
 
       # add SignatureValue
-      noko_sig_element = Nokogiri.parse(signature_element.to_s)
+      noko_sig_element = Nokogiri.parse(signature_element.to_s) do |options|
+        options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
+      end
+
       noko_signed_info_element = noko_sig_element.at_xpath('//ds:Signature/ds:SignedInfo', 'ds' => DSIG)
       canon_string = noko_signed_info_element.canonicalize(canon_algorithm(C14N))
       signature = compute_signature(private_key, algorithm(signature_method).new, canon_string)
@@ -165,7 +173,10 @@ class SignedDocument < BaseDocument
     def initialize(response, errors = [])
       super(response)
       @errors = errors
-      extract_signed_element_id
+    end
+
+    def signed_element_id
+      @signed_element_id ||= extract_signed_element_id
     end
 
     def validate_document(idp_cert_fingerprint, soft = true)
@@ -215,7 +226,9 @@ def validate_signature(base64_cert, soft = true)
       # check for inclusive namespaces
       inclusive_namespaces = extract_inclusive_namespaces
 
-      document = Nokogiri.parse(self.to_s)
+      document = Nokogiri.parse(self.to_s) do |options|
+        options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
+      end
 
       # create a working copy so we don't modify the original
       @working_copy ||= REXML::Document.new(self.to_s).root
@@ -238,7 +251,7 @@ def validate_signature(base64_cert, soft = true)
       REXML::XPath.each(@sig_element, "//ds:Reference", {"ds"=>DSIG}) do |ref|
         uri                           = ref.attributes.get_attribute("URI").value
 
-        hashed_element                = document.at_xpath("//*[@ID='#{uri[1..-1]}']")
+        hashed_element                = document.at_xpath("//*[@ID=$uri]", nil, { 'uri' => uri[1..-1] })
         canon_algorithm               = canon_algorithm REXML::XPath.first(ref, '//ds:CanonicalizationMethod', 'ds' => DSIG)
         canon_hashed_element          = hashed_element.canonicalize(canon_algorithm, inclusive_namespaces)
 

diff --git a/test/response_test.rb b/test/response_test.rb
@@ -18,6 +18,26 @@ class RubySamlTest < Minitest::Test
       response.validate!
     end
 
+    describe "Prevent XEE attack" do
+      before do
+        @response = OneLogin::RubySaml::Response.new(fixture(:attackxee))        
+      end
+
+      it "false when evil attack vector is present, soft = true" do
+        @response.soft = true
+        assert !@response.send(:validate_structure)
+        assert_includes @response.errors, "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
+      end
+
+      it "raise when evil attack vector is present, soft = false " do
+        @response.soft = false
+
+        assert_raises(OneLogin::RubySaml::ValidationError) do
+          @response.send(:validate_structure)
+        end
+      end
+    end
+
     it "adapt namespace" do
       response = OneLogin::RubySaml::Response.new(response_document)
       refute_nil response.name_id

diff --git a/test/responses/attackxee.xml b/test/responses/attackxee.xml
@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+<!DOCTYPE lolz [
+ <!ENTITY lol "lol">
+ <!ENTITY lol2 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
+ <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
+ <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
+ <!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
+ <!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
+ <!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
+ <!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
+ <!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
+]>
+<lolz>&lol9;</lolz>