From 751dbf50a51131b13d55989395f9b115045f9737 Mon Sep 17 00:00:00 2001 From: Ivan Kanakarakis Date: Mon, 4 Jan 2021 22:52:07 +0200 Subject: [PATCH] Fix CVE-2021-21239 - Restrict the key data that xmlsec1 accepts to only x509 certs All users of pysaml2 that use the default CryptoBackendXmlSec1 backend and need to verify signed SAML documents are impacted. pysaml2 <= 6.4.1 does not ensure that a signed SAML document is correctly signed. The default CryptoBackendXmlSec1 backend is using the xmlsec1 binary to verify the signature of signed SAML documents, but by default, xmlsec1 accepts any type of key found within the given document. xmlsec1 needs to be configured explicitly to only use only x509 certificates for the verification process of the SAML document signature. Signed-off-by: Ivan Kanakarakis --- src/saml2/sigver.py | 1 + tests/test_xmlsec1_key_data.py | 81 +++++++++++++++++++ .../signed-assertion-random-embedded-cert.xml | 47 +++++++++++ .../signed-assertion-with-hmac.xml | 49 +++++++++++ .../signed-response-with-hmac.xml | 49 +++++++++++ 5 files changed, 227 insertions(+) create mode 100644 tests/test_xmlsec1_key_data.py create mode 100644 tests/xmlsec1-keydata/signed-assertion-random-embedded-cert.xml create mode 100644 tests/xmlsec1-keydata/signed-assertion-with-hmac.xml create mode 100644 tests/xmlsec1-keydata/signed-response-with-hmac.xml diff --git a/src/saml2/sigver.py b/src/saml2/sigver.py index 0e8f1942f..dae6900d5 100644 --- a/src/saml2/sigver.py +++ b/src/saml2/sigver.py @@ -869,6 +869,7 @@ def validate_signature(self, signedtext, cert_file, cert_type, node_name, node_i self.xmlsec, '--verify', '--enabled-reference-uris', 'empty,same-doc', + '--enabled-key-data', 'raw-x509-cert', '--pubkey-cert-{type}'.format(type=cert_type), cert_file, '--id-attr:ID', node_name, ] diff --git a/tests/test_xmlsec1_key_data.py b/tests/test_xmlsec1_key_data.py new file mode 100644 index 000000000..2149659f9 --- /dev/null +++ b/tests/test_xmlsec1_key_data.py @@ -0,0 +1,81 @@ +from datetime import datetime +from dateutil import parser +from unittest.mock import Mock +from unittest.mock import patch + +from pytest import raises + +from saml2.config import config_factory +from saml2.response import authn_response +from saml2.sigver import SignatureError + +from pathutils import dotname +from pathutils import full_path + + +SIGNED_RESPONSE_HMAC = full_path("xmlsec1-keydata/signed-response-with-hmac.xml") +SIGNED_ASSERTION_HMAC = full_path("xmlsec1-keydata/signed-assertion-with-hmac.xml") +SIGNED_ASSERTION_RANDOM_EMBEDDED_CERT = full_path("xmlsec1-keydata/signed-assertion-random-embedded-cert.xml") + + +class TestAuthnResponse: + @patch('saml2.response.validate_on_or_after', return_value=True) + def test_signed_response_with_hmac_should_fail(self, mock_validate_on_or_after): + conf = config_factory("sp", dotname("server_conf")) + ar = authn_response(conf, return_addrs="https://example.org/acs/post") + ar.issue_instant_ok = Mock(return_value=True) + + with open(SIGNED_RESPONSE_HMAC) as fp: + xml_response = fp.read() + + ar.outstanding_queries = {"id-abc": "http://localhost:8088/sso"} + ar.timeslack = 10000 + + # .loads checks the response signature + with raises(SignatureError): + ar.loads(xml_response, decode=False) + + assert ar.ava is None + assert ar.name_id is None + + @patch('saml2.response.validate_on_or_after', return_value=True) + def test_signed_assertion_with_hmac_should_fail(self, mock_validate_on_or_after): + conf = config_factory("sp", dotname("server_conf")) + ar = authn_response(conf, return_addrs="https://example.org/acs/post") + ar.issue_instant_ok = Mock(return_value=True) + + with open(SIGNED_ASSERTION_HMAC) as fp: + xml_response = fp.read() + + ar.outstanding_queries = {"id-abc": "http://localhost:8088/sso"} + ar.timeslack = 10000 + + # .loads does not check the assertion, only the response signature + # use .verify to verify the contents of the response + assert ar.loads(xml_response, decode=False) + with raises(SignatureError): + ar.verify() + + assert ar.ava is None + assert ar.name_id is None + + @patch('saml2.response.validate_on_or_after', return_value=True) + def test_signed_assertion_with_random_embedded_cert_should_be_ignored(self, mock_validate_on_or_after): + """ + if the embedded cert is not ignored then verification will fail + """ + + conf = config_factory("sp", dotname("server_conf")) + ar = authn_response(conf, return_addrs="https://51.15.251.81.xip.io/acs/post") + ar.issue_instant_ok = Mock(return_value=True) + + with open(SIGNED_ASSERTION_RANDOM_EMBEDDED_CERT) as fp: + xml_response = fp.read() + + ar.outstanding_queries = {"id-abc": "http://localhost:8088/sso"} + ar.timeslack = 10000 + + # .loads does not check the assertion, only the response signature + # use .verify to verify the contents of the response + assert ar.loads(xml_response, decode=False) + assert ar.verify() diff --git a/tests/xmlsec1-keydata/signed-assertion-random-embedded-cert.xml b/tests/xmlsec1-keydata/signed-assertion-random-embedded-cert.xml new file mode 100644 index 000000000..3db22337b --- /dev/null +++ b/tests/xmlsec1-keydata/signed-assertion-random-embedded-cert.xml @@ -0,0 +1,47 @@ + + + urn:mace:example.com:saml:roland:idp + + + + + urn:mace:example.com:saml:roland:idp + + + + + + + + + + + NHB0WhPWj5OyRz9N52fZrEBWK3dXT2pVVT54f4kg1tM= + + + Mo4ZheAEDvdPQwWvT5SOYZZ2IBELwtmBpdsn+Th+IvsanychWQ6JHYKTI8hl+3DigbqQwdsqet8n9sfdvr+D+Q7XozjVaFPdzUGC9d96Mn/vrc+JIP/ESoDjDUQEsoSBhUFlrbu7tPJDJehPgd/maIwd/GqEHWXFlm1ZWVCmaH8= + + + MIIFLDCCBBSgAwIBAgISA3UxXAgBWRS1D74GOLiAjky6MA0GCSqGSIb3DQEBCwUAMDIxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MQswCQYDVQQDEwJSMzAeFw0yMTAxMTcyMDIxNTNaFw0yMTA0MTcyMDIxNTNaMB4xHDAaBgNVBAMTEzUxLjE1LjI1MS44MS54aXAuaW8wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDkjSN9vZDOwE1m7g1vyiwBsKBKNItyy05BcHUkM8fabBcsavugT8uE4wYz5aeZrnKb5dbDLHaZe6Dl5GHgRO8s7REwSJ/BHT3/eMaEakLwIGE5/6QSWuBjOawPfmYarW5IqoITjSt/o/jxu3haouqbr7XYf1WOuZmc6iwGnEgm0+cVB4CA0VGnnLYfsjp9iMt3pFI8a8ipdwp5lfzZU+j8JMVEn6SZhNjjTAjcakBQmZv4Q5/yU6yqfGjG47DO62xB/PPbDy78hDorER2v8UkoDTGV4aZrZaNltHBUxNohIiQnkhuakMmbf0NhA2ExBJw6KTCCxfYkyUX3CgYzaAnxAgMBAAGjggJOMIICSjAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFHD7gcF0sNITPqhwcUlFyOBrkxRFMB8GA1UdIwQYMBaAFBQusxe3WFbLrlAJQOYfr52LFMLGMFUGCCsGAQUFBwEBBEkwRzAhBggrBgEFBQcwAYYVaHR0cDovL3IzLm8ubGVuY3Iub3JnMCIGCCsGAQUFBzAChhZodHRwOi8vcjMuaS5sZW5jci5vcmcvMB4GA1UdEQQXMBWCEzUxLjE1LjI1MS44MS54aXAuaW8wTAYDVR0gBEUwQzAIBgZngQwBAgEwNwYLKwYBBAGC3xMBAQEwKDAmBggrBgEFBQcCARYaaHR0cDovL2Nwcy5sZXRzZW5jcnlwdC5vcmcwggEEBgorBgEEAdZ5AgQCBIH1BIHyAPAAdQBc3EOS/uarRUSxXprUVuYQN/vV+kfcoXOUsl7m9scOygAAAXcSOcqvAAAEAwBGMEQCIFhvPWYY+2VO55bHO1Sa9zJQk7B1kvCi7sfDCxAYp7mEAiBTKx6XD0GkfNGhji0LGuelvfD7gZOUSzURlJW1ahYcgQB3APZclC/RdzAiFFQYCDCUVo7jTRMZM7/fDC8gC8xO8WTjAAABdxI5yq4AAAQDAEgwRgIhAONNwcNZ66IxsWcUNDS0B9KV8Kk4VS9b/wUFNBHAQl3SAiEAxO5GwgaK3glL/6L/J7qpiedJBAs3h5406MWC0v4uYZ8wDQYJKoZIhvcNAQELBQADggEBAAZgbgfOb4+uI9kMGF4fMiompHeUFDXGyIND6y4FsfWHJG4Fn3aG+VQN/UtHeO8UusjS13/2yw3O+PeNTstBl+q6Ssega8zTYx2j3h3RFqM9JR8SWa83B0UTgyaxX3PTmfegV4/RZxC7KQ8pqjcLwKJSTgZF6W40Jo16tKVoi0VQY/2Gre6E9D1tPVw//mDGJST/5IcbFvtr79uft76IA+T674qNgAriBKxWncSbGzE42w2QsYsGMHHJn3vKbNl7alll9eJBqvdi1Q7ay86oI7NDQ0PPwjnB0/i4BOO0qQBcBSUIPPEChcrooEqN9PwM20aoyIjje0rtDGSxnEQRrg0= + MIIEZTCCA02gAwIBAgIQQAF1BIMUpMghjISpDBbN3zANBgkqhkiG9w0BAQsFADA/MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMTDkRTVCBSb290IENBIFgzMB4XDTIwMTAwNzE5MjE0MFoXDTIxMDkyOTE5MjE0MFowMjELMAkGA1UEBhMCVVMxFjAUBgNVBAoTDUxldCdzIEVuY3J5cHQxCzAJBgNVBAMTAlIzMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuwIVKMz2oJTTDxLsjVWSw/iC8ZmmekKIp10mqrUrucVMsa+Oa/l1yKPXD0eUFFU1V4yeqKI5GfWCPEKpTm71O8Mu243AsFzzWTjn7c9p8FoLG77AlCQlh/o3cbMT5xys4Zvv2+Q7RVJFlqnBU840yFLuta7tj95gcOKlVKu2bQ6XpUA0ayvTvGbrZjR8+muLj1cpmfgwF126cm/7gcWt0oZYPRfH5wm78Sv3htzB2nFd1EbjzK0lwYi8YGd1ZrPxGPeiXOZT/zqItkel/xMY6pgJdz+dU/nPAeX1pnAXFK9jpP+Zs5Od3FOnBv5IhR2haa4ldbsTzFID9e1RoYvbFQIDAQABo4IBaDCCAWQwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8EBAMCAYYwSwYIKwYBBQUHAQEEPzA9MDsGCCsGAQUFBzAChi9odHRwOi8vYXBwcy5pZGVudHJ1c3QuY29tL3Jvb3RzL2RzdHJvb3RjYXgzLnA3YzAfBgNVHSMEGDAWgBTEp7Gkeyxx+tvhS5B1/8QVYIWJEDBUBgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEEAYLfEwEBATAwMC4GCCsGAQUFBwIBFiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2VuY3J5cHQub3JnMDwGA1UdHwQ1MDMwMaAvoC2GK2h0dHA6Ly9jcmwuaWRlbnRydXN0LmNvbS9EU1RST09UQ0FYM0NSTC5jcmwwHQYDVR0OBBYEFBQusxe3WFbLrlAJQOYfr52LFMLGMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjANBgkqhkiG9w0BAQsFAAOCAQEA2UzgyfWEiDcx27sT4rP8i2tiEmxYt0l+PAK3qB8oYevO4C5z70kHejWEHx2taPDY/laBL21/WKZuNTYQHHPD5b1tXgHXbnL7KqC401dk5VvCadTQsvd8S8MXjohyc9z9/G2948kLjmE6Flh9dDYrVYA9x2O+hEPGOaEOa1eePynBgPayvUfLqjBstzLhWVQLGAkXXmNs+5ZnPBxzDJOLxhF2JIbeQAcH5H0tZrUlo5ZYyOqA7s9pO5b85o3AM/OJ+CktFBQtfvBhcJVd9wvlwPsk+uyOy2HI7mNxKKgsBTt375teA2TwUdHkhVNcsAKX1H7GNNLOEADksd86wuoXvg== + + + + + attack-name-id + + + + + + + urn:mace:example.com:saml:roland:sp + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified + + + + diff --git a/tests/xmlsec1-keydata/signed-assertion-with-hmac.xml b/tests/xmlsec1-keydata/signed-assertion-with-hmac.xml new file mode 100644 index 000000000..e6a28c03b --- /dev/null +++ b/tests/xmlsec1-keydata/signed-assertion-with-hmac.xml @@ -0,0 +1,49 @@ + + + urn:mace:example.com:saml:roland:idp + + + + + urn:mace:example.com:saml:roland:idp + + + + + + + + + + + 3eSifM9ENDpX4ore08DbmBaW3WrqLZMv57QMk0ACEPk= + + + 8v8fec9UyJ5g/GcZmkrG3gQT/eI= + + + Rk9PCg== + + + MIICXgIBAAKBgQDJg2cms7MqjniT8Fi/XkNHZNPbNVQyMUMXE9tXOdqwYCA1cc8vQdzkihscQMXy3iPw2cMggBu6gjMTOSOxECkuvX5ZCclKr8pXAJM5cY6gVOaVO2PdTZcvDBKGbiaNefiEw5hnoZomqZGp8wHNLAUkwtH9vjqqvxyS/vclc6k2ewIDAQABAoGBAKD2emW6ssmyhfQ9ztYFuJ4FlwiJf5icKuf7L4BsMRgjoHawUvt/k69l9aPKxZNrB7BycV+7lOqU57FaOf1MWGeWzsU5bYUVpFzOVwsY4umtsO78QGKLZe+91Z+ktOlmL3scAymAgE88Jmr0g8FC46Vv4Sam7zMCtmOvA9fYog1ZAkEA8lAe+XihSuZI6IZcdRdB6QJ5cgAJoZdWKKtUovb5Ah2w4D/ebkfpsQJK44aSR5GbnrnqSaMeLJMRz++Td0edHwJBANTlUBzoo3ihcBOZ0VzGYgDIG8foCTEf3jDBYNYaY9RH/c4P50GkDa4PBqtf1f+VORwAsC2NTeY6HUEWMpvfXyUCQQChQ3FZ1k6B6oDbP5CI3NGgoWTx2dSPFojgyCWrz3IpVllA5UDDZFjC1SPCCO2Rc/Z9zH2ARG7we3B/UpJx79dBAkEAiPc6sk6NFQevpjyYcDqFRIF5NgQ3Ha6l8PIITdZOkXz7cX3Txuw3jNrH7KtMbxDe3AApWDUHf+21cnFIf/WWLQJAeG0KKBfZw1iRu9vlcYakGWRUSga78QDy08uHDtxQLXxOfSvm/y8N1KrEsXf/cJzHUGQJrqk8nLzR5mTRqnAZWA== + + + + + attack-name-id + + + + + + + https://example.org/sp.xml + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified + + + + diff --git a/tests/xmlsec1-keydata/signed-response-with-hmac.xml b/tests/xmlsec1-keydata/signed-response-with-hmac.xml new file mode 100644 index 000000000..51068a044 --- /dev/null +++ b/tests/xmlsec1-keydata/signed-response-with-hmac.xml @@ -0,0 +1,49 @@ + + + urn:mace:example.com:saml:roland:idp + + + + + + + + + + + /tLLJtWfBNGVGkPWs09wGxvKL/rPVWt5maNs9DWbHfQ= + + + iInSCge8AdweKTwZ9Z8P6e8Kb24= + + + Rk9PCg== + + + MIICXgIBAAKBgQDJg2cms7MqjniT8Fi/XkNHZNPbNVQyMUMXE9tXOdqwYCA1cc8vQdzkihscQMXy3iPw2cMggBu6gjMTOSOxECkuvX5ZCclKr8pXAJM5cY6gVOaVO2PdTZcvDBKGbiaNefiEw5hnoZomqZGp8wHNLAUkwtH9vjqqvxyS/vclc6k2ewIDAQABAoGBAKD2emW6ssmyhfQ9ztYFuJ4FlwiJf5icKuf7L4BsMRgjoHawUvt/k69l9aPKxZNrB7BycV+7lOqU57FaOf1MWGeWzsU5bYUVpFzOVwsY4umtsO78QGKLZe+91Z+ktOlmL3scAymAgE88Jmr0g8FC46Vv4Sam7zMCtmOvA9fYog1ZAkEA8lAe+XihSuZI6IZcdRdB6QJ5cgAJoZdWKKtUovb5Ah2w4D/ebkfpsQJK44aSR5GbnrnqSaMeLJMRz++Td0edHwJBANTlUBzoo3ihcBOZ0VzGYgDIG8foCTEf3jDBYNYaY9RH/c4P50GkDa4PBqtf1f+VORwAsC2NTeY6HUEWMpvfXyUCQQChQ3FZ1k6B6oDbP5CI3NGgoWTx2dSPFojgyCWrz3IpVllA5UDDZFjC1SPCCO2Rc/Z9zH2ARG7we3B/UpJx79dBAkEAiPc6sk6NFQevpjyYcDqFRIF5NgQ3Ha6l8PIITdZOkXz7cX3Txuw3jNrH7KtMbxDe3AApWDUHf+21cnFIf/WWLQJAeG0KKBfZw1iRu9vlcYakGWRUSga78QDy08uHDtxQLXxOfSvm/y8N1KrEsXf/cJzHUGQJrqk8nLzR5mTRqnAZWA== + + + + + + + + urn:mace:example.com:saml:roland:idp + + attack-name-id + + + + + + + https://example.org/sp.xml + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified + + + +