Refactor of safe_load_xml method. Improve how handle XML that contains syntax errors

Author: pitbulk

lib/ruby_saml/idp_metadata_parser.rb

@@ -161,7 +161,7 @@ def parse_to_array(idp_metadata, options = {})
     end
 
     def parse_to_idp_metadata_array(idp_metadata, options = {})
-      @document = Nokogiri::XML(idp_metadata) # TODO: RubySaml::XML.safe_load_nokogiri
+      @document = RubySaml::XML.safe_load_xml(idp_metadata, check_malformed_doc: true)
       @options = options
 
       idpsso_descriptors = self.class.get_idps(@document, options[:entity_id])

lib/ruby_saml/logoutresponse.rb

@@ -41,7 +41,14 @@ def initialize(response, settings = nil, options = {})
 
       @options = options
       @response = RubySaml::XML::Decoder.decode_message(response, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@response)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@response, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << e.message if e.message != "Empty document"
+        return false if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document"
+      end
+
       super()
     end
 
@@ -136,9 +143,13 @@ def validate_success_status
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_structure
+      structure_error_msg = "Invalid SAML Logout Response. Not match the saml-schema-protocol-2.0.xsd"
+
+      doc_to_analize = @document.nil? ? @response : @document
+
       check_malformed_doc = check_malformed_doc?(settings)
-      unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc)
-        return append_error("Invalid SAML Logout Response. Not match the saml-schema-protocol-2.0.xsd")
+      unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc)
+        return append_error(structure_error_msg)
       end
 
       true

lib/ruby_saml/response.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+
+require "ruby_saml/settings"
 require "ruby_saml/xml"
 require "ruby_saml/attributes"
 require "time"
@@ -52,18 +54,29 @@ def initialize(response, options = {})
 
       @options = options
       @soft = true
+      message_max_bytesize = nil
       unless options[:settings].nil?
         @settings = options[:settings]
-        unless @settings.soft.nil?
-          @soft = @settings.soft
-        end
+
+        raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings. class. name}") if !@settings.is_a?(Settings) && !@settings.nil?
+
+        @soft = @settings&.respond_to?(:soft) && !@settings.soft.nil?  ? @settings.soft : true
+        message_max_bytesize = @settings.message_max_bytesize if @settings.respond_to?(:message_max_bytesize)
       end
 
-      @response = RubySaml::XML::Decoder.decode_message(response, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@response)
+      @response = RubySaml::XML::Decoder.decode_message(response, message_max_bytesize)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@response, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << "XML load failed: #{e.message}" if e.message != "Empty document"
+        return false if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document"
+      end
 
-      if assertion_encrypted?
-        @decrypted_document = generate_decrypted_document
+      unless @document.nil?
+        if assertion_encrypted?
+          @decrypted_document = generate_decrypted_document
+        end
       end
 
       super()
@@ -131,6 +144,8 @@ def sessionindex
     # @raise [ValidationError] if there are 2+ Attribute with the same Name
     #
     def attributes
+      return nil if @document.nil?
+
       @attr_statements ||= begin
         attributes = Attributes.new
 
@@ -367,6 +382,9 @@ def assertion_id
     #
     def validate(collect_errors = false)
       reset_errors!
+
+      return append_error("Blank response") if @document.nil?
+
       return false unless validate_response_state
 
       validations = %i[
@@ -417,8 +435,10 @@ def validate_success_status
     def validate_structure
       structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
 
+      doc_to_analize = @document.nil? ? @response : @document
+
       check_malformed_doc = check_malformed_doc_enabled?
-      unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc)
+      unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc)
         return append_error(structure_error_msg)
       end
 
@@ -900,6 +920,8 @@ def validate_signature
     end
 
     def name_id_node
+      return nil if @document.nil?
+
       @name_id_node ||=
         begin
           encrypted_node = xpath_first_from_signed_assertion('/a:Subject/a:EncryptedID')

lib/ruby_saml/saml_message.rb

@@ -29,6 +29,8 @@ def id(document)
     end
 
     def root_attribute(document, attribute)
