diff --git a/lib/ruby_saml/idp_metadata_parser.rb b/lib/ruby_saml/idp_metadata_parser.rb index 7e1551d5..5a354642 100644 --- a/lib/ruby_saml/idp_metadata_parser.rb +++ b/lib/ruby_saml/idp_metadata_parser.rb @@ -161,7 +161,12 @@ 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 + begin + @document = RubySaml::XML.safe_load_xml(idp_metadata, check_malformed_doc: true) + rescue StandardError => e + raise ArgumentError.new("XML load failed: #{e.message}") if e.message != 'Empty document' + end + @options = options idpsso_descriptors = self.class.get_idps(@document, options[:entity_id]) diff --git a/lib/ruby_saml/logoutresponse.rb b/lib/ruby_saml/logoutresponse.rb index 3368675e..0a78fd95 100644 --- a/lib/ruby_saml/logoutresponse.rb +++ b/lib/ruby_saml/logoutresponse.rb @@ -33,6 +33,8 @@ def initialize(response, settings = nil, options = {}) raise ArgumentError.new("Logoutresponse cannot be nil") if response.nil? @settings = settings + raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings.class.name}") if !@settings.is_a?(Settings) && !@settings.nil? + if settings.nil? || settings.soft.nil? @soft = true else @@ -41,7 +43,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 << "XML load failed: #{e.message}" if e.message != "Empty document" + return if @soft + raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document" + end + super() end @@ -136,9 +145,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 diff --git a/lib/ruby_saml/response.rb b/lib/ruby_saml/response.rb index 127c14c7..736dbb4e 100644 --- a/lib/ruby_saml/response.rb +++ b/lib/ruby_saml/response.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "ruby_saml/settings" require "ruby_saml/xml" require "ruby_saml/attributes" require "time" @@ -52,18 +53,27 @@ 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 if @soft + raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' + end - if assertion_encrypted? - @decrypted_document = generate_decrypted_document + if !@document.nil? && assertion_encrypted? + @decrypted_document = generate_decrypted_document end super() @@ -131,6 +141,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 +379,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 +432,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 +917,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') diff --git a/lib/ruby_saml/saml_message.rb b/lib/ruby_saml/saml_message.rb index c9ead9e4..edea5006 100644 --- a/lib/ruby_saml/saml_message.rb +++ b/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| diff --git a/lib/ruby_saml/slo_logoutrequest.rb b/lib/ruby_saml/slo_logoutrequest.rb index 854ab306..9abdf513 100644 --- a/lib/ruby_saml/slo_logoutrequest.rb +++ b/lib/ruby_saml/slo_logoutrequest.rb @@ -36,11 +36,22 @@ def initialize(request, options = {}) @soft = true unless options[:settings].nil? @settings = options[:settings] - @soft = @settings.soft unless @settings.soft.nil? + + 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 + + @request = RubySaml::XML::Decoder.decode_message(request, message_max_bytesize) + begin + @document = RubySaml::XML.safe_load_xml(@request, check_malformed_doc: @soft) + rescue StandardError => e + @errors << "XML load failed: #{e.message}" if e.message != 'Empty document' + return if @soft + raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' end - @request = RubySaml::XML::Decoder.decode_message(request, @settings&.message_max_bytesize) - @document = RubySaml::XML.safe_load_nokogiri(@request) super() end @@ -157,6 +168,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 +179,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 +206,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 diff --git a/lib/ruby_saml/xml.rb b/lib/ruby_saml/xml.rb index 5fd47bf3..929528fd 100644 --- a/lib/ruby_saml/xml.rb +++ b/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?(' e - error ||= e - # raise StandardError.new(e.message) + raise StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc_str.include?(' 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 doc.is_a?(Nokogiri::XML::Document) + StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc.internal_subset + StandardError.new("There were XML errors when parsing: #{doc.errors}") if check_malformed_doc && !doc.errors.empty? end - return Nokogiri::XML::Document.new if error || !xml - xml + doc 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 diff --git a/lib/ruby_saml/xml/decryptor.rb b/lib/ruby_saml/xml/decryptor.rb index 3ec54e6e..74faa704 100644 --- a/lib/ruby_saml/xml/decryptor.rb +++ b/lib/ruby_saml/xml/decryptor.rb @@ -11,8 +11,13 @@ module Decryptor # @param decryption_keys [Array] Array of private keys for decryption # @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) + # Copy the document to avoid modifying the original one + begin + document = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true) + rescue StandardError => e + raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' + end + validate_decryption_keys!(decryption_keys) response_node = document.at_xpath( diff --git a/lib/ruby_saml/xml/document_signer.rb b/lib/ruby_saml/xml/document_signer.rb index 823b4d61..3f73a27f 100644 --- a/lib/ruby_saml/xml/document_signer.rb +++ b/lib/ruby_saml/xml/document_signer.rb @@ -27,7 +27,11 @@ module DocumentSigner # # 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) + begin + noko = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true) + rescue StandardError => e + raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' + end sign_document!(noko, private_key, certificate, signature_method, digest_method) end diff --git a/lib/ruby_saml/xml/signed_document_info.rb b/lib/ruby_saml/xml/signed_document_info.rb index 081c9a6b..45d49d19 100644 --- a/lib/ruby_saml/xml/signed_document_info.rb +++ b/lib/ruby_saml/xml/signed_document_info.rb @@ -13,12 +13,17 @@ class SignedDocumentInfo # @param noko [Nokogiri::XML] The XML document to validate # @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) - else - RubySaml::XML.safe_load_nokogiri(noko, check_malformed_doc: check_malformed_doc) - end - @noko = noko + @noko = if noko.is_a?(Nokogiri::XML::Document) + RubySaml::XML.copy_xml(noko) + else + begin + 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' + + nil + end + end @check_malformed_doc = check_malformed_doc end diff --git a/test/response_test.rb b/test/response_test.rb index 239e3d65..aa3363f4 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -65,6 +65,15 @@ def generate_audience_error(expected, actual) assert_raises(ArgumentError) { RubySaml::Response.new(nil) } end + it 'raise an exception when the settings provided are not a RubySaml::Settings object' do + settings = "invalid settings" + error_msg = "Invalid settings type: expected RubySaml::Settings, got String" + options = { :settings => settings } + assert_raises(RubySaml::ValidationError, error_msg) do + RubySaml::Response.new(response_document_valid_signed, options) + end + end + it 'not filter available options only' do options = { skip_destination: true, foo: :bar } response = RubySaml::Response.new(response_document_valid_signed, options) @@ -85,6 +94,36 @@ def generate_audience_error(expected, actual) assert_includes ampersands_response.errors, 'SAML Response must contain 1 assertion' end + it 'Raise ValidationError if XML contains SyntaxError trying to initialize and soft = false' do + settings.soft = false + error_msg = if jruby? + 'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag "".' + else + 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + end + assert_raises(RubySaml::ValidationError, error_msg) do + OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings) + end + end + + it "Do not raise validation error when XML contains SyntaxError and soft = true, but validation fails" do + settings.soft = true + settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint + response = OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings) + error_msg = if jruby? + 'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag "".' + else + 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + end + assert_includes response.errors, error_msg + + refute response.is_valid? + + assert_includes response.errors, 'Blank response' + assert_nil response.nameid + assert_nil response.attributes + end + describe 'Prevent node text with comment attack (VU#475445)' do before do @response = RubySaml::Response.new(read_response('response_node_text_attack.xml.base64')) @@ -140,6 +179,7 @@ def generate_audience_error(expected, actual) it 'raise when evil attack vector is present, soft = false ' do @response.soft = false error_msg = 'XML load failed: Dangerous XML detected. No Doctype nodes allowed' + assert_raises(RubySaml::ValidationError, error_msg) do @response.send(:validate_structure) end diff --git a/test/responses/response_wrong_syntax.xml b/test/responses/response_wrong_syntax.xml new file mode 100644 index 00000000..11025a43 --- /dev/null +++ b/test/responses/response_wrong_syntax.xml @@ -0,0 +1,74 @@ + + https://idp.example.com/saml + + + + + + https://idp.example.com/saml + + admin@example.com + + + + + + + http://example.com/saml/metadata + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport + + + + + + + + + https://idp.example.com/saml + + + + + + + + + + + S7a2iDRyD6BNdJ4F3h2aTqkD35U= + + + iAvQPKdMGBs2BT0vLhF/BpbQ55w5N1M2P5zT5Xqj+g8E4Z6k4Z7l8Y8l9L0n9o1p3q5r7s9t/v0x/z+B2D4F6G8I+K+M+N+P+R+T+V+X+Z+b+d+f+h+j+l+n+p+r+t+v+x/z= + + + MIICZDCCAc2gAwIBAgIBADANBgkqhkiG9w0BAQ0FADBPMQswCQYDVQQGEwJ1czEUMBIGA1UECAwLZXhhbXBsZS5jb20xFDASBgNVBAoMC2V4YW1wbGUuY29tMRQwEgYDVQQDDAtleGFtcGxlLmNvbTAeFw0yNTExMjEyMzM4MThaFw0zNTExMTkyMzM4MThaME8xCzAJBgNVBAYTAnVzMRQwEgYDVQQIDAtleGFtcGxlLmNvbTEUMBIGA1UECgwLZXhhbXBsZS5jb20xFDASBgNVBAMMC2V4YW1wbGUuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCrujq+KXddSKM7KZHeERWPbGMDBRCPu2G0u1IcQaGsNLLlotdT7LG98f8V0aHJlQculgE/jjNzmDeOYlu6MVOqz1LuYTupToIwqQF7vJIXSBVRL8oS7oysYFi+BiA59D2vJy6TpENo7T/evWq7TOxFkxUvhu/R0v26vbRyJ3sjJwIDAQABo1AwTjAdBgNVHQ4EFgQUq1UNohqTxoPkcrhAlKN+tZqyDgIwHwYDVR0jBBgwFoAUq1UNohqTxoPkcrhAlKN+tZqyDgIwDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQ0FAAOBgQCJAv0p2W147R0mwKgse3OI0P/YN7kh9pZAXEq9TS46cjEsvlPL7PmYxD8tSDFwTsuOd1e4vjY6sMRm3zbxLNkzv6fehp46o7CJLu1rhHxVgY4CDxkNfptJaSOlueuiFnNyMK3Ux/Ks5k+dvpAXrc3gztwmlVYPYpKdhuwrTqxzHA== + + + + + victim@example.com + + + + + + + http://example.com/saml/metadata + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport + + + + \ No newline at end of file