Skip to content

Mitigate XSW in SMD verification #3148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gbrodman wants to merge 1 commit into
base: master
Choose a base branch
from
+170 −0
Open

Mitigate XSW in SMD verification #3148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions core/src/main/java/google/registry/tmch/TmchXmlSignature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -84,15 +85,48 @@ public void verify(byte[] smdXml)
checkArgument(smdXml.length > 0);
Document doc = parseSmdDocument(new ByteArrayInputStream(smdXml));

// Verify root element is <smd:signedMark> 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 <smd:signedMark> 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 <smd:signedMark> 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 <ds:Signature> element.");
}
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<Reference> 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);
Expand Down
136 changes: 136 additions & 0 deletions core/src/test/java/google/registry/tmch/TmchXmlSignatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

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;
Expand Down Expand Up @@ -139,4 +140,139 @@ void testIndTmvRevoked(String language) {
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 =
"""
<?xml version="1.0" encoding="UTF-8"?>
<container>
<smd:signedMark xmlns:smd="urn:ietf:params:xml:ns:signedMark-1.0" id="id-1"/>
</container>
""";
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 =
"""
<?xml version="1.0" encoding="UTF-8"?>
<smd:signedMark xmlns:smd="urn:ietf:params:xml:ns:signedMark-1.0" id="fake-id">
<smd:id>fake-id</smd:id>
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
<ds:SignedInfo>
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>
<ds:Reference URI="#real-id">
<ds:Transforms>
<ds:Transform Algorithm=\
"http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
</ds:Transforms>
<ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/>
<ds:DigestValue>dGVzdA==</ds:DigestValue>
</ds:Reference>
</ds:SignedInfo>
<ds:SignatureValue>dGVzdA==</ds:SignatureValue>
<ds:Object>
<smd:signedMark id="real-id">
<smd:id>real-id</smd:id>
</smd:signedMark>
</ds:Object>
</ds:Signature>
</smd:signedMark>
""";

XMLSignatureException e =
assertThrows(
XMLSignatureException.class, () -> tmchXmlSignature.verify(xswXml.getBytes(UTF_8)));
assertThat(e)
.hasMessageThat()
.contains("Expected exactly one <smd:signedMark> element in the document");
}

@Test
void testVerify_signatureDoesNotSignRoot_fails() {
// The internal signature reference URI must match the root signed mark ID
String xml =
"""
<?xml version="1.0" encoding="UTF-8"?>
<smd:signedMark xmlns:smd="urn:ietf:params:xml:ns:signedMark-1.0" id="modified-id">
<smd:id>modified-id</smd:id>
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
<ds:SignedInfo>
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>
<ds:Reference URI="#original-id">
<ds:Transforms>
<ds:Transform Algorithm=\
"http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
</ds:Transforms>
<ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/>
<ds:DigestValue>dGVzdA==</ds:DigestValue>
</ds:Reference>
</ds:SignedInfo>
<ds:SignatureValue>dGVzdA==</ds:SignatureValue>
</ds:Signature>
</smd:signedMark>
""";

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 =
"""
<?xml version="1.0" encoding="UTF-8"?>
<smd:signedMark xmlns:smd="urn:ietf:params:xml:ns:signedMark-1.0" id="id-1">
<smd:id>id-1</smd:id>
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
<ds:SignedInfo>
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>
<ds:Reference URI="#id-1">
<ds:Transforms>
<ds:Transform Algorithm=\
"http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
</ds:Transforms>
<ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/>
<ds:DigestValue>dGVzdA==</ds:DigestValue>
</ds:Reference>
</ds:SignedInfo>
<ds:SignatureValue>dGVzdA==</ds:SignatureValue>
<ds:Object>
<smd:signedMark id="id-2">
<smd:id>id-2</smd:id>
</smd:signedMark>
</ds:Object>
</ds:Signature>
</smd:signedMark>
""";

XMLSignatureException e =
assertThrows(
XMLSignatureException.class, () -> tmchXmlSignature.verify(xml.getBytes(UTF_8)));
assertThat(e)
.hasMessageThat()
.contains("Expected exactly one <smd:signedMark> element in the document");
}
}
Loading