diff --git a/.changeset/weak-scrypt-single-warning.md b/.changeset/weak-scrypt-single-warning.md new file mode 100644 index 00000000..8409d549 --- /dev/null +++ b/.changeset/weak-scrypt-single-warning.md @@ -0,0 +1,5 @@ +--- +"@nodesecure/js-x-ray": minor +--- + +Measure the scrypt salt length in bytes and report one warning per call with every failing check joined in the value \ No newline at end of file diff --git a/docs/crypto.weak-scrypt.md b/docs/crypto.weak-scrypt.md index e4e6bdc7..f28125e6 100644 --- a/docs/crypto.weak-scrypt.md +++ b/docs/crypto.weak-scrypt.md @@ -9,7 +9,7 @@ Detect usage of **weak scrypt** parameters with the Node.js core `crypto.scrypt()` function. This probe checks for: - **low-cost**: scrypt parameters (cost, blockSize, parallelization) that do not meet [OWASP minimum recommendations](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt). -- **short-salt**: salt is a hardcoded string literal shorter than 16 characters. +- **short-salt**: salt is a hardcoded string literal shorter than 16 bytes. - **hardcoded-salt**: salt is a hardcoded string literal (should be randomly generated). ## Example diff --git a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts index 6ebbce4d..054a92f6 100644 --- a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts +++ b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts @@ -76,6 +76,7 @@ function main(node: ESTree.CallExpression, ctx: ProbeContext) { const { sourceFile } = ctx; const salt = node.arguments.at(1); const options = node.arguments.at(3); + const reasons: string[] = []; if (options && options.type === "ObjectExpression") { const { properties } = options; @@ -99,33 +100,22 @@ function main(node: ESTree.CallExpression, ctx: ProbeContext) { parallelizationValue ?? kDefaultParallelization ) ) { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "low-cost", - location: node.loc - }) - ); + reasons.push("low-cost"); } } } - if (isStringLiteral(salt)) { - if (typeof salt.value === "string" && salt.value.length < 16) { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "short-salt", - location: node.loc - }) - ); - } - else { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "hardcoded-salt", - location: node.loc - }) - ); - } + if (isStringLiteral(salt) && typeof salt.value === "string") { + reasons.push(Buffer.byteLength(salt.value) < 16 ? "short-salt" : "hardcoded-salt"); + } + + if (reasons.length > 0) { + sourceFile.warnings.push( + generateWarning("crypto.weak-scrypt", { + value: reasons.join(", "), + location: node.loc + }) + ); } } diff --git a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts index bf49149b..b8f6275c 100644 --- a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts +++ b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts @@ -5,16 +5,20 @@ import { describe, it } from "node:test"; // Import Internal Dependencies import { AstAnalyser } from "../../../src/AstAnalyser.ts"; +function analyse(code: string) { + return new AstAnalyser({ + optionalWarnings: ["crypto.weak-scrypt"] + }).analyse(code); +} + describe("isWeakScrypt", () => { describe("short-salt", () => { - it("should warn when salt is a short string literal (less than 16 chars)", () => { + it("should warn when salt is a short string literal (less than 16 bytes)", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "short", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -26,9 +30,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, "", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -37,14 +39,24 @@ describe("isWeakScrypt", () => { }); describe("hardcoded-salt", () => { - it("should warn when salt is a hardcoded string literal (16 chars or more)", () => { + it("should warn when salt is a hardcoded string literal of 16 bytes or more", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "this-is-a-long-hardcoded-salt", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); + + assert.strictEqual(outputWarnings.length, 1); + assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); + assert.strictEqual(outputWarnings[0].value, "hardcoded-salt"); + }); + + it("should measure the length of salt in bytes (6 chars but 18 bytes)", () => { + const code = ` + import crypto from 'crypto'; + crypto.scrypt(password, "가나다라마바", 64, (err, key) => {}); + `; + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -58,9 +70,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -72,9 +82,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { "cost": 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -86,9 +94,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 16384 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -99,9 +105,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 131072, r: 4 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -112,9 +116,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 8192, p: 9 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -125,9 +127,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 16384, parallelization: 5 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -137,9 +137,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 131072, p: 1, r: 8 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -149,28 +147,22 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 8192, p: 10 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); }); describe("combined warnings", () => { - it("should emit both short-salt and low-cost warnings", () => { + it("should report low-cost and short-salt in a single warning", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "abc", 64, { cost: 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); - assert.strictEqual(outputWarnings.length, 2); - const values = outputWarnings.map((w) => w.value); - assert.ok(values.includes("short-salt")); - assert.ok(values.includes("low-cost")); + assert.strictEqual(outputWarnings.length, 1); + assert.strictEqual(outputWarnings[0].value, "low-cost, short-salt"); }); }); @@ -181,9 +173,7 @@ describe("isWeakScrypt", () => { const salt = crypto.randomBytes(16); crypto.scrypt(password, salt, 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -193,9 +183,7 @@ describe("isWeakScrypt", () => { const crypto = { scrypt() {} }; crypto.scrypt(password, "short", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); });