From 1e59eaa09a0b7185705004188fbbed0b53681a23 Mon Sep 17 00:00:00 2001 From: Ivan Kanakarakis Date: Sat, 19 Jun 2021 23:25:08 +0300 Subject: [PATCH] Improve signature checks - Enforce allowed canonicalization methods - Enforce allowed transform aglorithms - Ensure the Object element is absent Signed-off-by: Ivan Kanakarakis --- src/saml2/sigver.py | 55 +++++++++++++++++++++++++++++++---- src/saml2/xmldsig/__init__.py | 15 +++++++--- tests/test_00_xmldsig.py | 49 +++++++++++++++---------------- 3 files changed, 85 insertions(+), 34 deletions(-) diff --git a/src/saml2/sigver.py b/src/saml2/sigver.py index b18d2cced..86004b847 100644 --- a/src/saml2/sigver.py +++ b/src/saml2/sigver.py @@ -42,6 +42,10 @@ from saml2.s_utils import Unsupported from saml2.time_util import instant from saml2.time_util import str_to_time +from saml2.xmldsig import ALLOWED_CANONICALIZATIONS +from saml2.xmldsig import ALLOWED_TRANSFORMS +from saml2.xmldsig import TRANSFORM_C14N +from saml2.xmldsig import TRANSFORM_ENVELOPED from saml2.xmldsig import SIG_RSA_SHA1 from saml2.xmldsig import SIG_RSA_SHA224 from saml2.xmldsig import SIG_RSA_SHA256 @@ -1503,7 +1507,8 @@ def _check_signature(self, decoded_xml, item, node_name=NODE_NAME, origdoc=None, # * the Reference element must have a URI attribute # * the URI attribute contains an anchor # * the anchor points to the enclosing element's ID attribute - references = item.signature.signed_info.reference + signed_info = item.signature.signed_info + references = signed_info.reference signatures_must_have_a_single_reference_element = len(references) == 1 the_Reference_element_must_have_a_URI_attribute = ( signatures_must_have_a_single_reference_element @@ -1518,6 +1523,41 @@ def _check_signature(self, decoded_xml, item, node_name=NODE_NAME, origdoc=None, the_URI_attribute_contains_an_anchor and references[0].uri == "#{id}".format(id=item.id) ) + + # SAML implementations SHOULD use Exclusive Canonicalization, + # with or without comments + canonicalization_method_is_c14n = ( + signed_info.canonicalization_method.algorithm in ALLOWED_CANONICALIZATIONS + ) + + # Signatures in SAML messages SHOULD NOT contain transforms other than the + # - enveloped signature transform + # (with the identifier http://www.w3.org/2000/09/xmldsig#enveloped-signature) + # - or the exclusive canonicalization transforms + # (with the identifier http://www.w3.org/2001/10/xml-exc-c14n# + # or http://www.w3.org/2001/10/xml-exc-c14n#WithComments). + transform_alogs = [ + transform.algorithm + for transform in references[0].transforms.transform + ] + transform_alogs_n = len(transform_alogs) + only_up_to_two_transforms_are_defined = ( + signatures_must_have_a_single_reference_element + and 1 <= transform_alogs_n <= 2 + ) + all_transform_algs_are_allowed = ( + only_up_to_two_transforms_are_defined + and transform_alogs_n == len( + ALLOWED_TRANSFORMS.intersection(transform_alogs) + ) + ) + + # The element is not defined for use with SAML signatures, + # and SHOULD NOT be present. + # Since it can be used in service of an attacker by carrying unsigned data, + # verifiers SHOULD reject signatures that contain a element. + object_element_is_not_present = not item.signature.object + validators = { "signatures must have a single reference element": ( signatures_must_have_a_single_reference_element @@ -1531,6 +1571,12 @@ def _check_signature(self, decoded_xml, item, node_name=NODE_NAME, origdoc=None, "the anchor points to the enclosing element ID attribute": ( the_anchor_points_to_the_enclosing_element_ID_attribute ), + "canonicalization method is c14n": canonicalization_method_is_c14n, + "only up to two transforms are defined": ( + only_up_to_two_transforms_are_defined + ), + "all transform algs are allowed": all_transform_algs_are_allowed, + "object element is not present": object_element_is_not_present, } if not all(validators.values()): error_context = { @@ -1818,10 +1864,9 @@ def pre_signature_part( sign_alg = ds.DefaultSignature().get_sign_alg() signature_method = ds.SignatureMethod(algorithm=sign_alg) - canonicalization_method = ds.CanonicalizationMethod( - algorithm=ds.ALG_EXC_C14N) - trans0 = ds.Transform(algorithm=ds.TRANSFORM_ENVELOPED) - trans1 = ds.Transform(algorithm=ds.ALG_EXC_C14N) + canonicalization_method = ds.CanonicalizationMethod(algorithm=TRANSFORM_C14N) + trans0 = ds.Transform(algorithm=TRANSFORM_ENVELOPED) + trans1 = ds.Transform(algorithm=TRANSFORM_C14N) transforms = ds.Transforms(transform=[trans0, trans1]) digest_method = ds.DigestMethod(algorithm=digest_alg) diff --git a/src/saml2/xmldsig/__init__.py b/src/saml2/xmldsig/__init__.py index 02bac3f41..4177601fc 100644 --- a/src/saml2/xmldsig/__init__.py +++ b/src/saml2/xmldsig/__init__.py @@ -53,14 +53,21 @@ MAC_SHA1 = 'http://www.w3.org/2000/09/xmldsig#hmac-sha1' -C14N = 'http://www.w3.org/TR/2001/REC-xml-c14n-20010315' -C14N_WITH_C = 'http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments' -ALG_EXC_C14N = 'http://www.w3.org/2001/10/xml-exc-c14n#' - TRANSFORM_XSLT = 'http://www.w3.org/TR/1999/REC-xslt-19991116' TRANSFORM_XPATH = 'http://www.w3.org/TR/1999/REC-xpath-19991116' TRANSFORM_ENVELOPED = 'http://www.w3.org/2000/09/xmldsig#enveloped-signature' +TRANSFORM_C14N = 'http://www.w3.org/2001/10/xml-exc-c14n#' +TRANSFORM_C14N_WITH_COMMENTS = 'http://www.w3.org/2001/10/xml-exc-c14n#WithComments' +ALLOWED_CANONICALIZATIONS = { + TRANSFORM_C14N, + TRANSFORM_C14N_WITH_COMMENTS, +} +ALLOWED_TRANSFORMS = { + TRANSFORM_ENVELOPED, + TRANSFORM_C14N, + TRANSFORM_C14N_WITH_COMMENTS, +} class DefaultSignature(object): class _DefaultSignature(object): diff --git a/tests/test_00_xmldsig.py b/tests/test_00_xmldsig.py index 5b8001940..80453f9a1 100644 --- a/tests/test_00_xmldsig.py +++ b/tests/test_00_xmldsig.py @@ -33,12 +33,10 @@ def testUsingTestData(self): new_object = ds.object_from_string(ds_data.TEST_OBJECT) assert new_object.id == "object_id" assert new_object.encoding == ds.ENCODING_BASE64 - assert new_object.text.strip() == \ - "V2VkIEp1biAgNCAxMjoxMTowMyBFRFQgMjAwMwo" - + assert new_object.text.strip() == "V2VkIEp1biAgNCAxMjoxMTowMyBFRFQgMjAwMwo" -class TestMgmtData: +class TestMgmtData: def setup_class(self): self.mgmt_data = ds.MgmtData() @@ -156,7 +154,7 @@ def testAccessors(self): self.x509_data.x509_certificate = ds.X509Certificate( text="x509 certificate") self.x509_data.x509_crl = ds.X509CRL(text="x509 crl") - + new_x509_data = ds.x509_data_from_string(self.x509_data.to_string()) print(new_x509_data.keyswv()) print(new_x509_data.__dict__.keys()) @@ -231,7 +229,7 @@ def testAccessors(self): ds.TRANSFORM_ENVELOPED assert new_transforms.transform[0].x_path[0].text.strip() == "xpath" assert new_transforms.transform[1].x_path[0].text.strip() == "xpath" - + def testUsingTestData(self): """Test for transform_from_string() using test data""" new_transforms = ds.transforms_from_string(ds_data.TEST_TRANSFORMS) @@ -261,7 +259,7 @@ def testAccessors(self): assert new_retrieval_method.uri == "http://www.example.com/URI" assert new_retrieval_method.type == "http://www.example.com/Type" assert isinstance(new_retrieval_method.transforms, ds.Transforms) - + def testUsingTestData(self): """Test for retrieval_method_from_string() using test data""" new_retrieval_method = ds.retrieval_method_from_string( @@ -285,7 +283,7 @@ def testAccessors(self): assert isinstance(new_rsa_key_value.exponent, ds.Exponent) assert new_rsa_key_value.modulus.text.strip() == "modulus" assert new_rsa_key_value.exponent.text.strip() == "exponent" - + def testUsingTestData(self): """Test for rsa_key_value_from_string() using test data""" new_rsa_key_value = ds.rsa_key_value_from_string( @@ -325,7 +323,7 @@ def testAccessors(self): assert new_dsa_key_value.j.text.strip() == "j" assert new_dsa_key_value.seed.text.strip() == "seed" assert new_dsa_key_value.pgen_counter.text.strip() == "pgen counter" - + def testUsingTestData(self): """Test for dsa_key_value_from_string() using test data""" new_dsa_key_value = ds.dsa_key_value_from_string( @@ -362,7 +360,7 @@ def testAccessors(self): ds_data.TEST_RSA_KEY_VALUE) new_key_value = ds.key_value_from_string(self.key_value.to_string()) assert isinstance(new_key_value.rsa_key_value, ds.RSAKeyValue) - + def testUsingTestData(self): """Test for key_value_from_string() using test data""" new_key_value = ds.key_value_from_string(ds_data.TEST_KEY_VALUE1) @@ -384,7 +382,7 @@ def testAccessors(self): self.key_name.text = "key name" new_key_name = ds.key_name_from_string(self.key_name.to_string()) assert new_key_name.text.strip() == "key name" - + def testUsingTestData(self): """Test for key_name_from_string() using test data""" new_key_name = ds.key_name_from_string(ds_data.TEST_KEY_NAME) @@ -423,7 +421,7 @@ def testAccessors(self): assert isinstance(new_key_info.spki_data[0], ds.SPKIData) assert isinstance(new_key_info.mgmt_data[0], ds.MgmtData) assert new_key_info.id == "id" - + def testUsingTestData(self): """Test for key_info_from_string() using test data""" new_key_info = ds.key_info_from_string(ds_data.TEST_KEY_INFO) @@ -436,7 +434,7 @@ def testUsingTestData(self): assert isinstance(new_key_info.spki_data[0], ds.SPKIData) assert isinstance(new_key_info.mgmt_data[0], ds.MgmtData) assert new_key_info.id == "id" - + class TestDigestValue: @@ -448,7 +446,7 @@ def testAccessors(self): self.digest_value.text = "digest value" new_digest_value = ds.digest_value_from_string(self.digest_value.to_string()) assert new_digest_value.text.strip() == "digest value" - + def testUsingTestData(self): """Test for digest_value_from_string() using test data""" new_digest_value = ds.digest_value_from_string(ds_data.TEST_DIGEST_VALUE) @@ -466,7 +464,7 @@ def testAccessors(self): new_digest_method = ds.digest_method_from_string( self.digest_method.to_string()) assert new_digest_method.algorithm == ds.DIGEST_SHA1 - + def testUsingTestData(self): """Test for digest_method_from_string() using test data""" new_digest_method = ds.digest_method_from_string( @@ -497,7 +495,7 @@ def testAccessors(self): assert new_reference.id == "id" assert new_reference.uri == "http://www.example.com/URI" assert new_reference.type == "http://www.example.com/Type" - + def testUsingTestData(self): """Test for reference_from_string() using test data""" new_reference = ds.reference_from_string(ds_data.TEST_REFERENCE) @@ -524,7 +522,7 @@ def testAccessors(self): ds.HMACOutputLength) assert new_signature_method.hmac_output_length.text.strip() == "8" assert new_signature_method.algorithm == ds.SIG_RSA_SHA1 - + def testUsingTestData(self): """Test for signature_method_from_string() using test data""" new_signature_method = ds.signature_method_from_string( @@ -542,16 +540,17 @@ def setup_class(self): def testAccessors(self): """Test for CanonicalizationMethod accessors""" - self.canonicalization_method.algorithm = ds.C14N_WITH_C + self.canonicalization_method.algorithm = ds.TRANSFORM_C14N_WITH_COMMENTS new_canonicalization_method = ds.canonicalization_method_from_string( self.canonicalization_method.to_string()) - assert new_canonicalization_method.algorithm == ds.C14N_WITH_C - + assert new_canonicalization_method.algorithm == ds.TRANSFORM_C14N_WITH_COMMENTS + def testUsingTestData(self): """Test for canonicalization_method_from_string() using test data""" new_canonicalization_method = ds.canonicalization_method_from_string( - ds_data.TEST_CANONICALIZATION_METHOD) - assert new_canonicalization_method.algorithm == ds.C14N_WITH_C + ds_data.TEST_CANONICALIZATION_METHOD + ) + assert new_canonicalization_method.algorithm == "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments" class TestSignedInfo: @@ -574,7 +573,7 @@ def testAccessors(self): ds.CanonicalizationMethod) assert isinstance(new_si.signature_method, ds.SignatureMethod) assert isinstance(new_si.reference[0], ds.Reference) - + def testUsingTestData(self): """Test for signed_info_from_string() using test data""" new_si = ds.signed_info_from_string(ds_data.TEST_SIGNED_INFO) @@ -597,7 +596,7 @@ def testAccessors(self): self.signature_value.to_string()) assert new_signature_value.id == "id" assert new_signature_value.text.strip() == "signature value" - + def testUsingTestData(self): """Test for signature_value_from_string() using test data""" new_signature_value = ds.signature_value_from_string( @@ -627,7 +626,7 @@ def testAccessors(self): assert isinstance(new_signature.signature_value, ds.SignatureValue) assert isinstance(new_signature.key_info, ds.KeyInfo) assert isinstance(new_signature.object[0], ds.Object) - + def testUsingTestData(self): """Test for signature_value_from_string() using test data""" new_signature = ds.signature_from_string(ds_data.TEST_SIGNATURE)