Add RSA key size validation requiring >= 2048 bits

src/node/sign.ts

@@ -11,6 +11,7 @@ import type { DxtSignatureInfo } from "../types.js";
 // Signature block markers
 const SIGNATURE_HEADER = "DXT_SIG_V1";
 const SIGNATURE_FOOTER = "DXT_SIG_END";
+const MINIMUM_RSA_KEY_SIZE = 2048;
 
 const execFileAsync = promisify(execFile);
 
@@ -48,6 +49,16 @@ export function signDxtFile(
   const signingCert = forge.pki.certificateFromPem(certificatePem);
   const privateKey = forge.pki.privateKeyFromPem(privateKeyPem);
 
+  // Validate RSA key size (require >= 2048 bits for security)
+  if ("n" in privateKey && privateKey.n) {
+    const keySize = privateKey.n.bitLength();
+    if (keySize < MINIMUM_RSA_KEY_SIZE) {
+      throw new Error(
+        `RSA key size ${keySize} bits is too small. Minimum required is 2048 bits.`,
+      );
+    }
+  }
+
   p7.addCertificate(signingCert);
 
   // Add intermediate certificates
@@ -145,6 +156,15 @@ export async function verifyDxtFile(
     // Get the signing certificate (first one)
     const signingCert = certificates[0];
 
+    // Validate RSA key size (require >= 2048 bits for security)
+    const publicKey = signingCert.publicKey;
+    if ("n" in publicKey && publicKey.n) {
+      const keySize = publicKey.n.bitLength();
+      if (keySize < MINIMUM_RSA_KEY_SIZE) {
+        return { status: "unsigned" };
+      }
+    }
+
     // Verify PKCS#7 signature
     const contentBuf = forge.util.createBuffer(originalContent);
 
@@ -240,13 +260,20 @@ function createSignatureBlock(pkcs7Signature: Buffer): Buffer {
 
 /**
  * Extracts the signature block from a signed DXT file
+ *
+ * SECURITY NOTE: This function defines the boundary between content that gets
+ * signature-verified and the signature itself. It is critical that this function
+ * is implemented correctly scanning backwards from the end of the file to extract
+ * the last signature block.
  */
 export function extractSignatureBlock(fileContent: Buffer): {
   originalContent: Buffer;
   pkcs7Signature?: Buffer;
 } {
   // Look for signature footer at the end
   const footerBytes = Buffer.from(SIGNATURE_FOOTER, "utf-8");
+  // SECURITY NOTE: Using lastIndexOf to find the last footer marker is critical.
+  // Using indexOf would find the first one and could lead to vulnerabilities.
   const footerIndex = fileContent.lastIndexOf(footerBytes);
 
   if (footerIndex === -1) {
@@ -257,7 +284,8 @@ export function extractSignatureBlock(fileContent: Buffer): {
   const headerBytes = Buffer.from(SIGNATURE_HEADER, "utf-8");
   let headerIndex = -1;
 
-  // Search backwards from footer
+  // Search backwards from footer - finds FIRST header marker going backward
+  // SECURITY NOTE: It is critical to search backwards and take the first one found.
   for (let i = footerIndex - 1; i >= 0; i--) {
     if (fileContent.slice(i, i + headerBytes.length).equals(headerBytes)) {
       headerIndex = i;
@@ -270,6 +298,9 @@ export function extractSignatureBlock(fileContent: Buffer): {
   }
 
   // Extract original content (everything before signature block)
+  // SECURITY NOTE: It is critical to extract everything before the header. While zip parsers
+  // may not require that you separate this from the header, this is required from a
+  // security perspective.
   const originalContent = fileContent.slice(0, headerIndex);
 
   // Parse signature block
@@ -400,7 +431,10 @@ export async function verifyCertificateChain(
 }
 
 /**
- * Removes signature from a DXT file
+ * Removes signature from a DXT file. 
+ * 
+ * SECURITY NOTE: It is critical to call unsignDxtFile after verifying
+ * the signature. Failing to do so may lead to signature bypasses.
  */
 export function unsignDxtFile(dxtPath: string): void {
   const fileContent = readFileSync(dxtPath);

test/sign.e2e.test.ts

@@ -31,6 +31,8 @@ const CA_CERT = path.join(TEST_DIR, "ca.crt");
 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");
+const WEAK_CERT = path.join(TEST_DIR, "weak.crt");
+const WEAK_KEY = path.join(TEST_DIR, "weak.key");
 
 /**
  * Generate a self-signed certificate for testing
@@ -82,6 +84,50 @@ function generateSelfSignedCert() {
   fs.writeFileSync(SELF_SIGNED_KEY, forge.pki.privateKeyToPem(keys.privateKey));
 }
 
+/**
+ * Generate a weak certificate with RSA-1024 key for testing key size validation
+ */
+function generateWeakCert() {
+  const keys = forge.pki.rsa.generateKeyPair(1024); // Weak key size
+  const cert = forge.pki.createCertificate();
+
+  cert.publicKey = keys.publicKey;
+  cert.serialNumber = "01";
+  cert.validity.notBefore = new Date();
+  cert.validity.notAfter = new Date();
+  cert.validity.notAfter.setFullYear(cert.validity.notBefore.getFullYear() + 1);
+
+  const attrs = [
+    { name: "commonName", value: "Test Weak Key Publisher" },
+    { name: "countryName", value: "US" },
+    { name: "organizationName", value: "Test Weak Org" },
+  ];
+
+  cert.setSubject(attrs);
+  cert.setIssuer(attrs);
+
+  cert.setExtensions([
+    {
+      name: "basicConstraints",
+      cA: false,
+    },
+    {
+      name: "keyUsage",
+      digitalSignature: true,
+      nonRepudiation: true,
+    },
+    {
+      name: "extKeyUsage",
+      codeSigning: true,
+    },
+  ]);
+
+  cert.sign(keys.privateKey, forge.md.sha256.create());
+
+  fs.writeFileSync(WEAK_CERT, forge.pki.certificateToPem(cert));
+  fs.writeFileSync(WEAK_KEY, forge.pki.privateKeyToPem(keys.privateKey));
+}
+
 /**
  * Generate a CA and signed certificate for testing
  */
@@ -423,6 +469,112 @@ async function testSignatureRemoval() {
   fs.unlinkSync(testFile);
 }
 
+/**
+ * Test RSA key size validation during signing
+ */
+async function testWeakKeyRejection() {
+  // Create a copy for this test
+  const testFile = path.join(TEST_DIR, "test-weak-key.dxt");
+  fs.copyFileSync(TEST_DXT, testFile);
+
+  // Try to sign with weak key - should throw
+  expect(() => signDxtFile(testFile, WEAK_CERT, WEAK_KEY)).toThrow(
+    /RSA key size.*too small.*Minimum required is 2048 bits/,
+  );
+
+  // Clean up
+  fs.unlinkSync(testFile);
+}
+
+/**
+ * Test RSA key size validation during verification by creating
+ * a signed file with weak key using bypassed signing
+ */
+async function testWeakKeyVerificationRejection() {
+  // We need to create a signed DXT with weak key by bypassing our signing validation.
+  // We'll construct the PKCS#7 signature manually and append it to create a file
+  // that has a valid PKCS#7 structure but uses a weak key.
+
+  const testFile = path.join(TEST_DIR, "test-weak-verification.dxt");
+  fs.copyFileSync(TEST_DXT, testFile);
+
+  // Read the test DXT content
+  const dxtContent = fs.readFileSync(testFile);
+
+  // Create a weak key and certificate
+  const weakKeys = forge.pki.rsa.generateKeyPair(1024);
+  const weakCert = forge.pki.createCertificate();
+
+  weakCert.publicKey = weakKeys.publicKey;
+  weakCert.serialNumber = "01";
+  weakCert.validity.notBefore = new Date();
+  weakCert.validity.notAfter = new Date();
+  weakCert.validity.notAfter.setFullYear(
+    weakCert.validity.notBefore.getFullYear() + 1,
+  );
+
+  const attrs = [
+    { name: "commonName", value: "Test Weak Verification Publisher" },
+    { name: "countryName", value: "US" },
+    { name: "organizationName", value: "Test Weak Verification Org" },
+  ];
+
+  weakCert.setSubject(attrs);
+  weakCert.setIssuer(attrs);
+  weakCert.setExtensions([
+    { name: "basicConstraints", cA: false },
+    { name: "keyUsage", digitalSignature: true },
+    { name: "extKeyUsage", codeSigning: true },
+  ]);
+
+  // Self-sign
+  weakCert.sign(weakKeys.privateKey, forge.md.sha256.create());
+
+  // Create PKCS#7 manually bypassing our signing function
+  const p7 = forge.pkcs7.createSignedData();
+  p7.content = forge.util.createBuffer(dxtContent);
+  p7.addCertificate(weakCert);
+  p7.addSigner({
+    key: weakKeys.privateKey,
+    certificate: weakCert,
+    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 });
+
+  // Convert to DER format and create signature block
+  const asn1 = forge.asn1.toDer(p7.toAsn1());
+  const pkcs7Signature = Buffer.from(asn1.getBytes(), "binary");
+
+  // Create signature block manually (same as createSignatureBlock function)
+  const headerBytes = Buffer.from("DXT_SIG_V1", "utf-8");
+  const footerBytes = Buffer.from("DXT_SIG_END", "utf-8");
+  const sigLengthBuffer = Buffer.alloc(4);
+  sigLengthBuffer.writeUInt32LE(pkcs7Signature.length, 0);
+
+  const signatureBlock = Buffer.concat([
+    headerBytes,
+    sigLengthBuffer,
+    pkcs7Signature,
+    footerBytes,
+  ]);
+
+  // Append signature block to DXT file
+  const signedContent = Buffer.concat([dxtContent, signatureBlock]);
+  fs.writeFileSync(testFile, signedContent);
+
+  // Try to verify - should reject due to weak key
+  const result = await verifyDxtFile(testFile);
+  expect(result.status).toBe("unsigned");
+
+  // Clean up
+  fs.unlinkSync(testFile);
+}
+
 describe("DXT Signing E2E Tests", () => {
   beforeAll(() => {
     // Ensure test directory exists
@@ -433,6 +585,7 @@ describe("DXT Signing E2E Tests", () => {
     // Setup
     generateSelfSignedCert();
     generateCASignedCert();
+    generateWeakCert();
     createTestDxt();
   });
 
@@ -464,4 +617,12 @@ describe("DXT Signing E2E Tests", () => {
   it("should remove signatures", async () => {
     await testSignatureRemoval();
   });
+
+  it("should reject weak RSA keys during signing", async () => {
+    await testWeakKeyRejection();
+  });
+
+  it("should reject weak RSA keys during verification", async () => {
+    await testWeakKeyVerificationRejection();
+  });
 });