From a2f0035f8963f942983383a007eed34a23600b0b Mon Sep 17 00:00:00 2001 From: gbrodman Date: Mon, 20 Jul 2026 12:52:11 -0400 Subject: [PATCH] Mitigate XSW in SMD verification (#3148) There's a bit of a mismatch between the bit that parses the Java object and the bit that validates the XML. The parser parses the Java root node, however the validator follows the reference in a (valid) signature to *any* node, which can be hidden elsewhere. To fix this robustly: - Enforce that the XML signature Reference URI matches the root element ID precisely - Assert that exactly one element exists in the DOM - Don't allow additional signed marks elsewhere in the XML just in case --- .../registry/tmch/TmchXmlSignature.java | 34 +++++ .../registry/tmch/TmchXmlSignatureTest.java | 136 ++++++++++++++++++ 2 files changed, 170 insertions(+) diff --git a/core/src/main/java/google/registry/tmch/TmchXmlSignature.java b/core/src/main/java/google/registry/tmch/TmchXmlSignature.java index 56a27db59..54ddd5f8e 100644 --- a/core/src/main/java/google/registry/tmch/TmchXmlSignature.java +++ b/core/src/main/java/google/registry/tmch/TmchXmlSignature.java @@ -51,6 +51,7 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.validation.Schema; import org.w3c.dom.Document; +import org.w3c.dom.Element; import org.w3c.dom.NodeList; import org.xml.sax.SAXException; @@ -84,6 +85,27 @@ public class TmchXmlSignature { checkArgument(smdXml.length > 0); Document doc = parseSmdDocument(new ByteArrayInputStream(smdXml)); + // Verify root element is in correct namespace + Element rootElement = doc.getDocumentElement(); + if (!"signedMark".equals(rootElement.getLocalName()) + || !"urn:ietf:params:xml:ns:signedMark-1.0".equals(rootElement.getNamespaceURI())) { + throw new XMLSignatureException( + "Root element must be signedMark in namespace urn:ietf:params:xml:ns:signedMark-1.0"); + } + + // Assert exactly one element exists in the DOM to prevent wrapping/nesting + NodeList signedMarkNodes = + doc.getElementsByTagNameNS("urn:ietf:params:xml:ns:signedMark-1.0", "signedMark"); + if (signedMarkNodes.getLength() != 1) { + throw new XMLSignatureException( + "Expected exactly one element in the document."); + } + + String rootId = rootElement.getAttribute("id"); + if (rootId.isEmpty()) { + throw new XMLSignatureException("Root signedMark element must have an id attribute."); + } + NodeList signatureNodes = doc.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); if (signatureNodes.getLength() != 1) { throw new XMLSignatureException("Expected exactly one element."); @@ -91,8 +113,20 @@ public class TmchXmlSignature { XMLSignatureFactory factory = XMLSignatureFactory.getInstance("DOM"); KeyValueKeySelector selector = new KeyValueKeySelector(tmchCertificateAuthority); DOMValidateContext context = new DOMValidateContext(selector, signatureNodes.item(0)); + + // Explicitly register the root ID attribute in the validation context just in case + context.setIdAttributeNS(rootElement, null, "id"); + XMLSignature signature = factory.unmarshalXMLSignature(context); + // Verify that the signature Reference URI matches the root element ID ("#" + rootId) + String expectedUri = String.format("#%s", rootId); + List references = signature.getSignedInfo().getReferences(); + if (references.stream().noneMatch(ref -> expectedUri.equals(ref.getURI()))) { + throw new XMLSignatureException( + "Signature Reference URI does not match the root element ID."); + } + boolean isValid; try { isValid = signature.validate(context); diff --git a/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java b/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java index 417ad6ecd..66d5bf42d 100644 --- a/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java +++ b/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java @@ -16,6 +16,7 @@ package google.registry.tmch; import static com.google.common.truth.Truth.assertThat; import static google.registry.tmch.TmchTestData.loadSmd; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertThrows; import google.registry.config.RegistryConfig.ConfigModule.TmchCaMode; @@ -139,4 +140,139 @@ class TmchXmlSignatureTest { assertThrows(CertificateRevokedException.class, () -> tmchXmlSignature.verify(smdData)); assertThat(e).hasMessageThat().contains("Certificate has been revoked"); } + + // These tests check the structure of the decoded XML (unrelated to the decoding itself) + @Test + void testVerify_rootElementNotSignedMark_fails() { + String xml = + """ + + + + + """; + XMLSignatureException e = + assertThrows( + XMLSignatureException.class, () -> tmchXmlSignature.verify(xml.getBytes(UTF_8))); + assertThat(e) + .hasMessageThat() + .contains( + "Root element must be signedMark in namespace urn:ietf:params:xml:ns:signedMark-1.0"); + } + + @Test + void testVerify_xswWrapping_fails() { + // By default, the verifier follows the reference from a valid signature wherever it goes. This + // could be a second signedMark object hidden elsewhere in the XML (say, inside a ds:Object, + // which can contain anything). The SignedMark parser, however, uses the root node. We need to + // make sure that the valid signature points to the root node, and not anything else. + String xswXml = + """ + + + fake-id + + + + + + + + + + dGVzdA== + + + dGVzdA== + + + real-id + + + + + """; + + XMLSignatureException e = + assertThrows( + XMLSignatureException.class, () -> tmchXmlSignature.verify(xswXml.getBytes(UTF_8))); + assertThat(e) + .hasMessageThat() + .contains("Expected exactly one element in the document"); + } + + @Test + void testVerify_signatureDoesNotSignRoot_fails() { + // The internal signature reference URI must match the root signed mark ID + String xml = + """ + + + modified-id + + + + + + + + + + dGVzdA== + + + dGVzdA== + + + """; + + XMLSignatureException e = + assertThrows( + XMLSignatureException.class, () -> tmchXmlSignature.verify(xml.getBytes(UTF_8))); + assertThat(e) + .hasMessageThat() + .contains("Signature Reference URI does not match the root element ID"); + } + + @Test + void testVerify_multipleSignedMarks_fails() { + // Even if the signature does validate the root signed mark, it's sketchy at best to include + // another signed mark hidden in the XML. Don't allow it. + String xml = + """ + + + id-1 + + + + + + + + + + dGVzdA== + + + dGVzdA== + + + id-2 + + + + + """; + + XMLSignatureException e = + assertThrows( + XMLSignatureException.class, () -> tmchXmlSignature.verify(xml.getBytes(UTF_8))); + assertThat(e) + .hasMessageThat() + .contains("Expected exactly one element in the document"); + } }