From f62e87a9d88e56541cb3be05bd42fd5bf614ac06 Mon Sep 17 00:00:00 2001 From: David Dworken Date: Mon, 30 Jun 2025 16:18:53 -0700 Subject: [PATCH] Add RSA key size validation requiring >= 2048 bits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds validation during both signing and verification to reject weak RSA keys: - Signing: Throws clear error if key < 2048 bits - Verification: Returns "unsigned" status if key < 2048 bits - Comprehensive test coverage for both paths 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- src/node/sign.ts | 38 +++++++++- test/sign.e2e.test.ts | 161 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+), 2 deletions(-) mode change 100755 => 100644 test/sign.e2e.test.ts diff --git a/src/node/sign.ts b/src/node/sign.ts index f87c570..a5f5833 100644 --- a/src/node/sign.ts +++ b/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,6 +260,11 @@ 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; @@ -247,6 +272,8 @@ export function extractSignatureBlock(fileContent: 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); diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts old mode 100755 new mode 100644 index 999c039..424c9e6 --- a/test/sign.e2e.test.ts +++ b/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(); + }); });