From e3b1542715a369f5e6d8e82a2dbb63af2a380a12 Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 03:00:33 +0100 Subject: [PATCH 1/6] Refactor of safe_load_xml method. Improve how handle XML that contains syntax errors --- lib/ruby_saml/idp_metadata_parser.rb | 2 +- lib/ruby_saml/logoutresponse.rb | 17 +++++- lib/ruby_saml/response.rb | 38 +++++++++--- lib/ruby_saml/saml_message.rb | 6 +- lib/ruby_saml/slo_logoutrequest.rb | 17 +++++- lib/ruby_saml/xml.rb | 45 +++++--------- lib/ruby_saml/xml/decoder.rb | 41 +++++++++++-- lib/ruby_saml/xml/decryptor.rb | 2 +- lib/ruby_saml/xml/document_signer.rb | 2 +- lib/ruby_saml/xml/signed_document_info.rb | 8 ++- test/response_test.rb | 43 +++++++++++-- test/responses/response_wrong_syntax.xml | 74 +++++++++++++++++++++++ 12 files changed, 234 insertions(+), 61 deletions(-) create mode 100644 test/responses/response_wrong_syntax.xml diff --git a/lib/ruby_saml/idp_metadata_parser.rb b/lib/ruby_saml/idp_metadata_parser.rb index 666155c3..06efdc31 100644 --- a/lib/ruby_saml/idp_metadata_parser.rb +++ b/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]) diff --git a/lib/ruby_saml/logoutresponse.rb b/lib/ruby_saml/logoutresponse.rb index 86537974..a5595acf 100644 --- a/lib/ruby_saml/logoutresponse.rb +++ b/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 diff --git a/lib/ruby_saml/response.rb b/lib/ruby_saml/response.rb index 10bd6931..1f9da6de 100644 --- a/lib/ruby_saml/response.rb +++ b/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') 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 bcfac86d..c241a973 100644 --- a/lib/ruby_saml/slo_logoutrequest.rb +++ b/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 diff --git a/lib/ruby_saml/xml.rb b/lib/ruby_saml/xml.rb index 5fd47bf3..ead490b0 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 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 diff --git a/lib/ruby_saml/xml/decoder.rb b/lib/ruby_saml/xml/decoder.rb index 6d8e58d0..917ead3c 100644 --- a/lib/ruby_saml/xml/decoder.rb +++ b/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. diff --git a/lib/ruby_saml/xml/decryptor.rb b/lib/ruby_saml/xml/decryptor.rb index e481c8af..6b839bf5 100644 --- a/lib/ruby_saml/xml/decryptor.rb +++ b/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( diff --git a/lib/ruby_saml/xml/document_signer.rb b/lib/ruby_saml/xml/document_signer.rb index 823b4d61..a17b8dbe 100644 --- a/lib/ruby_saml/xml/document_signer.rb +++ b/lib/ruby_saml/xml/document_signer.rb @@ -27,7 +27,7 @@ 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) + noko = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true) 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..d6ef92c4 100644 --- a/lib/ruby_saml/xml/signed_document_info.rb +++ b/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 diff --git a/test/response_test.rb b/test/response_test.rb index a0050692..90884b31 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -66,6 +66,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,27 @@ 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 = 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + 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) + assert_includes response.errors, 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + + 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')) @@ -139,6 +169,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 @@ -252,7 +283,9 @@ def generate_audience_error(expected, actual) assert_raises(RubySaml::ValidationError, error_msg) do response_without_recipient.is_valid? end - assert !response_without_recipient.errors.empty? + + refute response_without_recipient.errors.empty? + assert_includes response_without_recipient.errors[0], error_msg end @@ -410,8 +443,8 @@ def generate_audience_error(expected, actual) settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint response_without_recipient.settings = settings error_msg = "Current time is on or after NotOnOrAfter condition" - assert !response_without_recipient.is_valid? - assert !response_without_recipient.errors.empty? + refute response_without_recipient.is_valid? + refute response_without_recipient.errors.empty? assert_includes response_without_recipient.errors[0], error_msg end @@ -557,14 +590,14 @@ def generate_audience_error(expected, actual) it "returns false on a case insenstive match on the path" do response_valid_signed_without_x509certificate.settings = settings response_valid_signed_without_x509certificate.settings.assertion_consumer_service_url = 'http://app.muda.no/SSO/consume' - assert !response_valid_signed_without_x509certificate.send(:validate_destination) + refute response_valid_signed_without_x509certificate.send(:validate_destination) assert_includes response_valid_signed_without_x509certificate.errors, "The response was received at #{response_valid_signed_without_x509certificate.destination} instead of #{response_valid_signed_without_x509certificate.settings.assertion_consumer_service_url}" end it "returns true if it can't parse out a full URI." do response_valid_signed_without_x509certificate.settings = settings response_valid_signed_without_x509certificate.settings.assertion_consumer_service_url = 'presenter' - assert !response_valid_signed_without_x509certificate.send(:validate_destination) + refute response_valid_signed_without_x509certificate.send(:validate_destination) assert_includes response_valid_signed_without_x509certificate.errors, "The response was received at #{response_valid_signed_without_x509certificate.destination} instead of #{response_valid_signed_without_x509certificate.settings.assertion_consumer_service_url}" end 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 From a7c847fedbeda537978078a25a90d238d8f1c780 Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 11:58:39 +0100 Subject: [PATCH 2/6] Fix Rubocop --- lib/ruby_saml/idp_metadata_parser.rb | 7 ++++++- lib/ruby_saml/logoutresponse.rb | 6 ++++-- lib/ruby_saml/response.rb | 15 ++++++--------- lib/ruby_saml/slo_logoutrequest.rb | 14 +++++++++----- lib/ruby_saml/xml.rb | 12 ++++++------ lib/ruby_saml/xml/decryptor.rb | 9 +++++++-- lib/ruby_saml/xml/document_signer.rb | 6 +++++- lib/ruby_saml/xml/signed_document_info.rb | 15 ++++++++------- 8 files changed, 51 insertions(+), 33 deletions(-) diff --git a/lib/ruby_saml/idp_metadata_parser.rb b/lib/ruby_saml/idp_metadata_parser.rb index 06efdc31..6ab1c6ed 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 = RubySaml::XML.safe_load_xml(idp_metadata, check_malformed_doc: true) + 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 a5595acf..cad757b7 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 @@ -44,8 +46,8 @@ def initialize(response, settings = nil, options = {}) 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 + @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 diff --git a/lib/ruby_saml/response.rb b/lib/ruby_saml/response.rb index 1f9da6de..84549b64 100644 --- a/lib/ruby_saml/response.rb +++ b/lib/ruby_saml/response.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true - require "ruby_saml/settings" require "ruby_saml/xml" require "ruby_saml/attributes" @@ -58,9 +57,9 @@ def initialize(response, options = {}) unless options[:settings].nil? @settings = options[:settings] - raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings. class. name}") if !@settings.is_a?(Settings) && !@settings.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 + @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 @@ -68,15 +67,13 @@ def initialize(response, options = {}) 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" + @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 - unless @document.nil? - if assertion_encrypted? + if !@document.nil? && assertion_encrypted? @decrypted_document = generate_decrypted_document - end end super() diff --git a/lib/ruby_saml/slo_logoutrequest.rb b/lib/ruby_saml/slo_logoutrequest.rb index c241a973..05c1dcf8 100644 --- a/lib/ruby_saml/slo_logoutrequest.rb +++ b/lib/ruby_saml/slo_logoutrequest.rb @@ -36,16 +36,20 @@ 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, @settings&.message_max_bytesize) + @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 << e.message if e.message != "Empty document" - return false if @soft - raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document" + @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() diff --git a/lib/ruby_saml/xml.rb b/lib/ruby_saml/xml.rb index ead490b0..929528fd 100644 --- a/lib/ruby_saml/xml.rb +++ b/lib/ruby_saml/xml.rb @@ -59,21 +59,21 @@ def safe_load_xml(document, check_malformed_doc: true) 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" + raise StandardError.new(e.message) if check_malformed_doc && e.message != 'Empty document' end - 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? + 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 - xml + doc end def copy_xml(noko) diff --git a/lib/ruby_saml/xml/decryptor.rb b/lib/ruby_saml/xml/decryptor.rb index 6b839bf5..50f705d3 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_xml(document.to_s, check_malformed_doc: true) + # 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 a17b8dbe..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_xml(document.to_s, check_malformed_doc: true) + 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 d6ef92c4..45d49d19 100644 --- a/lib/ruby_saml/xml/signed_document_info.rb +++ b/lib/ruby_saml/xml/signed_document_info.rb @@ -13,16 +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_xml(noko) - else + @noko = if noko.is_a?(Nokogiri::XML::Document) + RubySaml::XML.copy_xml(noko) + else begin - @document = RubySaml::XML.safe_load_xml(noko, check_malformed_doc: check_malformed_doc) + 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" + raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' + + nil end - end - @noko = noko + end @check_malformed_doc = check_malformed_doc end From 600e62f351b464c5c0e56141d95acfa21cc346b7 Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 12:08:58 +0100 Subject: [PATCH 3/6] Adapt the syntax error to JRuby --- test/response_test.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/response_test.rb b/test/response_test.rb index 90884b31..7ca8aa86 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -96,7 +96,11 @@ def generate_audience_error(expected, actual) it "Raise ValidationError if XML contains SyntaxError trying to initialize and soft = false" do settings.soft = false - error_msg = 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + error_msg = if jruby? + 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + else + 'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag ""' + end assert_raises(RubySaml::ValidationError, error_msg) do OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings) end From 528627478f65c1e13c66fb8b02bf7e36db9be7a2 Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 12:14:15 +0100 Subject: [PATCH 4/6] Fix order typo --- test/response_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/response_test.rb b/test/response_test.rb index 7ca8aa86..b41fdc0c 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -97,9 +97,9 @@ def generate_audience_error(expected, actual) it "Raise ValidationError if XML contains SyntaxError trying to initialize and soft = false" do settings.soft = false error_msg = if jruby? - 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + 'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag ""' else - 'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag ""' + '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) From c8b7adaeb79eba5abc9e95d944d96105c9fac7b3 Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 12:23:54 +0100 Subject: [PATCH 5/6] Adapt error_msg on forgotten test --- test/response_test.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/response_test.rb b/test/response_test.rb index da337370..8c215793 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -110,7 +110,12 @@ def generate_audience_error(expected, actual) settings.soft = true settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint response = OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings) - assert_includes response.errors, 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' + 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? From 67984426170d4ce807fa22169e2c0465ebb8622e Mon Sep 17 00:00:00 2001 From: Sixto Martin Date: Sun, 30 Nov 2025 12:54:56 +0100 Subject: [PATCH 6/6] Minor change on test --- test/response_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/response_test.rb b/test/response_test.rb index 8c215793..aa3363f4 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -97,9 +97,9 @@ def generate_audience_error(expected, actual) 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 ""' + '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' + '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) @@ -111,9 +111,9 @@ def generate_audience_error(expected, actual) 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 ""' + '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' + 'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue' end assert_includes response.errors, error_msg