Skip to content

Commit 9f70c39

Browse files
zendesk-mradmacherzendesk-mradmacher
committed
fix signature wrapping vulnerability
1 parent c7a7716 commit 9f70c39

13 files changed

+299
-36
lines changed

‎lib/samlr/signature.rb‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@ def initialize(original, prefix, options)
1515
@document = original.dup
1616
@prefix = prefix
1717
@options = options
18+
@signature = nil
1819

19-
if @signature = document.at("#{prefix}/ds:Signature", NS_MAP)
20-
@signature.remove # enveloped signatures only
21-
end
20+
id = document.at("#{prefix}")&.attribute('ID')
21+
@signature = document.at("#{prefix}/ds:Signature/ds:SignedInfo/ds:Reference[@URI='##{id}']", NS_MAP)&.parent&.parent if id
22+
@signature.remove if @signature # enveloped signatures only
2223

2324
@fingerprint = if options[:fingerprint]
2425
Fingerprint.from_string(options[:fingerprint])

‎lib/samlr/tools/metadata_builder.rb‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,14 @@ def self.build(options = {})
1313
name_identity_format = options[:name_identity_format]
1414
consumer_service_url = options[:consumer_service_url]
1515
consumer_service_binding = options[:consumer_service_binding] || "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
16+
metadata_id = options[:metadata_id] || Samlr::Tools.uuid
17+
sign_metadata = options[:sign_metadata] || false
1618

1719
# Mandatory
18-
entity_id = options.fetch(:entity_id)
20+
entity_id = options.fetch(:entity_id)
1921

2022
builder = Nokogiri::XML::Builder.new do |xml|
21-
xml.EntityDescriptor("xmlns:md" => NS_MAP["md"], "entityID" => entity_id) do
23+
xml.EntityDescriptor("xmlns:md" => NS_MAP["md"], "ID" => metadata_id, "entityID" => entity_id) do
2224
xml.doc.root.namespace = xml.doc.root.namespace_definitions.find { |ns| ns.prefix == "md" }
2325

2426
xml["md"].SPSSODescriptor("protocolSupportEnumeration" => NS_MAP["samlp"]) do
@@ -33,7 +35,14 @@ def self.build(options = {})
3335
end
3436
end
3537

36-
builder.to_xml(COMPACT)
38+
metadata = builder.doc
39+
40+
if sign_metadata
41+
metadata_options = options.merge(namespaces: [])
42+
metadata = ResponseBuilder.sign(metadata, metadata_id, metadata_options)
43+
end
44+
45+
metadata.to_xml(COMPACT)
3746
end
3847

3948
end

‎lib/samlr/tools/response_builder.rb‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,11 @@ def self.sign(document, element_id, options)
125125
end unless skip_keyinfo
126126
end
127127
# digest.root.last_element_child.after "<SignatureValue>#{signature}</SignatureValue>"
128-
element.at("./saml:Issuer", NS_MAP).add_next_sibling(digest)
128+
if element.at("./saml:Issuer", NS_MAP)
129+
element.at("./saml:Issuer", NS_MAP).add_next_sibling(digest)
130+
else
131+
element.children.first.add_previous_sibling(digest)
132+
end
129133

130134
document
131135
end

