diff --git a/lib/samlr/signature.rb b/lib/samlr/signature.rb index 0ec9e91..ac62a2a 100644 --- a/lib/samlr/signature.rb +++ b/lib/samlr/signature.rb @@ -15,10 +15,11 @@ def initialize(original, prefix, options) @document = original.dup @prefix = prefix @options = options + @signature = nil - if @signature = document.at("#{prefix}/ds:Signature", NS_MAP) - @signature.remove # enveloped signatures only - end + id = document.at("#{prefix}")&.attribute('ID') + @signature = document.at("#{prefix}/ds:Signature/ds:SignedInfo/ds:Reference[@URI='##{id}']", NS_MAP)&.parent&.parent if id + @signature.remove if @signature # enveloped signatures only @fingerprint = if options[:fingerprint] Fingerprint.from_string(options[:fingerprint]) diff --git a/lib/samlr/tools/metadata_builder.rb b/lib/samlr/tools/metadata_builder.rb index 5ae5964..52eec27 100644 --- a/lib/samlr/tools/metadata_builder.rb +++ b/lib/samlr/tools/metadata_builder.rb @@ -13,12 +13,14 @@ def self.build(options = {}) name_identity_format = options[:name_identity_format] consumer_service_url = options[:consumer_service_url] consumer_service_binding = options[:consumer_service_binding] || "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" + metadata_id = options[:metadata_id] || Samlr::Tools.uuid + sign_metadata = options[:sign_metadata] || false # Mandatory - entity_id = options.fetch(:entity_id) + entity_id = options.fetch(:entity_id) builder = Nokogiri::XML::Builder.new do |xml| - xml.EntityDescriptor("xmlns:md" => NS_MAP["md"], "entityID" => entity_id) do + xml.EntityDescriptor("xmlns:md" => NS_MAP["md"], "ID" => metadata_id, "entityID" => entity_id) do xml.doc.root.namespace = xml.doc.root.namespace_definitions.find { |ns| ns.prefix == "md" } xml["md"].SPSSODescriptor("protocolSupportEnumeration" => NS_MAP["samlp"]) do @@ -33,7 +35,14 @@ def self.build(options = {}) end end - builder.to_xml(COMPACT) + metadata = builder.doc + + if sign_metadata + metadata_options = options.merge(namespaces: []) + metadata = ResponseBuilder.sign(metadata, metadata_id, metadata_options) + end + + metadata.to_xml(COMPACT) end end diff --git a/lib/samlr/tools/response_builder.rb b/lib/samlr/tools/response_builder.rb index 7fa82f2..79f2f90 100644 --- a/lib/samlr/tools/response_builder.rb +++ b/lib/samlr/tools/response_builder.rb @@ -125,7 +125,11 @@ def self.sign(document, element_id, options) end unless skip_keyinfo end # digest.root.last_element_child.after "#{signature}" - element.at("./saml:Issuer", NS_MAP).add_next_sibling(digest) + if element.at("./saml:Issuer", NS_MAP) + element.at("./saml:Issuer", NS_MAP).add_next_sibling(digest) + else + element.children.first.add_previous_sibling(digest) + end document end diff --git a/test/fixtures/assertion_signature_wrapping.xml b/test/fixtures/assertion_signature_wrapping.xml new file mode 100644 index 0000000..7225fcf --- /dev/null +++ b/test/fixtures/assertion_signature_wrapping.xml @@ -0,0 +1,50 @@ + + + ResponseBuilder IdP + + + + + ResponseBuilder IdP + + + + + + + + + + + + + RIrP+dFpc1LqZr0nUVvVNNrqBj0= + + + NUShdJOltVGPmJknieVEqwKV13nRRZNrNaOE61YmAU3VvRxWBQiJClEV7VJ2jQlUxj36+5p8CSuhwwghfm3rnQ== + + + MIIBjTCCATegAwIBAgIBATANBgkqhkiG9w0BAQUFADBPMQswCQYDVQQGEwJVUzEUMBIGA1UECgwLZXhhbXBsZS5vcmcxHTAbBgNVBAsMFFphbWwgUmVzcG9uc2VCdWlsZGVyMQswCQYDVQQDDAJDQTAeFw0xMjA4MDgwMjAxMDlaFw0zMjA4MDMwMjAxMTRaME8xCzAJBgNVBAYTAlVTMRQwEgYDVQQKDAtleGFtcGxlLm9yZzEdMBsGA1UECwwUWmFtbCBSZXNwb25zZUJ1aWxkZXIxCzAJBgNVBAMMAkNBMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALb9pPmyHrbZJMDLLkVsHzzXvP7DFcPiYdaNU50l5znRr8ZGhwRZFAwKroOxXwhK5e9lz06C+kGqnL1v10h1BEUCAwEAATANBgkqhkiG9w0BAQUFAANBAKU10RznL2p7xRhO9vOh0CY+gWYmT2kbkLTVRYLApghQFAW8EzIHC/NggfEHM554ykzbbPwjSvM7cRBBDHYuWoY= + + + + + evil@crack.it + + + + + identity_format + + + + + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:Password + + + + diff --git a/test/fixtures/response_signature_wrapping.xml b/test/fixtures/response_signature_wrapping.xml new file mode 100644 index 0000000..923c25a --- /dev/null +++ b/test/fixtures/response_signature_wrapping.xml @@ -0,0 +1,51 @@ + + + ResponseBuilder IdP + + + + + + + + + + + + + qdW6Sx1sUmpN/joEMOUFPO1HOAc= + + + i+RCXmzCX5FMlh+0xgxkN6QjTJ5wzku36gv8hmUPB0GAN2AXHaSdYDtFLKCTa4F3MoFZq6oJeqSBfhkg/h1IKw== + + + MIIBjTCCATegAwIBAgIBATANBgkqhkiG9w0BAQUFADBPMQswCQYDVQQGEwJVUzEUMBIGA1UECgwLZXhhbXBsZS5vcmcxHTAbBgNVBAsMFFphbWwgUmVzcG9uc2VCdWlsZGVyMQswCQYDVQQDDAJDQTAeFw0xMjA4MDgwMjAxMDlaFw0zMjA4MDMwMjAxMTRaME8xCzAJBgNVBAYTAlVTMRQwEgYDVQQKDAtleGFtcGxlLm9yZzEdMBsGA1UECwwUWmFtbCBSZXNwb25zZUJ1aWxkZXIxCzAJBgNVBAMMAkNBMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALb9pPmyHrbZJMDLLkVsHzzXvP7DFcPiYdaNU50l5znRr8ZGhwRZFAwKroOxXwhK5e9lz06C+kGqnL1v10h1BEUCAwEAATANBgkqhkiG9w0BAQUFAANBAKU10RznL2p7xRhO9vOh0CY+gWYmT2kbkLTVRYLApghQFAW8EzIHC/NggfEHM554ykzbbPwjSvM7cRBBDHYuWoY= + + + + + + + + + identity_format + + + + + + + ResponseBuilder IdP + + evil@crack.it + + + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:Password + + + + diff --git a/test/unit/test_assertion.rb b/test/unit/test_assertion.rb index aaa25cb..f640ed8 100644 --- a/test/unit/test_assertion.rb +++ b/test/unit/test_assertion.rb @@ -52,6 +52,22 @@ end end + describe "#signature" do + it "is associated to the assertion" do + assert subject.signature.present? + end + + describe "when assertion envelops a signature referencing other element" do + let(:fingerprint) { Samlr::Certificate.new(TEST_CERTIFICATE.x509).fingerprint.value } + let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'assertion_signature_wrapping.xml'))) } + subject { Samlr::Response.new(xml_response_doc, fingerprint: fingerprint).assertion } + + it "does not associate it with the assertion" do + assert subject.signature.missing? + end + end + end + describe "#verify!" do let(:condition) do Class.new do diff --git a/test/unit/test_condition.rb b/test/unit/test_condition.rb index 38f8a4d..69b4cb0 100644 --- a/test/unit/test_condition.rb +++ b/test/unit/test_condition.rb @@ -37,7 +37,7 @@ def verify! verify! flunk "Expected exception" rescue Samlr::ConditionsError => e - assert_match /Audience/, e.message + assert_match %r{Audience}, e.message end end end @@ -132,7 +132,7 @@ def verify! verify! flunk "Expected exception" rescue Samlr::ConditionsError => e - assert_match /Audience/, e.message + assert_match %r{Audience}, e.message end end end @@ -151,7 +151,7 @@ def verify! subject.verify! flunk "Expected exception" rescue Samlr::ConditionsError => e - assert_match /Not before/, e.message + assert_match %r{Not before}, e.message end end end @@ -168,7 +168,7 @@ def verify! subject.verify! flunk "Expected exception" rescue Samlr::ConditionsError => e - assert_match /Not on or after/, e.message + assert_match %r{Not on or after}, e.message end end end diff --git a/test/unit/test_logout_request.rb b/test/unit/test_logout_request.rb index d567418..e97d336 100644 --- a/test/unit/test_logout_request.rb +++ b/test/unit/test_logout_request.rb @@ -57,29 +57,29 @@ def capture_stderr request.body end - body.must_include '' - stderr.must_equal "[DEPRECATION] options[:name_id_format] is deprecated. Please use options[:name_id_options][:format] instead\n" + _(body).must_include '' + _(stderr).must_equal "[DEPRECATION] options[:name_id_format] is deprecated. Please use options[:name_id_options][:format] instead\n" end it "understands [:name_id_options][:format]" do options.merge!(:name_id_options => {:format => "some format"}) request = Samlr::LogoutRequest.new(nil, options) - assert_match //, request.body + assert_match %r{}, request.body end it "understands NameQualifier" do options.merge!(:name_id_options => {:name_qualifier => "Some name qualifier"}) request = Samlr::LogoutRequest.new(nil, options) - assert_match /NameQualifier="Some name qualifier"/, request.body + assert_match %r{NameQualifier="Some name qualifier"}, request.body end it "understands SPNameQualifier" do options.merge!(:name_id_options => {:spname_qualifier => "Some SPName qualifier"}) request = Samlr::LogoutRequest.new(nil, options) - assert_match /SPNameQualifier="Some SPName qualifier"/, request.body + assert_match %r{SPNameQualifier="Some SPName qualifier"}, request.body end end diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb index 7d53265..7c51181 100644 --- a/test/unit/test_response.rb +++ b/test/unit/test_response.rb @@ -26,6 +26,45 @@ end end + describe "#signature" do + let(:metadata_doc) do + Nokogiri::XML( + Samlr::Tools::MetadataBuilder.build( + :entity_id => "https://sp.example.com/saml2", + :name_identity_format => "identity_format", + :consumer_service_url => "https://support.sp.example.com/", + :sign_metadata => true, + :certificate => TEST_CERTIFICATE + ) + ) + end + + it "is associated to the response" do + assert subject.signature.present? + end + + describe "when response envelops a signature" do + let(:fingerprint) { Samlr::Certificate.new(TEST_CERTIFICATE.x509).fingerprint.value } + let(:saml_response) { Samlr::Response.new(xml_response_doc, fingerprint: fingerprint) } + + describe "referencing other response" do + let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'multiple_responses.xml'))) } + + it "does not associate it with the response" do + assert saml_response.signature.missing? + end + end + + describe "referencing other element" do + let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'response_signature_wrapping.xml'))) } + + it "does not associate it with the response" do + assert saml_response.signature.missing? + end + end + end + end + describe "XSW attack" do it "should not validate if SAML response is hacked" do document = saml_response_document(:certificate => TEST_CERTIFICATE) @@ -85,12 +124,12 @@ let(:saml_resp) { Samlr::Response.new(saml_response_doc, fingerprint: Samlr::FingerprintSHA256.x509(TEST_CERTIFICATE.x509)) } it "validates the saml response" do - assert_match /user@user.com.evil.com/, saml_response_doc + assert_match %r{user@user.com.evil.com}, saml_response_doc assert saml_resp.verify! end it "ignores the comment and parses the name_id XML node correctly" do - assert_match /user@user.com.evil.com/, saml_response_doc + assert_match %r{user@user.com.evil.com}, saml_response_doc assert_equal "user@user.com.evil.com", saml_resp.name_id end end diff --git a/test/unit/test_response_scenarios.rb b/test/unit/test_response_scenarios.rb index 2410af0..12171a4 100644 --- a/test/unit/test_response_scenarios.rb +++ b/test/unit/test_response_scenarios.rb @@ -40,15 +40,6 @@ end end - describe "invalid multiple saml responses" do - let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'multiple_responses.xml'))) } - let(:saml_response) { Samlr::Response.new(xml_response_doc, fingerprint: '6F:B9:D2:55:52:E8:81:0C:F2:91:97:3D:CE:60:08:82:09:96:27:77:3C:FF:33:A2:0E:04:A6:01:D1:B8:CA:1D') } - - it "fails" do - assert_raises(Samlr::FormatError) { saml_response.verify! } - end - end - describe "an unsatisfied before condition" do subject { saml_response(:certificate => TEST_CERTIFICATE, :not_before => Samlr::Tools::Timestamp.stamp(Time.now + 60)) } diff --git a/test/unit/test_response_signature_wrapping_attack.rb b/test/unit/test_response_signature_wrapping_attack.rb new file mode 100644 index 0000000..b768a30 --- /dev/null +++ b/test/unit/test_response_signature_wrapping_attack.rb @@ -0,0 +1,89 @@ +require File.expand_path("test/test_helper") + +describe Samlr do + # Zendesk acts as a SAML Service Provider (SP). + # The attacker uses publicly available and signed EntityDescriptor, + # takes the real from it and adds it at a or level. + # + # The public signature and certificate are left untouched - copied exactly as found. + # Only the Assertion is attacker-controlled (and unsigned). + # The document is constructed so that, if a SAML parser looks for a valid signature on any element, + # it will pass signature validation on the wrong node (EntityDescriptor), and allows an attacker to log in with the fake assertion. + describe "signature wrapping attack" do + let(:fingerprint) { Samlr::Certificate.new(TEST_CERTIFICATE.x509).fingerprint.value } + # Most IdPs publish a public, signed metadata file. + # - The signature () covers ONLY this EntityDescriptor and matches the data/ID exactly. + # - This is meant to be public so SPs (like Zendesk) can get the IdP’s public keys for SSO. + let(:metadata_doc) do + Nokogiri::XML( + Samlr::Tools::MetadataBuilder.build( + :entity_id => "https://sp.example.com/saml2", + :name_identity_format => "identity_format", + :consumer_service_url => "https://support.sp.example.com/", + :sign_metadata => true, + :certificate => TEST_CERTIFICATE + ) + ) + end + let(:response_doc) do + Nokogiri::XML( + Samlr::Tools::ResponseBuilder.build( + :destination => "https://example.org/saml/endpoint", + :in_response_to => Samlr::Tools.uuid, + # The attacker crafts their own , filling in any identity/attributes. + :name_id => "evil@crack.it", + :audience => "example.org", + :not_on_or_after => Samlr::Tools::Timestamp.stamp(Time.now + 60), + :not_before => Samlr::Tools::Timestamp.stamp(Time.now - 60), + :response_id => Samlr::Tools.uuid, + :skip_conditions => true, + # This Assertion is not signed - meaning the attacker can put anything they want for the user, roles, etc. + :sign_assertion => false, + :sign_response => false, + :certificate => TEST_CERTIFICATE + ) + ) + end + + it "is prevented by checking if the enveloped signature references the response" do + # The attaker, using genuine, validly signed EntityDescriptor, + metadata_entity_descriptor_doc = metadata_doc.at("./md:EntityDescriptor", Samlr::NS_MAP) + # takes the signature out of it + metadata_signature_doc = metadata_entity_descriptor_doc.at("./ds:Signature", Samlr::NS_MAP) + # and adds it the Response, so it pretends the Response's signature. + # According to SAML schema, the needs to be placed after . + response_doc.at("/samlp:Response/saml:Issuer", Samlr::NS_MAP).add_next_sibling(metadata_signature_doc) + + # The signature, instead of referencing the Response, references the EntityDescriptor embedded somewhere in the Response. + response_doc.at("/samlp:Response/samlp:Status/samlp:StatusCode").add_next_sibling("") + response_doc.at("/samlp:Response/samlp:Status/samlp:StatusDetail").add_child(metadata_entity_descriptor_doc) + # check test/fixtures/response_signature_wrapping.xml to see an example message + crafted_saml_response = Samlr::Response.new(Base64.encode64(response_doc.to_xml), fingerprint: fingerprint) + + # The parser detects that the Signature references different node and rejects the Response. + error = assert_raises(Samlr::SignatureError) { crafted_saml_response.verify! } + assert_equal "Neither response nor assertion signed with a certificate", error.message + end + + it "is prevented by checking if the enveloped signature references the assertion" do + assertion_doc = response_doc.xpath("/samlp:Response/saml:Assertion", Samlr::NS_MAP).first + + # The attaker, using genuine, validly signed EntityDescriptor, + metadata_entity_descriptor_doc = metadata_doc.xpath("md:EntityDescriptor", Samlr::NS_MAP).first + # takes the signature out of it + metadata_signature_doc = metadata_entity_descriptor_doc.xpath("ds:Signature", Samlr::NS_MAP).first + # and adds it the Assertion, so it pretends the Assertion's signature. + # According to SAML schema, the needs to be placed after . + assertion_doc.at("./saml:Issuer", Samlr::NS_MAP).add_next_sibling(metadata_signature_doc) + + # The signature, instead of referencing the Assertion, references the EntityDescriptor embedded somewhere in the Assertion. + assertion_doc.at("./saml:Subject/saml:SubjectConfirmation/saml:SubjectConfirmationData", Samlr::NS_MAP).add_child(metadata_entity_descriptor_doc) + # check test/fixtures/assertion_signature_wrapping.xml to see an example message + crafted_saml_response = Samlr::Response.new(Base64.encode64(response_doc.to_xml(Samlr::COMPACT)), fingerprint: fingerprint) + + # The parser detects that the Signature references different node and rejects the Response. + error = assert_raises(Samlr::SignatureError) { crafted_saml_response.verify! } + assert_equal "Neither response nor assertion signed with a certificate", error.message + end + end +end diff --git a/test/unit/tools/test_metadata_builder.rb b/test/unit/tools/test_metadata_builder.rb index 5e39b0f..8b20245 100644 --- a/test/unit/tools/test_metadata_builder.rb +++ b/test/unit/tools/test_metadata_builder.rb @@ -2,25 +2,38 @@ describe Samlr::Tools::MetadataBuilder do describe "#build" do - before do - @xml = Samlr::Tools::MetadataBuilder.build({ + let(:options) do + { :entity_id => "https://sp.example.com/saml2", :name_identity_format => "identity_format", :consumer_service_url => "https://support.sp.example.com/" - }) - - @doc = Nokogiri::XML(@xml) { |c| c.strict } + } end + let(:xml) { Samlr::Tools::MetadataBuilder.build(options) } + let(:doc) { Nokogiri::XML(xml) { |c| c.strict } } it "generates a metadata document" do - assert_equal "EntityDescriptor", @doc.root.name - assert_equal "identity_format", @doc.at("//md:NameIDFormat", { "md" => Samlr::NS_MAP["md"] }).text + assert_equal "EntityDescriptor", doc.root.name + assert_equal "identity_format", doc.at("//md:NameIDFormat", { "md" => Samlr::NS_MAP["md"] }).text end it "validates against schemas" do - result = Samlr::Tools.validate(:document => @xml, :schema => Samlr::META_SCHEMA) + result = Samlr::Tools.validate(:document => xml, :schema => Samlr::META_SCHEMA) assert result end + it "does not sign metadata by default" do + assert_nil doc.xpath("md:EntityDescriptor/ds:Signature", Samlr::NS_MAP).first + end + + describe "when prompted to add a signature" do + before do + options[:sign_metadata] = true + end + + it "signs metadata" do + refute_nil doc.xpath("md:EntityDescriptor/ds:Signature", Samlr::NS_MAP).first + end + end end end diff --git a/test/unit/tools/test_response_builder.rb b/test/unit/tools/test_response_builder.rb index 3d2affd..ecf1d43 100644 --- a/test/unit/tools/test_response_builder.rb +++ b/test/unit/tools/test_response_builder.rb @@ -18,7 +18,7 @@ it "doesn't include the name_id when nil" do result = saml_response_document(:certificate => @certificate, :name_id => nil) - refute_match /nameid/i, result + refute_match %r{nameid}i, result end end end