Skip to content

[PR] Improve PGPData keypacket checks #626

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
seanjmullan wants to merge 2 commits into
base: main
Choose a base branch
from
+82 −2
Open

[PR] Improve PGPData keypacket checks #626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
732d269
Check for invalid key packets in PGPData when unmarshalling.
seanjmullan Jul 2, 2026
c839402
Remove commented out code.
seanjmullan Jul 2, 2026
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
5 changes: 3 additions & 2 deletions src/main/java/org/apache/jcp/xml/dsig/internal/dom/DOMPGPData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ public DOMPGPData(Element pdElem) throws MarshalException {
} else if ("PGPKeyPacket".equals(localName) && XMLSignature.XMLNS.equals(namespace)) {
String content = XMLUtils.getFullTextChildrenFromNode(childElem);
pgpKeyPacket = XMLUtils.decode(content);
checkKeyPacket(pgpKeyPacket);
} else {
other.add
(new javax.xml.crypto.dom.DOMStructure(childElem));
Expand Down Expand Up @@ -250,8 +251,8 @@ private void checkKeyPacket(byte[] keyPacket) {
}

// tag value must be 6, 14, 5 or 7
if ((tag & 6) != 6 && (tag & 14) != 14 &&
(tag & 5) != 5 && (tag & 7) != 7) {
tag = tag & 0x3f;
if (tag != 5 && tag != 6 && tag != 7 && tag != 14) {
throw new IllegalArgumentException("keypacket tag is invalid: " +
"must be 6, 14, 5, or 7");
}
Expand Down
79 changes: 79 additions & 0 deletions src/test/java/org/apache/xml/security/test/javax/xml/crypto/dsig/keyinfo/PGPDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,28 @@
import java.util.ArrayList;
import java.util.List;

import javax.xml.crypto.MarshalException;
import javax.xml.crypto.XMLStructure;
import javax.xml.crypto.dsig.XMLSignature;
import javax.xml.crypto.dom.DOMStructure;
import javax.xml.crypto.dsig.keyinfo.KeyInfo;
import javax.xml.crypto.dsig.keyinfo.KeyInfoFactory;
import javax.xml.crypto.dsig.keyinfo.PGPData;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;

import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Text;

import org.apache.xml.security.utils.XMLUtils;

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;


Expand All @@ -44,6 +57,7 @@
class PGPDataTest {

private final KeyInfoFactory fac;
private final DocumentBuilder db;
private final byte[][] values = {
{
0x01, 0x02, 0x03, 0x04,
Expand All @@ -54,9 +68,35 @@ class PGPDataTest {
}
};

private final byte[][] invalidKeyPackets = {
{
// key packet too short
0x01, 0x02
},
{
// invalid key packet header (bit 7 not set to 1)
0x66, 0x01, 0x00
},
{
// invalid old key packet format (bit 6 not set to 1)
(byte)0x86, 0x01, 0x00
},
{
// invalid key packet tag (must be 6, 14, 5, or 7)
(byte)0xc3, 0x01, 0x00
},
{
// invalid key packet tag (must be 6, 14, 5, or 7)
(byte)0xd6, 0x01, 0x00
}
};

public PGPDataTest() throws Exception {
fac = KeyInfoFactory.getInstance
("DOM", new org.apache.jcp.xml.dsig.internal.dom.XMLDSigRI());
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
dbf.setNamespaceAware(true);
db = dbf.newDocumentBuilder();
}

@SuppressWarnings("rawtypes")
Expand Down Expand Up @@ -123,6 +163,45 @@ void testConstructor() {
// test newPGPData(byte[], List)
pd = fac.newPGPData(values[1], null);
assertArrayEquals(values[1], pd.getKeyPacket());

// test that newPGPData throws IllegalArgumentExc on invalid key packets
for (byte[] keyPacket : invalidKeyPackets) {
testInvalidKeyPacket(keyPacket);
}
}

private void testInvalidKeyPacket(byte[] keyPacket) {
assertThrows(IllegalArgumentException.class, () ->
fac.newPGPData(keyPacket, null));
}

@Test
void testUnmarshal() throws Exception {
Document doc = db.newDocument();
Element kiElem = doc.createElementNS(XMLSignature.XMLNS, "KeyInfo");
Element pdElem = doc.createElementNS(XMLSignature.XMLNS, "PGPData");
Element pkpElem = doc.createElementNS(XMLSignature.XMLNS, "PGPKeyPacket");
Element pkiElem = doc.createElementNS(XMLSignature.XMLNS, "PGPKeyID");
kiElem.appendChild(pdElem);

// test unmarshalling key id and packets
Text pkiText = doc.createTextNode(XMLUtils.encodeToString(values[0]));
pkiElem.appendChild(pkiText);
Text pkpText = doc.createTextNode(XMLUtils.encodeToString(values[1]));
pkpElem.appendChild(pkpText);
pdElem.appendChild(pkiElem);
pdElem.appendChild(pkpElem);
KeyInfo ki = fac.unmarshalKeyInfo(new DOMStructure(kiElem));
PGPData pd = (PGPData)ki.getContent().get(0);
assertArrayEquals(values[0], pd.getKeyId());
assertArrayEquals(values[1], pd.getKeyPacket());

// test that unmarshalKeyInfo throws MarshalExc on invalid key packets
for (byte[] keyPacket : invalidKeyPackets) {
pkpText.setNodeValue(XMLUtils.encodeToString(keyPacket));
assertThrows(MarshalException.class, () ->
fac.unmarshalKeyInfo(new DOMStructure(kiElem)));
}
}

@Test
Expand Down