+      return nil if document.nil?
+
       document.at_xpath(
         "/p:AuthnRequest | /p:Response | /p:LogoutResponse | /p:LogoutRequest",
         { "p" => RubySaml::XML::NS_PROTOCOL }
@@ -43,10 +45,10 @@ def root_attribute(document, attribute)
     # @raise [ValidationError] if soft == false and validation fails
     def valid_saml?(document, soft = true, check_malformed_doc: true)
       begin
-        xml = RubySaml::XML.safe_load_nokogiri(document, check_malformed_doc: check_malformed_doc)
+        xml = RubySaml::XML.safe_load_xml(document, check_malformed_doc: check_malformed_doc)
       rescue StandardError => error
         return false if soft
-        raise ValidationError.new("XML load failed: #{error.message}")
+        raise ValidationError.new("XML load failed: #{error.message}") if error.message != "Empty document"
       end
 
       SamlMessage.schema.validate(xml).each do |schema_error|

lib/ruby_saml/slo_logoutrequest.rb

@@ -40,7 +40,14 @@ def initialize(request, options = {})
       end
 
       @request = RubySaml::XML::Decoder.decode_message(request, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@request)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@request, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << e.message if e.message != "Empty document"
+        return false if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document"
+      end
+
       super()
     end
 
@@ -157,6 +164,8 @@ def validate(collect_errors = false)
     # @return [Boolean] True if the Logout Request contains an ID, otherwise returns False
     #
     def validate_id
+      return append_error("Missing ID attribute on Logout Request") if document.nil?
+
       return true if id
       append_error("Missing ID attribute on Logout Request")
     end
@@ -166,6 +175,8 @@ def validate_id
     # @return [Boolean] True if the Logout Request is 2.0, otherwise returns False
     #
     def validate_version
+      return append_error("Unsupported SAML version") if document.nil?
+
       return true if version(document) == "2.0"
       append_error("Unsupported SAML version")
     end
@@ -191,8 +202,10 @@ def validate_not_on_or_after
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_structure
+      doc_to_analize = @document.nil? ? @request : @document
+
       check_malformed_doc = check_malformed_doc?(settings)
-      unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc)
+      unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc)
         return append_error("Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd")
       end
 

lib/ruby_saml/xml.rb

@@ -49,49 +49,34 @@ module XML
     NOKOGIRI_OPTIONS = Nokogiri::XML::ParseOptions::STRICT |
                        Nokogiri::XML::ParseOptions::NONET
 
-    # TODO: safe_load_message (rename safe_load_nokogiri --> safe_load_xml)
-    # def safe_load_message(message, check_malformed_doc: true)
-    #   message = Decoder.decode(message)
-    #   begin
-    #     safe_load_nokogiri(message, check_malformed_doc: check_malformed_doc)
-    #   rescue RubySaml::Errors::XMLLoadError
-    #     Nokogiri::XML::Document.new
-    #   end
-    # end
-
     # Safely load the SAML Message XML.
     # @param document [String | Nokogiri::XML::Document] The message to be loaded
     # @param check_malformed_doc [Boolean] check_malformed_doc Enable or Disable the check for malformed XML
     # @return [Nokogiri::XML::Document] The nokogiri document
-    # @raise [ValidationError] If there was a problem loading the SAML Message XML
-    def safe_load_nokogiri(document, check_malformed_doc: true)
+    # @raise [StandardError] If there was a problem loading the SAML Message XML
+    def safe_load_xml(document, check_malformed_doc: true)
       doc_str = document.to_s
-      error = nil
-      error = StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc_str.include?('<!DOCTYPE')
-
-      xml = nil
-      unless error
-        begin
-          xml = Nokogiri::XML(doc_str) do |config|
-            config.options = NOKOGIRI_OPTIONS
-          end
-        rescue StandardError => e
-          error ||= e
-          # raise StandardError.new(e.message)
+      raise StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc_str.include?('<!DOCTYPE')
+
+      begin
+        xml = Nokogiri::XML(doc_str) do |config|
+          config.options = NOKOGIRI_OPTIONS
         end
+      rescue StandardError => e
+        raise StandardError.new(e.message)
+      rescue SyntaxError => e
+        raise StandardError.new(e.message) if check_malformed_doc && e.message != "Empty document"
       end
 
-      # TODO: This is messy, its shims how the old REXML parser works
-      if xml
-        error ||= StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if xml.internal_subset
-        error ||= StandardError.new("There were XML errors when parsing: #{xml.errors}") if check_malformed_doc && !xml.errors.empty?
+      if xml && xml.is_a?(Nokogiri::XML::Document)
+        StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if xml.internal_subset
+        StandardError.new("There were XML errors when parsing: #{xml.errors}") if check_malformed_doc && !xml.errors.empty?
       end
-      return Nokogiri::XML::Document.new if error || !xml
 
       xml
     end
 
-    def copy_nokogiri(noko)
+    def copy_xml(noko)
       Nokogiri::XML(noko.to_xml(save_with: Nokogiri::XML::Node::SaveOptions::AS_XML)) do |config|
         config.options = NOKOGIRI_OPTIONS
       end

