From be820186a7f325936a5689aaf9090b1e7b4d02de Mon Sep 17 00:00:00 2001 From: King Star Date: Fri, 19 Jun 2026 19:05:28 +0800 Subject: [PATCH 1/2] Verify signed MCPB bundles --- src/node/sign.ts | 285 ++++++++++++++++++++++++++---------------- test/sign.e2e.test.ts | 3 +- 2 files changed, 179 insertions(+), 109 deletions(-) diff --git a/src/node/sign.ts b/src/node/sign.ts index bd4c8c4e..57668e69 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -41,69 +41,39 @@ export function signMcpbFile( readFileSync(path, "utf-8"), ); - // Create PKCS#7 signed data - const p7 = forge.pkcs7.createSignedData(); - p7.content = forge.util.createBuffer(mcpbContent); - - // Parse and add certificates - const signingCert = forge.pki.certificateFromPem(certificatePem); - const privateKey = forge.pki.privateKeyFromPem(privateKeyPem); + const initialSignature = createPkcs7Signature( + mcpbContent, + certificatePem, + privateKeyPem, + intermediatePems, + ); + let signatureBlockLength = createSignatureBlock(initialSignature).length; - p7.addCertificate(signingCert); + for (let attempts = 0; attempts < 3; attempts++) { + const updatedContent = updateEocdCommentLength( + mcpbContent, + signatureBlockLength, + ); + const signature = createPkcs7Signature( + updatedContent, + certificatePem, + privateKeyPem, + intermediatePems, + ); + const signatureBlock = createSignatureBlock(signature); - // Add intermediate certificates - if (intermediatePems) { - for (const pem of intermediatePems) { - p7.addCertificate(forge.pki.certificateFromPem(pem)); + if (signatureBlock.length !== signatureBlockLength) { + signatureBlockLength = signatureBlock.length; + continue; } - } - - // Add signer - p7.addSigner({ - key: privateKey, - certificate: signingCert, - digestAlgorithm: forge.pki.oids.sha256, - authenticatedAttributes: [ - { - type: forge.pki.oids.contentType, - value: forge.pki.oids.data, - }, - { - type: forge.pki.oids.messageDigest, - // Value will be auto-populated - }, - { - type: forge.pki.oids.signingTime, - // Value will be auto-populated with current time - }, - ], - }); - - // Sign with detached signature - p7.sign({ detached: true }); - // Convert to DER format - const asn1 = forge.asn1.toDer(p7.toAsn1()); - const pkcs7Signature = Buffer.from(asn1.getBytes(), "binary"); - - // Create signature block with PKCS#7 data - const signatureBlock = createSignatureBlock(pkcs7Signature); - - // Update ZIP EOCD comment_length to include signature block - // This ensures strict ZIP parsers accept the signed file - const updatedContent = Buffer.from(mcpbContent); - const eocdOffset = findEocdOffset(updatedContent); - if (eocdOffset !== -1) { - const currentCommentLength = updatedContent.readUInt16LE(eocdOffset + 20); - updatedContent.writeUInt16LE( - currentCommentLength + signatureBlock.length, - eocdOffset + 20, - ); + // Append signature block to MCPB file + const signedContent = Buffer.concat([updatedContent, signatureBlock]); + writeFileSync(mcpbPath, signedContent); + return; } - // Append signature block to MCPB file - const signedContent = Buffer.concat([updatedContent, signatureBlock]); - writeFileSync(mcpbPath, signedContent); + throw new Error("Failed to stabilize MCPB signature block length"); } /** @@ -137,17 +107,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 forge.pkcs7.PkcsSignedData; // Extract certificates from PKCS#7 const certificates = p7.certificates || []; @@ -158,34 +118,11 @@ 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) { + const signatureValid = await verifyPkcs7Signature( + pkcs7Signature, + originalContent, + ); + if (!signatureValid) { return { status: "unsigned" }; } @@ -195,22 +132,24 @@ export async function verifyMcpbFile( .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, - ); - - if (!chainValid) { - // Signature is valid but certificate is not trusted - return { status: "unsigned" }; - } - // Extract certificate info const isSelfSigned = signingCert.issuer.getField("CN")?.value === signingCert.subject.getField("CN")?.value; + if (!isSelfSigned) { + // Verify certificate chain against OS trust store + const chainValid = await verifyCertificateChain( + Buffer.from(certPem), + intermediatePems, + ); + + if (!chainValid) { + // Signature is valid but certificate is not trusted + return { status: "unsigned" }; + } + } + return { status: isSelfSigned ? "self-signed" : "signed", publisher: signingCert.subject.getField("CN")?.value || "Unknown", @@ -230,6 +169,138 @@ export async function verifyMcpbFile( } } +function createPkcs7Signature( + content: Buffer, + certificatePem: string, + privateKeyPem: string, + intermediatePems?: string[], +): Buffer { + const p7 = forge.pkcs7.createSignedData(); + p7.content = forge.util.createBuffer(content); + + const signingCert = forge.pki.certificateFromPem(certificatePem); + const privateKey = forge.pki.privateKeyFromPem(privateKeyPem); + + p7.addCertificate(signingCert); + + if (intermediatePems) { + for (const pem of intermediatePems) { + p7.addCertificate(forge.pki.certificateFromPem(pem)); + } + } + + p7.addSigner({ + key: privateKey, + certificate: signingCert, + digestAlgorithm: forge.pki.oids.sha256, + authenticatedAttributes: [ + { + type: forge.pki.oids.contentType, + value: forge.pki.oids.data, + }, + { + type: forge.pki.oids.messageDigest, + // Value will be auto-populated + }, + { + type: forge.pki.oids.signingTime, + // Value will be auto-populated with current time + }, + ], + }); + + p7.sign({ detached: true }); + + const asn1 = forge.asn1.toDer(p7.toAsn1()); + return Buffer.from(asn1.getBytes(), "binary"); +} + +function updateEocdCommentLength( + content: Buffer, + signatureBlockLength: number, +): Buffer { + const updatedContent = Buffer.from(content); + const eocdOffset = findEocdOffset(updatedContent); + if (eocdOffset === -1) { + return updatedContent; + } + + const currentCommentLength = updatedContent.readUInt16LE(eocdOffset + 20); + const updatedCommentLength = currentCommentLength + signatureBlockLength; + if (updatedCommentLength > 0xffff) { + throw new Error("Signature block exceeds ZIP comment length limit"); + } + + updatedContent.writeUInt16LE(updatedCommentLength, eocdOffset + 20); + return updatedContent; +} + +async function verifyPkcs7Signature( + pkcs7Signature: Buffer, + originalContent: Buffer, +): Promise { + let tempDir: string | null = null; + + try { + tempDir = await mkdtemp(join(tmpdir(), "mcpb-pkcs7-")); + const signaturePath = join(tempDir, "signature.der"); + const contentPath = join(tempDir, "content.bin"); + const outputPath = join(tempDir, "verified-content.bin"); + + await writeFile(signaturePath, pkcs7Signature); + await writeFile(contentPath, originalContent); + + if (process.platform === "win32") { + const psCommand = ` + $ErrorActionPreference = 'Stop' + $content = [System.IO.File]::ReadAllBytes('${escapePowerShellString(contentPath)}') + $signature = [System.IO.File]::ReadAllBytes('${escapePowerShellString(signaturePath)}') + $contentInfo = New-Object System.Security.Cryptography.Pkcs.ContentInfo -ArgumentList @(,$content) + $signedCms = New-Object System.Security.Cryptography.Pkcs.SignedCms -ArgumentList $contentInfo, $true + $signedCms.Decode($signature) + $signedCms.CheckSignature($true) + `.trim(); + + await execFileAsync("powershell.exe", [ + "-NoProfile", + "-NonInteractive", + "-Command", + psCommand, + ]); + } else { + await execFileAsync("openssl", [ + "cms", + "-verify", + "-inform", + "DER", + "-in", + signaturePath, + "-content", + contentPath, + "-noverify", + "-binary", + "-out", + outputPath, + ]); + } + return true; + } catch { + return false; + } finally { + if (tempDir) { + try { + await rm(tempDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors + } + } + } +} + +function escapePowerShellString(value: string): string { + return value.replace(/'/g, "''"); +} + /** * Finds the offset of the ZIP End of Central Directory record * by scanning backwards for the EOCD magic bytes (0x06054b50) diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts index 9af52a0c..3ffd900d 100755 --- a/test/sign.e2e.test.ts +++ b/test/sign.e2e.test.ts @@ -341,8 +341,7 @@ 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); + expect(result.status).toBe("self-signed"); // Clean up fs.unlinkSync(testFile); From df147189657f2153760cf1b131dcd41954acfa4e Mon Sep 17 00:00:00 2001 From: King Star Date: Sun, 4 Oct 2026 10:37:57 +0800 Subject: [PATCH 2/2] Fix MCPB signature verification --- src/cli/cli.ts | 16 ++++- src/node/sign.ts | 140 ++++++++++++++++++++++++++++++------------ src/shared/common.ts | 2 +- test/sign.e2e.test.ts | 68 +++++++++++++++++++- 4 files changed, 181 insertions(+), 45 deletions(-) diff --git a/src/cli/cli.ts b/src/cli/cli.ts index 5ba121c1..eb25799a 100644 --- a/src/cli/cli.ts +++ b/src/cli/cli.ts @@ -280,7 +280,13 @@ program ); console.log(`Fingerprint: ${result.fingerprint}`); } else { - console.error(`ERROR: Extension is not signed`); + const errorMessage = + result.status === "unsigned" + ? "Extension is not signed" + : result.status === "invalid" + ? "Signature is invalid" + : "Signature is valid, but the certificate is not trusted"; + console.error(`ERROR: ${errorMessage}`); process.exit(1); } } catch (error) { @@ -330,8 +336,14 @@ program ); console.log(` Fingerprint: ${signatureInfo.fingerprint}`); console.log(` Status: Valid (self-signed)`); - } else { + } else if (signatureInfo.status === "unsigned") { console.log(`\nWARNING: Not signed`); + } else if (signatureInfo.status === "invalid") { + console.log(`\nWARNING: Signature is invalid`); + } else { + console.log( + `\nWARNING: Signature is valid, but the certificate is not trusted`, + ); } } catch (error) { console.log( diff --git a/src/node/sign.ts b/src/node/sign.ts index 57668e69..e0e6ef21 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -96,15 +96,20 @@ export async function verifyMcpbFile( } // Parse PKCS#7 signature - const asn1 = forge.asn1.fromDer(pkcs7Signature.toString("binary")); - const p7Message = forge.pkcs7.messageFromAsn1(asn1); + let p7Message: ReturnType; + try { + const asn1 = forge.asn1.fromDer(pkcs7Signature.toString("binary")); + p7Message = forge.pkcs7.messageFromAsn1(asn1); + } catch { + return { status: "invalid" }; + } // Verify it's signed data and cast to correct type if ( !("type" in p7Message) || p7Message.type !== forge.pki.oids.signedData ) { - return { status: "unsigned" }; + return { status: "invalid" }; } const p7 = p7Message as forge.pkcs7.PkcsSignedData; @@ -112,7 +117,7 @@ export async function verifyMcpbFile( // Extract certificates from PKCS#7 const certificates = p7.certificates || []; if (certificates.length === 0) { - return { status: "unsigned" }; + return { status: "invalid" }; } // Get the signing certificate (first one) @@ -121,9 +126,10 @@ export async function verifyMcpbFile( const signatureValid = await verifyPkcs7Signature( pkcs7Signature, originalContent, + fileContent.length - originalContent.length, ); if (!signatureValid) { - return { status: "unsigned" }; + return { status: "invalid" }; } // Convert forge certificate to PEM for OS verification @@ -146,7 +152,7 @@ export async function verifyMcpbFile( if (!chainValid) { // Signature is valid but certificate is not trusted - return { status: "unsigned" }; + return { status: "untrusted" }; } } @@ -238,6 +244,7 @@ function updateEocdCommentLength( async function verifyPkcs7Signature( pkcs7Signature: Buffer, originalContent: Buffer, + signatureBlockLength: number, ): Promise { let tempDir: string | null = null; @@ -248,43 +255,57 @@ async function verifyPkcs7Signature( const outputPath = join(tempDir, "verified-content.bin"); await writeFile(signaturePath, pkcs7Signature); - await writeFile(contentPath, originalContent); - if (process.platform === "win32") { - const psCommand = ` - $ErrorActionPreference = 'Stop' - $content = [System.IO.File]::ReadAllBytes('${escapePowerShellString(contentPath)}') - $signature = [System.IO.File]::ReadAllBytes('${escapePowerShellString(signaturePath)}') - $contentInfo = New-Object System.Security.Cryptography.Pkcs.ContentInfo -ArgumentList @(,$content) - $signedCms = New-Object System.Security.Cryptography.Pkcs.SignedCms -ArgumentList $contentInfo, $true - $signedCms.Decode($signature) - $signedCms.CheckSignature($true) - `.trim(); + for (const content of getSignatureContentCandidates( + originalContent, + signatureBlockLength, + )) { + await writeFile(contentPath, content); - await execFileAsync("powershell.exe", [ - "-NoProfile", - "-NonInteractive", - "-Command", - psCommand, - ]); - } else { - await execFileAsync("openssl", [ - "cms", - "-verify", - "-inform", - "DER", - "-in", - signaturePath, - "-content", - contentPath, - "-noverify", - "-binary", - "-out", - outputPath, - ]); + try { + if (process.platform === "win32") { + const psCommand = ` + $ErrorActionPreference = 'Stop' + $content = [System.IO.File]::ReadAllBytes('${escapePowerShellString(contentPath)}') + $signature = [System.IO.File]::ReadAllBytes('${escapePowerShellString(signaturePath)}') + $contentInfo = New-Object System.Security.Cryptography.Pkcs.ContentInfo -ArgumentList @(,$content) + $signedCms = New-Object System.Security.Cryptography.Pkcs.SignedCms -ArgumentList $contentInfo, $true + $signedCms.Decode($signature) + $signedCms.CheckSignature($true) + `.trim(); + + await execFileAsync("powershell.exe", [ + "-NoProfile", + "-NonInteractive", + "-Command", + psCommand, + ]); + } else { + await execFileAsync("openssl", [ + "cms", + "-verify", + "-inform", + "DER", + "-in", + signaturePath, + "-content", + contentPath, + "-noverify", + "-binary", + "-out", + outputPath, + ]); + } + return true; + } catch (error) { + if (!isChildProcessExit(error)) { + throw error; + } + // Older MCPB signers patched the EOCD comment length after signing. + // Try the pre-patch bytes before treating the signature as invalid. + } } - return true; - } catch { + return false; } finally { if (tempDir) { @@ -297,6 +318,36 @@ async function verifyPkcs7Signature( } } +function isChildProcessExit(error: unknown): boolean { + return ( + typeof error === "object" && + error !== null && + typeof (error as NodeJS.ErrnoException).code === "number" + ); +} + +function getSignatureContentCandidates( + originalContent: Buffer, + signatureBlockLength: number, +): Buffer[] { + const candidates = [originalContent]; + const eocdOffset = findEocdOffset(originalContent); + + if (eocdOffset !== -1) { + const storedCommentLength = originalContent.readUInt16LE(eocdOffset + 20); + if (storedCommentLength >= signatureBlockLength) { + const prePatchContent = Buffer.from(originalContent); + prePatchContent.writeUInt16LE( + storedCommentLength - signatureBlockLength, + eocdOffset + 20, + ); + candidates.push(prePatchContent); + } + } + + return candidates; +} + function escapePowerShellString(value: string): string { return value.replace(/'/g, "''"); } @@ -417,6 +468,9 @@ export async function verifyCertificateChain( ]); return true; } catch (error) { + if (!isChildProcessExit(error)) { + throw error; + } return false; } } else if (process.platform === "win32") { @@ -481,10 +535,16 @@ export async function verifyCertificateChain( ]); return true; } catch (error) { + if (!isChildProcessExit(error)) { + throw error; + } return false; } } } catch (error) { + if (!isChildProcessExit(error)) { + throw error; + } return false; } finally { if (tempDir) { diff --git a/src/shared/common.ts b/src/shared/common.ts index 9a8aeb3e..837bd2c7 100644 --- a/src/shared/common.ts +++ b/src/shared/common.ts @@ -6,7 +6,7 @@ export const McpbUserConfigValuesSchema = z.record( ); export const McpbSignatureInfoSchema = z.strictObject({ - status: z.enum(["signed", "unsigned", "self-signed"]), + status: z.enum(["signed", "unsigned", "self-signed", "invalid", "untrusted"]), publisher: z.string().optional(), issuer: z.string().optional(), valid_from: z.string().optional(), diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts index 3ffd900d..ab36d28e 100755 --- a/test/sign.e2e.test.ts +++ b/test/sign.e2e.test.ts @@ -36,6 +36,52 @@ const CA_KEY = path.join(TEST_DIR, "ca.key"); const SIGNED_CERT = path.join(TEST_DIR, "signed.crt"); const SIGNED_KEY = path.join(TEST_DIR, "signed.key"); +function signMcpbFileWithLegacyEocd(pathToFile: string) { + const content = fs.readFileSync(pathToFile); + const p7 = forge.pkcs7.createSignedData(); + p7.content = forge.util.createBuffer(content); + + const certificate = forge.pki.certificateFromPem( + fs.readFileSync(SELF_SIGNED_CERT, "utf-8"), + ); + const privateKey = forge.pki.privateKeyFromPem( + fs.readFileSync(SELF_SIGNED_KEY, "utf-8"), + ); + p7.addCertificate(certificate); + p7.addSigner({ + key: privateKey, + certificate, + digestAlgorithm: forge.pki.oids.sha256, + authenticatedAttributes: [ + { type: forge.pki.oids.contentType, value: forge.pki.oids.data }, + { type: forge.pki.oids.messageDigest }, + { type: forge.pki.oids.signingTime }, + ], + }); + p7.sign({ detached: true }); + + const signature = Buffer.from( + forge.asn1.toDer(p7.toAsn1()).getBytes(), + "binary", + ); + const signatureLength = Buffer.alloc(4); + signatureLength.writeUInt32LE(signature.length, 0); + const signatureBlock = Buffer.concat([ + Buffer.from("MCPB_SIG_V1", "utf-8"), + signatureLength, + signature, + Buffer.from("MCPB_SIG_END", "utf-8"), + ]); + + const eocdOffset = content.lastIndexOf(Buffer.from([0x50, 0x4b, 0x05, 0x06])); + expect(eocdOffset).toBeGreaterThanOrEqual(0); + content.writeUInt16LE( + content.readUInt16LE(eocdOffset + 20) + signatureBlock.length, + eocdOffset + 20, + ); + fs.writeFileSync(pathToFile, Buffer.concat([content, signatureBlock])); +} + /** * Generate a self-signed certificate for testing */ @@ -347,6 +393,20 @@ async function testSelfSignedSigning() { fs.unlinkSync(testFile); } +/** + * Test signatures produced before the signer began patching the EOCD first. + */ +async function testLegacySignedFile() { + const testFile = path.join(TEST_DIR, "test-legacy-signed.dxt"); + fs.copyFileSync(TEST_MCPB, testFile); + signMcpbFileWithLegacyEocd(testFile); + + const result = await verifyMcpbFile(testFile); + expect(result.status).toBe("self-signed"); + + fs.unlinkSync(testFile); +} + /** * Test signing with CA-signed certificate */ @@ -364,7 +424,7 @@ async function testCASignedSigning() { const result = await verifyMcpbFile(testFile); // CA-signed status depends on whether test CA is trusted by OS - expect(["signed", "unsigned"]).toContain(result.status); + expect(result.status).toBe("untrusted"); // Clean up fs.unlinkSync(testFile); @@ -389,7 +449,7 @@ async function testTamperingDetection() { // Try to verify - should fail const result = await verifyMcpbFile(testFile); - expect(result.status).toBe("unsigned"); + expect(result.status).toBe("invalid"); // Clean up fs.unlinkSync(testFile); @@ -452,6 +512,10 @@ describe("MCPB Signing E2E Tests", () => { await testSelfSignedSigning(); }); + it("should verify files signed before the EOCD length fix", async () => { + await testLegacySignedFile(); + }); + it("should sign and verify with CA-signed certificate", async () => { await testCASignedSigning(); });