Verify signed MCPB bundles

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<boolean> {
+  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)

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);