Merge pull request #247 from onelogin/prevent_entity_expansion

Security improvement: Avoid entity expansion (XEE attacks)

Author: pitbulk

lib/onelogin/ruby-saml/saml_message.rb

@@ -63,10 +63,17 @@ def id(document)
       # @raise [ValidationError] if soft == false and validation fails 
       #
       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

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#"
+    NOKOGIRI_OPTIONS = Nokogiri::XML::ParseOptions::STRICT |
+                       Nokogiri::XML::ParseOptions::NONET
 
     def canon_algorithm(element)
       algorithm = element
@@ -106,7 +109,9 @@ def uuid
       #<Object />
     #</Signature>
     def sign_document(private_key, certificate, signature_method = RSA_SHA1, digest_method = SHA1)
-      noko = Nokogiri.parse(self.to_s)
+      noko = Nokogiri.parse(self.to_s) do |options|
+        options = XMLSecurity::BaseDocument::NOKOGIRI_OPTIONS
+      end
 
       signature_element = REXML::Element.new("ds:Signature").add_namespace('ds', DSIG)
       signed_info_element = signature_element.add_element("ds:SignedInfo")
@@ -128,7 +133,10 @@ def sign_document(private_key, certificate, signature_method = RSA_SHA1, digest_
       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))
 
@@ -178,7 +186,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, options = {})
@@ -221,7 +232,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

test/response_test.rb

@@ -53,6 +53,26 @@ class RubySamlTest < Minitest::Test
       assert_includes ampersands_response.errors, "SAML Response must contain 1 assertion"
     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
       refute_nil response.nameid
       refute_nil response_without_attributes.nameid

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>