‎test/fixtures/assertion_signature_wrapping.xml‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="samlr-be3960f5-1528-411b-ba60-aa57528d2a9e" InResponseTo="samlr-b09700a2-89ba-4c39-b7ff-6bcb34e6e10d" Version="2.0" IssueInstant="2025-10-30T13:34:17Z" Destination="https://example.org/saml/endpoint">
3+
<saml:Issuer>ResponseBuilder IdP</saml:Issuer>
4+
<samlp:Status>
5+
<samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
6+
</samlp:Status>
7+
<saml:Assertion ID="samlr-d51de4a4-e1f2-4b21-a10e-2e309f360160" IssueInstant="2025-10-30T13:34:17Z" Version="2.0">
8+
<saml:Issuer>ResponseBuilder IdP</saml:Issuer>
9+
<Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
10+
<SignedInfo>
11+
<CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
12+
<SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
13+
<Reference URI="#samlr-31d06f27-5176-430b-b087-ac831b9ab9e8">
14+
<Transforms>
15+
<Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
16+
<Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#">
17+
<InclusiveNamespaces xmlns="http://www.w3.org/2001/10/xml-exc-c14n#" PrefixList=""/>
18+
</Transform>
19+
</Transforms>
20+
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
21+
<DigestValue>RIrP+dFpc1LqZr0nUVvVNNrqBj0=</DigestValue>
22+
</Reference>
23+
</SignedInfo>
24+
<SignatureValue>NUShdJOltVGPmJknieVEqwKV13nRRZNrNaOE61YmAU3VvRxWBQiJClEV7VJ2jQlUxj36+5p8CSuhwwghfm3rnQ==</SignatureValue>
25+
<KeyInfo>
26+
<X509Data>
27+
<X509Certificate>MIIBjTCCATegAwIBAgIBATANBgkqhkiG9w0BAQUFADBPMQswCQYDVQQGEwJVUzEUMBIGA1UECgwLZXhhbXBsZS5vcmcxHTAbBgNVBAsMFFphbWwgUmVzcG9uc2VCdWlsZGVyMQswCQYDVQQDDAJDQTAeFw0xMjA4MDgwMjAxMDlaFw0zMjA4MDMwMjAxMTRaME8xCzAJBgNVBAYTAlVTMRQwEgYDVQQKDAtleGFtcGxlLm9yZzEdMBsGA1UECwwUWmFtbCBSZXNwb25zZUJ1aWxkZXIxCzAJBgNVBAMMAkNBMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALb9pPmyHrbZJMDLLkVsHzzXvP7DFcPiYdaNU50l5znRr8ZGhwRZFAwKroOxXwhK5e9lz06C+kGqnL1v10h1BEUCAwEAATANBgkqhkiG9w0BAQUFAANBAKU10RznL2p7xRhO9vOh0CY+gWYmT2kbkLTVRYLApghQFAW8EzIHC/NggfEHM554ykzbbPwjSvM7cRBBDHYuWoY=</X509Certificate>
28+
</X509Data>
29+
</KeyInfo>
30+
</Signature>
31+
<saml:Subject>
32+
<saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">[email protected]</saml:NameID>
33+
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
34+
<saml:SubjectConfirmationData InResponseTo="samlr-b09700a2-89ba-4c39-b7ff-6bcb34e6e10d" NotOnOrAfter="2025-10-30T13:35:17Z" Recipient="https://example.org/saml/endpoint">
35+
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" ID="samlr-31d06f27-5176-430b-b087-ac831b9ab9e8" entityID="https://sp.example.com/saml2">
36+
<md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
37+
<md:NameIDFormat>identity_format</md:NameIDFormat>
38+
<md:AssertionConsumerService index="0" Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://support.sp.example.com/"/>
39+
</md:SPSSODescriptor>
40+
</md:EntityDescriptor>
41+
</saml:SubjectConfirmationData>
42+
</saml:SubjectConfirmation>
43+
</saml:Subject>
44+
<saml:AuthnStatement AuthnInstant="2025-10-30T13:34:17Z" SessionIndex="samlr-d51de4a4-e1f2-4b21-a10e-2e309f360160">
45+
<saml:AuthnContext>
46+
<saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef>
47+
</saml:AuthnContext>
48+
</saml:AuthnStatement>
49+
</saml:Assertion>
50+
</samlp:Response>