lib/ruby_saml/xml/decoder.rb

@@ -23,7 +23,7 @@ def decode_message(message, max_bytesize = nil)
 
         return message unless base64_encoded?(message)
 
-        message = try_inflate(base64_decode(message))
+        message = try_inflate(base64_decode(message), max_bytesize)
 
         if message.bytesize > max_bytesize # rubocop:disable Style/IfUnlessModifier
           raise ValidationError.new("SAML Message exceeds #{max_bytesize} bytes, so was rejected")
@@ -61,23 +61,52 @@ def base64_encode(string)
       # @param string [String] string to check the encoding of
       # @return [true, false] whether or not the string is base64 encoded
       def base64_encoded?(string)
-        string.gsub(/[\s\r\n]|\\r|\\n/, '').match?(BASE64_FORMAT)
+        string.gsub(/\s|\\r|\\n/, '').match?(BASE64_FORMAT)
       end
 
       # Attempt inflating a string, if it fails, return the original string.
       # @param data [String] The string
+      # @param max_bytesize [Integer] The maximum allowed size of the SAML Message,
+      #   to prevent a possible DoS attack.
       # @return [String] The inflated or original string
-      def try_inflate(data)
-        inflate(data)
+      def try_inflate(data, max_bytesize = nil)
+        inflate(data, max_bytesize)
       rescue Zlib::Error
         data
       end
 
       # Inflate method.
       # @param deflated [String] The string
+      # @param max_bytesize [Integer] The maximum allowed size of the SAML Message,
+      #   to prevent a possible DoS attack.
       # @return [String] The inflated string
-      def inflate(deflated)
-        Zlib::Inflate.new(-Zlib::MAX_WBITS).inflate(deflated)
+      def inflate(deflated, max_bytesize = nil)
+        if max_bytesize.nil?
+          Zlib::Inflate.new(-Zlib::MAX_WBITS).inflate(deflated)
+        else
+          inflater = Zlib::Inflate.new(-Zlib::MAX_WBITS)
+
+          # Use a StringIO buffer to build the inflated message incrementally.
+          buffer = StringIO.new
+
+          inflater.inflate(deflated) do |chunk|
+            if buffer.length + chunk.bytesize > max_bytesize
+              inflater.close
+              raise ValidationError.new("SAML Message exceeds #{max_bytesize} bytes during decompression, so was rejected")
+            end
+            buffer << chunk
+          end
+
+          final_chunk = inflater.finish
+          unless final_chunk.empty?
+            raise ValidationError.new("SAML Message exceeds #{max_bytesize} bytes during decompression, so was rejected") if buffer.length + final_chunk.bytesize > max_bytesize
+
+            buffer << final_chunk
+          end
+
+          inflater.close
+          buffer.string
+        end
       end
 
       # Deflate method.

lib/ruby_saml/xml/decryptor.rb

@@ -12,7 +12,7 @@ module Decryptor
       # @return [Nokogiri::XML::Document] The SAML document with assertions decrypted
       def decrypt_document(document, decryption_keys)
         # Copy the document
-        document = RubySaml::XML.safe_load_nokogiri(document.to_s)
+        document = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true)
         validate_decryption_keys!(decryption_keys)
 
         response_node = document.at_xpath(

lib/ruby_saml/xml/document_signer.rb

@@ -27,7 +27,7 @@ module DocumentSigner
       #   <Object />
       # </Signature>
       def sign_document(document, private_key, certificate, signature_method = RubySaml::XML::RSA_SHA256, digest_method = RubySaml::XML::SHA256)
-        noko = RubySaml::XML.safe_load_nokogiri(document.to_s)
+        noko = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true)
 
         sign_document!(noko, private_key, certificate, signature_method, digest_method)
       end

lib/ruby_saml/xml/signed_document_info.rb

@@ -14,9 +14,13 @@ class SignedDocumentInfo
       # @param check_malformed_doc [Boolean] Whether to check for malformed documents
       def initialize(noko, check_malformed_doc: true)
         noko = if noko.is_a?(Nokogiri::XML::Document)
-                 RubySaml::XML.copy_nokogiri(noko)
+                 RubySaml::XML.copy_xml(noko)
                else
-                 RubySaml::XML.safe_load_nokogiri(noko, check_malformed_doc: check_malformed_doc)
+                  begin
+                    @document = RubySaml::XML.safe_load_xml(noko, check_malformed_doc: check_malformed_doc)
+                  rescue StandardError => e
+                    raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document"
+                  end
                end
         @noko = noko
         @check_malformed_doc = check_malformed_doc