Handle ambiguous KeyValue situations

- XMLSigner.sign(): add always_add_key_value kwarg to include both
  X509Data and KeyValue for ill-defined signing applications

- XMLVerifier.verify(): reject signatures that contain both X509Data
  and KeyValue by default; add ignore_ambiguous_key_info kwarg to
  bypass

- Add security warnings in docs

Fixes #52
Fixes #65
Fixes #64

Author: kislyuk

signxml/__init__.py

@@ -320,7 +320,12 @@ def sign(self, data, key=None, passphrase=None, cert=None, reference_uri=None, k
         :param id_attribute:
             Name of the attribute whose value ``URI`` refers to. By default, SignXML will search for "Id", then "ID".
         :type id_attribute: string
-        :param always_add_key_value: Write the key value to the KeyInfo element even if an X509 certificate is present.
+        :param always_add_key_value:
+            Write the key value to the KeyInfo element even if a X509 certificate is present. Use of this parameter
+            is discouraged, as it introduces an ambiguity and a security hazard. The public key used to sign the
+            document is already encoded in the certificate (which is in X509Data), so the verifier must either ignore
+            KeyValue or make sure it matches what's in the certificate. This parameter is provided for compatibility
+            purposes only.
         :type always_add_key_value: boolean
 
         :returns:
@@ -610,7 +615,7 @@ def _apply_transforms(self, payload, transforms_node, signature, c14n_algorithm)
 
     def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None, ca_pem_file=None, ca_path=None,
                hmac_key=None, validate_schema=True, parser=None, uri_resolver=None, id_attribute=None,
-               expect_references=1):
+               expect_references=1, ignore_ambiguous_key_info=False):
         """
         Verify the XML signature supplied in the data and return the XML node signed by the signature, or raise an
         exception if the signature is not valid. By default, this requires the signature to be generated using a valid
@@ -681,6 +686,14 @@ def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None
             Number of references to expect in the signature. If this is not 1, an array of VerifyResults is returned.
             If set to a non-integer, any number of references is accepted (otherwise a mismatch raises an error).
         :type expect_references: int or boolean
+        :param ignore_ambiguous_key_info:
+            Ignore the presence of a KeyValue element when X509Data is present in the signature and used for verifying.
+            The presence of both elements is an ambiguity and a security hazard. The public key used to sign the
+            document is already encoded in the certificate (which is in X509Data), so the verifier must either ignore
+            KeyValue or make sure it matches what's in the certificate. SignXML does not implement the functionality
+            necessary to match the keys, and throws an InvalidInput error instead. Set this to True to bypass the error
+            and validate the signature using X509Data only.
+        :type ignore_ambiguous_key_info: boolean
 
         :raises: :py:class:`cryptography.exceptions.InvalidSignature`
 
@@ -716,6 +729,7 @@ def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None
         signature_alg = signature_method.get("Algorithm")
         raw_signature = b64decode(signature_value.text)
         x509_data = signature.find("ds:KeyInfo/ds:X509Data", namespaces=namespaces)
+        key_value = signature.find("ds:KeyInfo/ds:KeyValue", namespaces=namespaces)
         signed_info_c14n = self._c14n(signed_info, algorithm=c14n_algorithm)
 
         # TODO: if both X509Data and KeyValue is present, match one against the other and raise an error on mismatch
@@ -749,6 +763,12 @@ def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None
                 except Exception:
                     reason = e
                 raise InvalidSignature("Signature verification failed: {}".format(reason))
+
+            if ignore_ambiguous_key_info is False:
+                if key_value is not None:
+                    raise InvalidInput("Both X509Data and KeyValue found. Use verify(ignore_ambiguous_key_info=True) "
+                                       "to ignore KeyValue and validate using X509Data only.")
+
             # TODO: CN verification goes here
             # TODO: require one of the following to be set: either x509_cert or (ca_pem_file or ca_path) or common_name
             # Use ssl.match_hostname or code from it to perform match
@@ -764,7 +784,6 @@ def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None
             if raw_signature != signer.finalize():
                 raise InvalidSignature("Signature mismatch (HMAC)")
         else:
-            key_value = signature.find("ds:KeyInfo/ds:KeyValue", namespaces=namespaces)
             if key_value is None:
                 raise InvalidInput("Expected to find either KeyValue or X509Data XML element in KeyInfo")
 

test/test.py

@@ -375,6 +375,13 @@ def test_reference_uris_and_custom_key_info(self):
             self.assertTrue(s3.xpath("/ds:Signature/ds:KeyInfo/wsse:SecurityTokenReference",
                                      namespaces=dict(namespaces, wsse=wsse_ns)))
 
+            # Test setting both X509Data and KeyInfo
+            s4 = XMLSigner().sign(data, reference_uri=reference_uri, key=key, cert=crt, always_add_key_value=True)
+            with self.assertRaisesRegexp(InvalidInput, "Both X509Data and KeyValue found"):
+                XMLVerifier().verify(s4, x509_cert=crt)
+            expect_refs = etree.tostring(s4).decode().count("<ds:Reference")
+            XMLVerifier().verify(s4, x509_cert=crt, ignore_ambiguous_key_info=True, expect_references=expect_refs)
+
     def test_excision_of_untrusted_comments(self):
         pass  # TODO: test comments excision