From cdbf89e8f826d051d643580fe1e1099a2d1db5cf Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Fri, 26 Jun 2026 22:12:46 -0400 Subject: [PATCH] Remediate XML Signature Wrapping in SMD validation Implement multi-layered defense-in-depth to completely block XML Signature Wrapping (XSW) vulnerabilities during Sunrise phase SMD verification: - Expose the unmarshalled XML id attribute (xsdId) from the SignedMark JAXB model. - Refactor TmchXmlSignature.verify() to assert that the root element name is 'signedMark' in the correct namespace, that exactly one such element exists in the DOM, and that the signature's Reference URI matches the root element ID. Return the validated ID. - In DomainFlowTmchUtils.verifyEncodedSignedMark(), assert that the cryptographically verified ID matches the unmarshalled xsdId. - Add a robust integration test case to TmchXmlSignatureTest.java verifying that XSW payloads are correctly caught and rejected by our DOM uniqueness check. --- .../flows/domain/DomainFlowTmchUtils.java | 7 ++- .../google/registry/model/smd/SignedMark.java | 4 ++ .../registry/tmch/TmchXmlSignature.java | 45 +++++++++++++++++-- .../registry/tmch/TmchXmlSignatureTest.java | 42 +++++++++++++++++ 4 files changed, 94 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowTmchUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowTmchUtils.java index bbeae85d54c..d1c4649cbc2 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowTmchUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowTmchUtils.java @@ -101,8 +101,9 @@ public SignedMark verifyEncodedSignedMark(EncodedSignedMark encodedSignedMark, I throw new SignedMarkRevokedErrorException(); } + String signedElementId; try { - tmchXmlSignature.verify(signedMarkData); + signedElementId = tmchXmlSignature.verify(signedMarkData); } catch (CertificateExpiredException e) { throw new SignedMarkCertificateExpiredException(e); } catch (CertificateNotYetValidException e) { @@ -122,6 +123,10 @@ public SignedMark verifyEncodedSignedMark(EncodedSignedMark encodedSignedMark, I throw new SignedMarkParsingErrorException(e); } + if (!signedElementId.equals(signedMark.getXsdId())) { + throw new SignedMarkSignatureException(new Exception("Signature reference ID mismatch.")); + } + if (now.isBefore(signedMark.getCreationTime())) { throw new FoundMarkNotYetValidException(); } diff --git a/core/src/main/java/google/registry/model/smd/SignedMark.java b/core/src/main/java/google/registry/model/smd/SignedMark.java index d67a51027c2..662cb9b0f23 100644 --- a/core/src/main/java/google/registry/model/smd/SignedMark.java +++ b/core/src/main/java/google/registry/model/smd/SignedMark.java @@ -78,6 +78,10 @@ public Mark getMark() { return mark; } + public String getXsdId() { + return xsdId; + } + public boolean hasSignature() { return xmlSignature != null; } diff --git a/core/src/main/java/google/registry/tmch/TmchXmlSignature.java b/core/src/main/java/google/registry/tmch/TmchXmlSignature.java index 56a27db5918..3a51115525c 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.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; @@ -78,12 +79,33 @@ public TmchXmlSignature(TmchCertificateAuthority tmchCertificateAuthority) { * @throws GeneralSecurityException for unsupported protocols, certs not signed by the TMCH, * incorrect keys, and for invalid, old, not-yet-valid or revoked certificates. */ - public void verify(byte[] smdXml) - throws GeneralSecurityException, IOException, MarshalException, ParserConfigurationException, - SAXException, XMLSignatureException { + public String verify(byte[] smdXml) + throws GeneralSecurityException, + IOException, + MarshalException, + ParserConfigurationException, + SAXException, + XMLSignatureException { checkArgument(smdXml.length > 0); Document doc = parseSmdDocument(new ByteArrayInputStream(smdXml)); + Element rootElement = doc.getDocumentElement(); + if (!"signedMark".equals(rootElement.getLocalName()) + || !"urn:ietf:params:xml:ns:signedMark-1.0".equals(rootElement.getNamespaceURI())) { + throw new XMLSignatureException("Invalid root element name or namespace."); + } + String rootId = rootElement.getAttribute("id"); + if (rootId.isEmpty()) { + throw new XMLSignatureException("Root element missing id attribute."); + } + + // Verify that exactly one element exists in the DOM + NodeList smdNodes = + doc.getElementsByTagNameNS("urn:ietf:params:xml:ns:signedMark-1.0", "signedMark"); + if (smdNodes.getLength() != 1) { + throw new XMLSignatureException("Expected exactly one element."); + } + NodeList signatureNodes = doc.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); if (signatureNodes.getLength() != 1) { throw new XMLSignatureException("Expected exactly one element."); @@ -103,6 +125,23 @@ public void verify(byte[] smdXml) if (!isValid) { throw new XMLSignatureException(explainValidationProblem(context, signature)); } + + @SuppressWarnings("unchecked") + List references = signature.getSignedInfo().getReferences(); + if (references.isEmpty()) { + throw new XMLSignatureException("No references found in signature."); + } + Reference reference = references.get(0); + String uri = reference.getURI(); + if (uri == null || !uri.startsWith("#")) { + throw new XMLSignatureException("Invalid signature reference URI: " + uri); + } + String signedElementId = uri.substring(1); + if (!signedElementId.equals(rootId)) { + throw new XMLSignatureException( + String.format("Signature reference ID mismatch: expected #%s, got %s", rootId, uri)); + } + return signedElementId; } private static Document parseSmdDocument(InputStream input) diff --git a/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java b/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java index 417ad6ecd41..48ea8d10d5d 100644 --- a/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java +++ b/core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java @@ -23,17 +23,26 @@ import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension; import google.registry.testing.FakeClock; import google.registry.tmch.TmchXmlSignature.CertificateSignatureException; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.security.cert.CertificateExpiredException; import java.security.cert.CertificateNotYetValidException; import java.security.cert.CertificateRevokedException; import java.time.Instant; import javax.xml.crypto.dsig.XMLSignatureException; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.junitpioneer.jupiter.cartesian.CartesianTest; import org.junitpioneer.jupiter.cartesian.CartesianTest.Values; +import org.w3c.dom.Document; +import org.w3c.dom.Element; /** * Unit tests for {@link TmchXmlSignature}. @@ -139,4 +148,37 @@ void testIndTmvRevoked(String language) { assertThrows(CertificateRevokedException.class, () -> tmchXmlSignature.verify(smdData)); assertThat(e).hasMessageThat().contains("Certificate has been revoked"); } + + @Test + void testVerify_signatureReferenceIdMismatch() throws Exception { + smdData = loadSmd("smd/active.smd"); + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setNamespaceAware(true); + Document originalDoc = dbf.newDocumentBuilder().parse(new ByteArrayInputStream(smdData)); + Element originalRoot = originalDoc.getDocumentElement(); + + // Create a new document to construct the XML Signature Wrapping (XSW) payload + Document forgedDoc = dbf.newDocumentBuilder().newDocument(); + Element forgedRoot = + forgedDoc.createElementNS("urn:ietf:params:xml:ns:signedMark-1.0", "smd:signedMark"); + forgedRoot.setAttribute("id", "forged-id"); + forgedRoot.setIdAttribute("id", true); + forgedDoc.appendChild(forgedRoot); + + // Import the original valid signedMark element as a child of the forged root + Element importedChild = (Element) forgedDoc.importNode(originalRoot, true); + forgedRoot.appendChild(importedChild); + // Ensure the DOM resolver treats the original ID attribute correctly + importedChild.setIdAttribute("id", true); + + // Serialize the XSW DOM back to bytes + Transformer transformer = TransformerFactory.newInstance().newTransformer(); + ByteArrayOutputStream bos = new ByteArrayOutputStream(); + transformer.transform(new DOMSource(forgedDoc), new StreamResult(bos)); + byte[] manipulatedData = bos.toByteArray(); + + XMLSignatureException e = + assertThrows(XMLSignatureException.class, () -> tmchXmlSignature.verify(manipulatedData)); + assertThat(e).hasMessageThat().contains("Expected exactly one element"); + } }