‎test/fixtures/response_signature_wrapping.xml‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="samlr-32966210-941b-414d-9e74-dd16f7e676ae" InResponseTo="samlr-2bb5bd98-44c8-4676-bb68-7a8673e141c0" Version="2.0" IssueInstant="2025-10-30T13:23:08Z" Destination="https://example.org/saml/endpoint">
3+
<saml:Issuer>ResponseBuilder IdP</saml:Issuer>
4+
<Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
5+
<SignedInfo>
6+
<CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
7+
<SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
8+
<Reference URI="#samlr-222fe0da-90c6-44ed-93a4-3d79a01596d4">
9+
<Transforms>
10+
<Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
11+
<Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#">
12+
<InclusiveNamespaces xmlns="http://www.w3.org/2001/10/xml-exc-c14n#" PrefixList=""/>
13+
</Transform>
14+
</Transforms>
15+
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
16+
<DigestValue>qdW6Sx1sUmpN/joEMOUFPO1HOAc=</DigestValue>
17+
</Reference>
18+
</SignedInfo>
19+
<SignatureValue>i+RCXmzCX5FMlh+0xgxkN6QjTJ5wzku36gv8hmUPB0GAN2AXHaSdYDtFLKCTa4F3MoFZq6oJeqSBfhkg/h1IKw==</SignatureValue>
20+
<KeyInfo>
21+
<X509Data>
22+
<X509Certificate>MIIBjTCCATegAwIBAgIBATANBgkqhkiG9w0BAQUFADBPMQswCQYDVQQGEwJVUzEUMBIGA1UECgwLZXhhbXBsZS5vcmcxHTAbBgNVBAsMFFphbWwgUmVzcG9uc2VCdWlsZGVyMQswCQYDVQQDDAJDQTAeFw0xMjA4MDgwMjAxMDlaFw0zMjA4MDMwMjAxMTRaME8xCzAJBgNVBAYTAlVTMRQwEgYDVQQKDAtleGFtcGxlLm9yZzEdMBsGA1UECwwUWmFtbCBSZXNwb25zZUJ1aWxkZXIxCzAJBgNVBAMMAkNBMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALb9pPmyHrbZJMDLLkVsHzzXvP7DFcPiYdaNU50l5znRr8ZGhwRZFAwKroOxXwhK5e9lz06C+kGqnL1v10h1BEUCAwEAATANBgkqhkiG9w0BAQUFAANBAKU10RznL2p7xRhO9vOh0CY+gWYmT2kbkLTVRYLApghQFAW8EzIHC/NggfEHM554ykzbbPwjSvM7cRBBDHYuWoY=</X509Certificate>
23+
</X509Data>
24+
</KeyInfo>
25+
</Signature>
26+
<samlp:Status>
27+
<samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
28+
<samlp:StatusDetail>
29+
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" ID="samlr-222fe0da-90c6-44ed-93a4-3d79a01596d4" entityID="https://sp.example.com/saml2">
30+
<md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
31+
<md:NameIDFormat>identity_format</md:NameIDFormat>
32+
<md:AssertionConsumerService index="0" Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://support.sp.example.com/"/>
33+
</md:SPSSODescriptor>
34+
</md:EntityDescriptor>
35+
</samlp:StatusDetail>
36+
</samlp:Status>
37+
<saml:Assertion ID="samlr-43c490e8-6c91-4a6f-bf28-b52134404be6" IssueInstant="2025-10-30T13:23:08Z" Version="2.0">
38+
<saml:Issuer>ResponseBuilder IdP</saml:Issuer>
39+
<saml:Subject>
40+
<saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">[email protected]</saml:NameID>
41+
<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
42+
<saml:SubjectConfirmationData InResponseTo="samlr-2bb5bd98-44c8-4676-bb68-7a8673e141c0" NotOnOrAfter="2025-10-30T13:24:08Z" Recipient="https://example.org/saml/endpoint"/>
43+
</saml:SubjectConfirmation>
44+
</saml:Subject>
45+
<saml:AuthnStatement AuthnInstant="2025-10-30T13:23:08Z" SessionIndex="samlr-43c490e8-6c91-4a6f-bf28-b52134404be6">
46+
<saml:AuthnContext>
47+
<saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef>
48+
</saml:AuthnContext>
49+
</saml:AuthnStatement>
50+
</saml:Assertion>
51+
</samlp:Response>

‎test/unit/test_assertion.rb‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,22 @@
5252
end
5353
end
5454

55+
describe "#signature" do
56+
it "is associated to the assertion" do
57+
assert subject.signature.present?
58+
end
59+
60+
describe "when assertion envelops a signature referencing other element" do
61+
let(:fingerprint) { Samlr::Certificate.new(TEST_CERTIFICATE.x509).fingerprint.value }
62+
let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'assertion_signature_wrapping.xml'))) }
63+
subject { Samlr::Response.new(xml_response_doc, fingerprint: fingerprint).assertion }
64+
65+
it "does not associate it with the assertion" do
66+
assert subject.signature.missing?
67+
end
68+
end
69+
end
70+
5571
describe "#verify!" do
5672
let(:condition) do
5773
Class.new do

‎test/unit/test_condition.rb‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ def verify!
3737
verify!
3838
flunk "Expected exception"
3939
rescue Samlr::ConditionsError => e
40-
assert_match /Audience/, e.message
40+
assert_match %r{Audience}, e.message
4141
end
4242
end
4343
end
@@ -132,7 +132,7 @@ def verify!
132132
verify!
133133
flunk "Expected exception"
134134
rescue Samlr::ConditionsError => e
135-
assert_match /Audience/, e.message
135+
assert_match %r{Audience}, e.message
136136
end
137137
end
138138
end
@@ -151,7 +151,7 @@ def verify!
151151
subject.verify!
152152
flunk "Expected exception"
153153
rescue Samlr::ConditionsError => e
154-
assert_match /Not before/, e.message
154+
assert_match %r{Not before}, e.message
155155
end
156156
end
157157
end
@@ -168,7 +168,7 @@ def verify!
168168
subject.verify!
169169
flunk "Expected exception"
170170
rescue Samlr::ConditionsError => e
171-
assert_match /Not on or after/, e.message
171+
assert_match %r{Not on or after}, e.message
172172
end
173173
end
174174
end

‎test/unit/test_logout_request.rb‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,29 +57,29 @@ def capture_stderr
5757
request.body
5858
end
5959

