Add note about comment canonicalization

Author: kislyuk

README.rst

@@ -86,8 +86,9 @@ Assuming ``metadata.xml`` contains SAML metadata for the assertion source:
  It is important to understand and follow the best practice rule of "See what is signed" when verifying XML
  signatures. The gist of this rule is: if your application neglects to verify that the information it trusts is
  what was actually signed, the attacker can supply a valid signature but point you to malicious data that wasn't signed
- by that signature. Failure to follow this rule can lead to vulnerability against attacks like
- `SAML signature wrapping <https://www.usenix.org/system/files/conference/usenixsecurity12/sec12-final91.pdf>`_.
+ by that signature. Failure to follow this rule can lead to vulnerabilities against attacks like
+ `SAML signature wrapping <https://www.usenix.org/system/files/conference/usenixsecurity12/sec12-final91.pdf>`_ and
+ `XML comment canonicalization induced text element truncation <https://duo.com/blog/duo-finds-saml-vulnerabilities-affecting-multiple-implementations>`_.
 
  In SignXML, you can ensure that the information signed is what you expect to be signed by only trusting the
  data returned by ``XMLVerifier.verify()``. The ``signed_xml`` attribute of the return value is the XML node or string

signxml/verifier.py

@@ -389,7 +389,7 @@ def verify(
         root = self.get_root(data)
         signature_ref = self._get_signature(root)
 
-        # HACK: deep copy won't keep root's namespaces
+        # We could do a deep copy here, but it wouldn't preserve root level namespaces
         signature = self._fromstring(self._tostring(signature_ref))
 
         if validate_schema:
@@ -514,7 +514,9 @@ def _verify_reference(self, reference, index, root, uri_resolver, c14n_algorithm
         if b64decode(digest_value.text) != self._get_digest(payload_c14n, digest_alg):
             raise InvalidDigest(f"Digest mismatch for reference {index} ({reference.get('URI')})")
 
-        # We return the signed XML (and only that) to ensure no access to unsigned data happens
+        # We return the signed XML (and only that) to ensure no access to unsigned data happens.
+        # Note it is essential to roundtrip the payload and render it from canonicalized XML, to avoid returning
+        # untrusted comments, avoid text nodes being broken up even after comments are excised, etc.
         try:
             payload_c14n_xml = self._fromstring(payload_c14n)
         except etree.XMLSyntaxError: