From 7027d1297d9b90a7b1cedef452448bad34580b93 Mon Sep 17 00:00:00 2001 From: Joan Xie Date: Sat, 18 Oct 2025 11:52:56 -0700 Subject: [PATCH 1/2] Fix PKCS#7 signature verification - Implement manual PKCS#7 signature verification to replace broken node-forge p7.verify() - Verify message digest matches content - Verify cryptographic signature using certificate's public key - Fix self-signed certificate handling to return 'self-signed' status even when not in OS trust store - Fixes #46 --- src/node/sign.ts | 126 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 96 insertions(+), 30 deletions(-) diff --git a/src/node/sign.ts b/src/node/sign.ts index bb83389..4dfa8dc 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -145,37 +145,101 @@ export async function verifyMcpbFile( // Get the signing certificate (first one) const signingCert = certificates[0]; - // Verify PKCS#7 signature - const contentBuf = forge.util.createBuffer(originalContent); - + // Manually verify PKCS#7 signature (node-forge's verify() is not implemented) 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" }; + if (!signerInfo) { + return { status: "unsigned" }; + } + + // Step 1: Verify the message digest in authenticated attributes matches the content + const md = forge.md.sha256.create(); + md.update(forge.util.createBuffer(originalContent).getBytes()); + const contentDigest = md.digest().getBytes(); + + // Find and verify the message digest attribute + let messageDigestAttr = null; + for (const attr of signerInfo.authenticatedAttributes) { + if (attr.type === forge.pki.oids.messageDigest) { + messageDigestAttr = attr.value; + break; } } + + if (!messageDigestAttr || messageDigestAttr !== contentDigest) { + return { status: "unsigned" }; + } + + // Step 2: Verify the signature over the authenticated attributes + // Create a DER encoding of the authenticated attributes for signature verification + const authenticatedAttributesAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SET, + true, + signerInfo.authenticatedAttributes.map((attr: any) => + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OID, + false, + forge.asn1.oidToDer(attr.type).getBytes(), + ), + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SET, + true, + [ + typeof attr.value === "string" + ? forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OCTETSTRING, + false, + attr.value, + ) + : attr.value, + ], + ), + ], + ), + ), + ); + + const bytes = forge.asn1.toDer(authenticatedAttributesAsn1).getBytes(); + + // Hash the authenticated attributes + const attrMd = forge.md.sha256.create(); + attrMd.update(bytes); + + // Verify the signature using the certificate's public key + // Cast to rsa.PublicKey since PKCS#7 typically uses RSA + const publicKey = signingCert.publicKey as forge.pki.rsa.PublicKey; + if (!publicKey || typeof publicKey === "object" && Buffer.isBuffer(publicKey)) { + return { status: "unsigned" }; + } + + const verified = publicKey.verify( + attrMd.digest().getBytes(), + (signerInfo as any).signature, + ); + + if (!verified) { + return { status: "unsigned" }; + } } catch (error) { return { status: "unsigned" }; } + // Check if certificate is self-signed + const isSelfSigned = + signingCert.issuer.getField("CN")?.value === + signingCert.subject.getField("CN")?.value; + // Convert forge certificate to PEM for OS verification const certPem = forge.pki.certificateToPem(signingCert); const intermediatePems = certificates @@ -188,18 +252,20 @@ export async function verifyMcpbFile( intermediatePems, ); - if (!chainValid) { - // Signature is valid but certificate is not trusted - return { status: "unsigned" }; + // Determine status based on trust validation + let status: "signed" | "self-signed" | "unsigned"; + if (chainValid) { + // Certificate is trusted by OS + status = isSelfSigned ? "self-signed" : "signed"; + } else { + // Signature is cryptographically valid but certificate is not trusted + // For self-signed certificates, still report as self-signed (not unsigned) + // For other certificates, report as unsigned (untrusted) + status = isSelfSigned ? "self-signed" : "unsigned"; } - // Extract certificate info - const isSelfSigned = - signingCert.issuer.getField("CN")?.value === - signingCert.subject.getField("CN")?.value; - return { - status: isSelfSigned ? "self-signed" : "signed", + status, publisher: signingCert.subject.getField("CN")?.value || "Unknown", issuer: signingCert.issuer.getField("CN")?.value || "Unknown", valid_from: signingCert.validity.notBefore.toISOString(), From 2f9030f9215833bddade8cf70fcf390223ecf64b Mon Sep 17 00:00:00 2001 From: Joan Xie Date: Mon, 20 Oct 2025 08:35:08 -0700 Subject: [PATCH 2/2] Fix lint errors: remove explicit any types and fix formatting --- src/node/sign.ts | 26 +++++++++++++++++--------- src/schemas.ts | 2 +- test/schemas.test.ts | 11 ++++++++--- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/node/sign.ts b/src/node/sign.ts index 4dfa8dc..99320d4 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -126,13 +126,18 @@ export async function verifyMcpbFile( // Now we know it's PkcsSignedData. The types are incorrect, so we'll // fix them there + interface AuthenticatedAttribute { + type: string; + value: string | forge.asn1.Asn1; + } + + interface SignerInfo { + authenticatedAttributes: AuthenticatedAttribute[]; + signature: string; + } + const p7 = p7Message as unknown as forge.pkcs7.PkcsSignedData & { - signerInfos: Array<{ - authenticatedAttributes: Array<{ - type: string; - value: unknown; - }>; - }>; + signerInfos: SignerInfo[]; verify: (options?: { authenticatedAttributes?: boolean }) => boolean; }; @@ -178,7 +183,7 @@ export async function verifyMcpbFile( forge.asn1.Class.UNIVERSAL, forge.asn1.Type.SET, true, - signerInfo.authenticatedAttributes.map((attr: any) => + signerInfo.authenticatedAttributes.map((attr: AuthenticatedAttribute) => forge.asn1.create( forge.asn1.Class.UNIVERSAL, forge.asn1.Type.SEQUENCE, @@ -219,13 +224,16 @@ export async function verifyMcpbFile( // Verify the signature using the certificate's public key // Cast to rsa.PublicKey since PKCS#7 typically uses RSA const publicKey = signingCert.publicKey as forge.pki.rsa.PublicKey; - if (!publicKey || typeof publicKey === "object" && Buffer.isBuffer(publicKey)) { + if ( + !publicKey || + (typeof publicKey === "object" && Buffer.isBuffer(publicKey)) + ) { return { status: "unsigned" }; } const verified = publicKey.verify( attrMd.digest().getBytes(), - (signerInfo as any).signature, + signerInfo.signature, ); if (!verified) { diff --git a/src/schemas.ts b/src/schemas.ts index 50777b8..3a68ab1 100644 --- a/src/schemas.ts +++ b/src/schemas.ts @@ -100,7 +100,7 @@ export const McpbManifestSchema = z screenshots: z.array(z.string()).optional(), server: McpbManifestServerSchema, tools: z.array(McpbManifestToolSchema).optional(), - tools_generated: z.boolean().optional(), + tools_generated: z.boolean().optional(), prompts: z.array(McpbManifestPromptSchema).optional(), prompts_generated: z.boolean().optional(), keywords: z.array(z.string()).optional(), diff --git a/test/schemas.test.ts b/test/schemas.test.ts index 86c6ffa..23ab031 100644 --- a/test/schemas.test.ts +++ b/test/schemas.test.ts @@ -155,7 +155,10 @@ describe("McpbManifestSchema", () => { const manifest = { ...base, _meta: { - "com.microsoft.windows": { package_family_name: "Pkg_123", channel: "stable" }, + "com.microsoft.windows": { + package_family_name: "Pkg_123", + channel: "stable", + }, "com.apple.darwin": { bundle_id: "com.example.app", notarized: true }, }, }; @@ -167,7 +170,10 @@ describe("McpbManifestSchema", () => { const manifest = { ...base, _meta: { - "com.microsoft.windows": "raw-string" as unknown as Record, + "com.microsoft.windows": "raw-string" as unknown as Record< + string, + unknown + >, }, }; const result = McpbManifestSchema.safeParse(manifest); @@ -206,5 +212,4 @@ describe("McpbManifestSchema", () => { expect(result.success).toBe(true); }); }); - });