60-
body.must_include '<saml:NameID Format="some format">'
61-
stderr.must_equal "[DEPRECATION] options[:name_id_format] is deprecated. Please use options[:name_id_options][:format] instead\n"
60+
_(body).must_include '<saml:NameID Format="some format">'
61+
_(stderr).must_equal "[DEPRECATION] options[:name_id_format] is deprecated. Please use options[:name_id_options][:format] instead\n"
6262
end
6363

6464
it "understands [:name_id_options][:format]" do
6565
options.merge!(:name_id_options => {:format => "some format"})
6666
request = Samlr::LogoutRequest.new(nil, options)
6767

68-
assert_match /<saml:NameID Format="some format">/, request.body
68+
assert_match %r{<saml:NameID Format="some format">}, request.body
6969
end
7070

7171
it "understands NameQualifier" do
7272
options.merge!(:name_id_options => {:name_qualifier => "Some name qualifier"})
7373
request = Samlr::LogoutRequest.new(nil, options)
7474

75-
assert_match /NameQualifier="Some name qualifier"/, request.body
75+
assert_match %r{NameQualifier="Some name qualifier"}, request.body
7676
end
7777

7878
it "understands SPNameQualifier" do
7979
options.merge!(:name_id_options => {:spname_qualifier => "Some SPName qualifier"})
8080
request = Samlr::LogoutRequest.new(nil, options)
8181

82-
assert_match /SPNameQualifier="Some SPName qualifier"/, request.body
82+
assert_match %r{SPNameQualifier="Some SPName qualifier"}, request.body
8383
end
8484
end
8585

‎test/unit/test_response.rb‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,45 @@
2626
end
2727
end
2828

29+
describe "#signature" do
30+
let(:metadata_doc) do
31+
Nokogiri::XML(
32+
Samlr::Tools::MetadataBuilder.build(
33+
:entity_id => "https://sp.example.com/saml2",
34+
:name_identity_format => "identity_format",
35+
:consumer_service_url => "https://support.sp.example.com/",
36+
:sign_metadata => true,
37+
:certificate => TEST_CERTIFICATE
38+
)
39+
)
40+
end
41+
42+
it "is associated to the response" do
43+
assert subject.signature.present?
44+
end
45+
46+
describe "when response envelops a signature" do
47+
let(:fingerprint) { Samlr::Certificate.new(TEST_CERTIFICATE.x509).fingerprint.value }
48+
let(:saml_response) { Samlr::Response.new(xml_response_doc, fingerprint: fingerprint) }
49+
50+
describe "referencing other response" do
51+
let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'multiple_responses.xml'))) }
52+
53+
it "does not associate it with the response" do
54+
assert saml_response.signature.missing?
55+
end
56+
end
57+
58+
describe "referencing other element" do
59+
let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'response_signature_wrapping.xml'))) }
60+
61+
it "does not associate it with the response" do
62+
assert saml_response.signature.missing?
63+
end
64+
end
65+
end
66+
end
67+
2968
describe "XSW attack" do
3069
it "should not validate if SAML response is hacked" do
3170
document = saml_response_document(:certificate => TEST_CERTIFICATE)
@@ -85,12 +124,12 @@
85124
let(:saml_resp) { Samlr::Response.new(saml_response_doc, fingerprint: Samlr::FingerprintSHA256.x509(TEST_CERTIFICATE.x509)) }
86125

87126
it "validates the saml response" do
88-
assert_match /[email protected]<!---->.evil.com/, saml_response_doc
127+
assert_match %r{[email protected]<!---->.evil.com}, saml_response_doc
89128
assert saml_resp.verify!
90129
end
91130

92131
it "ignores the comment and parses the name_id XML node correctly" do
93-
assert_match /[email protected]<!---->.evil.com/, saml_response_doc
132+
assert_match %r{[email protected]<!---->.evil.com}, saml_response_doc
94133
assert_equal "[email protected]", saml_resp.name_id
95134
end
96135
end

‎test/unit/test_response_scenarios.rb‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,6 @@
4040
end
4141
end
4242

43-
describe "invalid multiple saml responses" do
44-
let(:xml_response_doc) { Base64.encode64(File.read(File.join('.', 'test', 'fixtures', 'multiple_responses.xml'))) }
45-
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') }
46-
47-
it "fails" do
48-
assert_raises(Samlr::FormatError) { saml_response.verify! }
49-
end
50-
end
51-
5243
describe "an unsatisfied before condition" do
5344
subject { saml_response(:certificate => TEST_CERTIFICATE, :not_before => Samlr::Tools::Timestamp.stamp(Time.now + 60)) }
5445

0 commit comments

Comments
 (0)