Skip to content

Commit

Permalink
Add ValidationError if the SignatureAlgorithm does not have SHA256
Browse files Browse the repository at this point in the history
  • Loading branch information
Sgtpluck committed Nov 27, 2023
1 parent b1bd75b commit 0bc0aa9
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 6 deletions.
1 change: 0 additions & 1 deletion lib/saml_idp/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,5 @@ def service_provider_finder
config.service_provider.finder
end
private :service_provider_finder

end
end
7 changes: 7 additions & 0 deletions lib/saml_idp/xml_security.rb
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,13 @@ def verify_signature(base64_cert, sig_alg, signature, canon_string, soft)
cert = OpenSSL::X509::Certificate.new(cert_text)
signature_algorithm = algorithm(sig_alg)

if signature_algorithm != OpenSSL::Digest::SHA256 && !soft
raise ValidationError.new(
'All signatures must use RSA SHA-256',
:require_sha256
)
end

unless cert.public_key.verify(signature_algorithm.new, signature, canon_string)
return soft ? false : (raise ValidationError.new(
'Key validation error',
Expand Down
14 changes: 14 additions & 0 deletions spec/lib/saml_idp/request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,20 @@ module SamlIdp
expect(subject.errors.include?(:digest_mismatch)).to be true
end
end

describe 'request not using SHA256 hashing algorithm' do
let(:options) { {} }
let(:request_saml) { signed_auth_request(algo: "sha1") }

it 'is not valid' do
expect(subject.valid?).to eq false
end

it 'adds a require_sha256 error' do
subject.valid?
expect(subject.errors.include?(:require_sha256)).to be true
end
end
end
end
end
Expand Down
10 changes: 5 additions & 5 deletions spec/support/saml_request_macros.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ def url(saml_settings)
auth_request.create(saml_settings)
end

def signed_auth_request(embed: true)
CGI.unescape(url(signed_saml_settings(embed: embed)).split('=').last)
def signed_auth_request(embed: true, algo: "sha256")
CGI.unescape(url(signed_saml_settings(embed: embed, algo: algo)).split('=').last)
end

def signed_auth_request_options
Expand Down Expand Up @@ -78,14 +78,14 @@ def saml_settings(requested_saml_acs_url = 'https://foo.example.com/saml/consume
end
end

def signed_saml_settings(embed: true)
def signed_saml_settings(embed: true, algo: 'sha256')
settings = saml_settings('https://foo.example.com/saml/consume')
settings.security = {
embed_sign: embed,
authn_requests_signed: true,
want_assertions_signed: true,
digest_method: 'http://www.w3.org/2001/04/xmlenc#sha256',
signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'
digest_method: "http://www.w3.org/2001/04/xmlenc#sha256",
signature_method: "http://www.w3.org/2001/04/xmldsig-more##{algo}"
}
settings
end
Expand Down
3 changes: 3 additions & 0 deletions spec/support/security_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ def fixture(document, base64 = true)
end
end

# TODO: go back and name these methods and files in semantically meaningful ways.
# and/or update and use the SamlRequestMacros to give tests more control over
# values being passed in
def response_document
@response_document ||= File.read(File.join(File.dirname(__FILE__), 'responses', 'response1.xml.base64'))
end
Expand Down
12 changes: 12 additions & 0 deletions spec/xml_security_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,22 @@ module SamlIdp
)
end

it 'raises require SHA256 error' do
response = Base64.decode64(response_document)
response.sub!('<ds:DigestValue>pJQ7MS/ek4KRRWGmv/H43ReHYMs=</ds:DigestValue>',
'<ds:DigestValue>b9xsAXLsynugg3Wc1CI3kpWku+0=</ds:DigestValue>')
document = XMLSecurity::SignedDocument.new(response)
base64cert = document.elements['//ds:X509Certificate'].text
expect { document.validate_doc(base64cert, false) }.to(
raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256')
)
end

it 'raises Key validation error' do
response = Base64.decode64(response_document)
response.sub!('<ds:DigestValue>pJQ7MS/ek4KRRWGmv/H43ReHYMs=</ds:DigestValue>',
'<ds:DigestValue>b9xsAXLsynugg3Wc1CI3kpWku+0=</ds:DigestValue>')
response.gsub!('<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>', '<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha256"/>')
document = XMLSecurity::SignedDocument.new(response)
base64cert = document.elements['//ds:X509Certificate'].text
expect { document.validate_doc(base64cert, false) }.to(
Expand Down

0 comments on commit 0bc0aa9

Please sign in to comment.