From ea7f9a5836b939610aaaed41beb24db714e67bcb Mon Sep 17 00:00:00 2001 From: vcolin7 Date: Fri, 20 Feb 2026 19:15:18 -0500 Subject: [PATCH] Fix PKCS#7 signature verification and update self-signed certificate test --- src/node/sign.ts | 246 ++++++++++++++++++++++++++++++++---------- test/sign.e2e.test.ts | 5 +- 2 files changed, 190 insertions(+), 61 deletions(-) diff --git a/src/node/sign.ts b/src/node/sign.ts index e0ed5ce..a01004e 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -1,4 +1,5 @@ import { execFile } from "child_process"; +import { createHash, createVerify } from "crypto"; import { readFileSync, writeFileSync } from "fs"; import { mkdtemp, rm, writeFile } from "fs/promises"; import forge from "node-forge"; @@ -113,11 +114,11 @@ export async function verifyMcpbFile( return { status: "unsigned" }; } - // Parse PKCS#7 signature + // Parse PKCS#7 signature to extract certificate info const asn1 = forge.asn1.fromDer(pkcs7Signature.toString("binary")); const p7Message = forge.pkcs7.messageFromAsn1(asn1); - // Verify it's signed data and cast to correct type + // Verify it's signed data if ( !("type" in p7Message) || p7Message.type !== forge.pki.oids.signedData @@ -125,17 +126,7 @@ export async function verifyMcpbFile( return { status: "unsigned" }; } - // Now we know it's PkcsSignedData. The types are incorrect, so we'll - // fix them there - const p7 = p7Message as unknown as forge.pkcs7.PkcsSignedData & { - signerInfos: Array<{ - authenticatedAttributes: Array<{ - type: string; - value: unknown; - }>; - }>; - verify: (options?: { authenticatedAttributes?: boolean }) => boolean; - }; + const p7 = p7Message as unknown as forge.pkcs7.PkcsSignedData; // Extract certificates from PKCS#7 const certificates = p7.certificates || []; @@ -146,61 +137,24 @@ export async function verifyMcpbFile( // Get the signing certificate (first one) const signingCert = certificates[0]; - // Verify PKCS#7 signature - const contentBuf = forge.util.createBuffer(originalContent); - - try { - p7.verify({ authenticatedAttributes: true }); - - // Also verify the content matches - const signerInfos = p7.signerInfos; - const signerInfo = signerInfos?.[0]; - if (signerInfo) { - const md = forge.md.sha256.create(); - md.update(contentBuf.getBytes()); - const digest = md.digest().getBytes(); - - // Find the message digest attribute - let messageDigest = null; - for (const attr of signerInfo.authenticatedAttributes) { - if (attr.type === forge.pki.oids.messageDigest) { - messageDigest = attr.value; - break; - } - } - - if (!messageDigest || messageDigest !== digest) { - return { status: "unsigned" }; - } - } - } catch (error) { - return { status: "unsigned" }; - } - - // Convert forge certificate to PEM for OS verification - const certPem = forge.pki.certificateToPem(signingCert); - const intermediatePems = certificates - .slice(1) - .map((cert) => Buffer.from(forge.pki.certificateToPem(cert))); - - // Verify certificate chain against OS trust store - const chainValid = await verifyCertificateChain( - Buffer.from(certPem), - intermediatePems, + // Verify the PKCS#7 detached signature cryptographically + // (node-forge's p7.verify() is not implemented, so we use Node.js crypto) + const signatureValid = verifyPkcs7DetachedSignature( + originalContent, + pkcs7Signature, ); - if (!chainValid) { - // Signature is valid but certificate is not trusted + if (!signatureValid) { return { status: "unsigned" }; } - // Extract certificate info + // Check if self-signed const isSelfSigned = signingCert.issuer.getField("CN")?.value === signingCert.subject.getField("CN")?.value; - return { - status: isSelfSigned ? "self-signed" : "signed", + // Build certificate info + const certInfo = { publisher: signingCert.subject.getField("CN")?.value || "Unknown", issuer: signingCert.issuer.getField("CN")?.value || "Unknown", valid_from: signingCert.validity.notBefore.toISOString(), @@ -213,11 +167,185 @@ export async function verifyMcpbFile( .digest() .toHex(), }; + + if (isSelfSigned) { + // Self-signed certs can never pass chain verification, so report + // their status directly + return { status: "self-signed", ...certInfo }; + } + + // For CA-signed certs, verify certificate chain against OS trust store + const certPem = forge.pki.certificateToPem(signingCert); + const intermediatePems = certificates + .slice(1) + .map((cert) => Buffer.from(forge.pki.certificateToPem(cert))); + + const chainValid = await verifyCertificateChain( + Buffer.from(certPem), + intermediatePems, + ); + + if (!chainValid) { + return { status: "unsigned" }; + } + + return { status: "signed", ...certInfo }; } catch (error) { throw new Error(`Failed to verify MCPB file: ${error}`); } } +/** + * Verifies a detached PKCS#7 signature over the given content. + * + * node-forge does not implement PKCS#7 signature verification + * (see https://github.com/digitalbazaar/forge/issues/1088), so we parse the + * ASN.1 structure with node-forge and verify the RSA signature using Node.js + * built-in crypto. + * + * The verification follows RFC 5652 (CMS) Section 5.4: + * 1. Compute hash of original content, compare with messageDigest attribute + * 2. DER-encode the authenticated attributes as a SET (re-tagged from [0]) + * 3. Verify the RSA signature over the DER-encoded attributes + */ +function verifyPkcs7DetachedSignature( + content: Buffer, + pkcs7Der: Buffer, +): boolean { + try { + // Parse the raw ASN.1 to navigate the PKCS#7 structure + const contentInfoAsn1 = forge.asn1.fromDer(pkcs7Der.toString("binary")); + + // ContentInfo: SEQUENCE { contentType OID, [0] EXPLICIT content } + const contentInfoValues = contentInfoAsn1.value as forge.asn1.Asn1[]; + const signedDataAsn1 = (contentInfoValues[1].value as forge.asn1.Asn1[])[0]; + + // SignedData: SEQUENCE { + // version INTEGER, + // digestAlgorithms SET, + // encapContentInfo SEQUENCE, + // [0] IMPLICIT certificates (optional), + // [1] IMPLICIT crls (optional), + // signerInfos SET + // } + const signedDataValues = signedDataAsn1.value as forge.asn1.Asn1[]; + + // Collect certificates [0] and all SET children + let certsAsn1: forge.asn1.Asn1[] = []; + const sets: forge.asn1.Asn1[] = []; + + for (const child of signedDataValues) { + // Context-specific [0] constructed = certificates + if ( + child.tagClass === forge.asn1.Class.CONTEXT_SPECIFIC && + child.constructed && + child.type === 0 + ) { + certsAsn1 = child.value as forge.asn1.Asn1[]; + } + // Universal SET = digestAlgorithms or signerInfos + if ( + child.tagClass === forge.asn1.Class.UNIVERSAL && + child.type === forge.asn1.Type.SET && + child.constructed + ) { + sets.push(child); + } + } + + // The last SET in SignedData is signerInfos (first SET is digestAlgorithms) + if (sets.length < 2 || certsAsn1.length === 0) { + return false; + } + + const signerInfosAsn1 = sets[sets.length - 1]; + const signerInfosList = signerInfosAsn1.value as forge.asn1.Asn1[]; + if (signerInfosList.length === 0) { + return false; + } + + // First SignerInfo SEQUENCE + const signerInfoAsn1 = signerInfosList[0]; + const signerInfoChildren = signerInfoAsn1.value as forge.asn1.Asn1[]; + + // Find authenticated attributes [0] and encryptedDigest (OCTET STRING) + let authAttrsNode: forge.asn1.Asn1 | null = null; + let encryptedDigestNode: forge.asn1.Asn1 | null = null; + + for (const child of signerInfoChildren) { + // Context-specific [0] constructed = authenticatedAttributes + if ( + child.tagClass === forge.asn1.Class.CONTEXT_SPECIFIC && + child.type === 0 && + child.constructed + ) { + authAttrsNode = child; + } + // OCTET STRING (primitive) = encryptedDigest (the RSA signature) + if ( + child.tagClass === forge.asn1.Class.UNIVERSAL && + child.type === forge.asn1.Type.OCTETSTRING && + !child.constructed + ) { + encryptedDigestNode = child; + } + } + + if (!authAttrsNode || !encryptedDigestNode) { + return false; + } + + // Step 1: Verify message digest attribute matches content hash + const contentHash = createHash("sha256").update(content).digest(); + + const authAttrs = authAttrsNode.value as forge.asn1.Asn1[]; + let messageDigest: Buffer | null = null; + + for (const attr of authAttrs) { + const attrSeq = attr.value as forge.asn1.Asn1[]; + const oid = forge.asn1.derToOid(attrSeq[0].value as string); + if (oid === forge.pki.oids.messageDigest) { + // Attribute value: SET { OCTET STRING } + const valueSet = attrSeq[1].value as forge.asn1.Asn1[]; + messageDigest = Buffer.from(valueSet[0].value as string, "binary"); + break; + } + } + + if (!messageDigest || !contentHash.equals(messageDigest)) { + return false; + } + + // Step 2: Verify the RSA signature over the authenticated attributes + // Per RFC 5652 Section 5.4, the signature is computed over the + // DER-encoded authenticated attributes with EXPLICIT SET tag (0x31), + // not the IMPLICIT [0] tag (0xA0) used in the SignerInfo encoding. + const authAttrsDer = Buffer.from( + forge.asn1.toDer(authAttrsNode).getBytes(), + "binary", + ); + // Re-tag from CONTEXT_SPECIFIC [0] (0xA0) to SET (0x31) + authAttrsDer[0] = 0x31; + + // Get the signature bytes + const signatureBytes = Buffer.from( + encryptedDigestNode.value as string, + "binary", + ); + + // Get the signing certificate's public key as PEM + const signingCertForge = forge.pki.certificateFromAsn1(certsAsn1[0]); + const certPem = forge.pki.certificateToPem(signingCertForge); + + // Verify using Node.js built-in crypto + const verifier = createVerify("SHA256"); + verifier.update(authAttrsDer); + return verifier.verify(certPem, signatureBytes); + } catch { + return false; + } +} + /** * Creates a signature block buffer with PKCS#7 signature */ diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts index cbf1c4f..9c1d7e9 100755 --- a/test/sign.e2e.test.ts +++ b/test/sign.e2e.test.ts @@ -341,8 +341,9 @@ async function testSelfSignedSigning() { // Verify the signature const result = await verifyMcpbFile(testFile); - // Self-signed certs may not be trusted by OS, so we accept either status - expect(["self-signed", "unsigned"]).toContain(result.status); + // Self-signed certs should now be correctly identified as self-signed + expect(result.status).toBe("self-signed"); + expect(result.publisher).toBe("Test MCPB Publisher"); // Clean up fs.unlinkSync(testFile);