-
Notifications
You must be signed in to change notification settings - Fork 39
feat(js-x-ray): add weak-argon2 detection probe #592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@nodesecure/js-x-ray": minor | ||
| --- | ||
|
|
||
| add weak-argon2 detection probe |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # Weak argon2 | ||
|
|
||
| | Code | Severity | i18n | Experimental | | ||
| | --- | --- | --- | :-: | | ||
| | weak-argon2 | `Warning` | `sast_warnings.weak_argon2` | :x: | | ||
|
|
||
| ## Introduction | ||
|
|
||
| Detect usage of **weak Argon2** parameters with the Node.js core `crypto.argon2()` / `crypto.argon2Sync()` functions. This probe checks for: | ||
|
|
||
| - **wrong-algorithm**: using `argon2d` or `argon2i` instead of the recommended `argon2id`. | ||
| - **weak-parameters**: memory, passes, or parallelism values that do not meet [OWASP minimum recommendations](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id). | ||
| - **hardcoded-nonce**: nonce (salt) is a hardcoded string literal (should be randomly generated). | ||
|
|
||
| ## Example | ||
|
|
||
| ```js | ||
| import crypto from "crypto"; | ||
|
|
||
| // wrong-algorithm: argon2d is vulnerable to side-channel attacks | ||
| crypto.argon2("argon2d", { | ||
| message: "password", | ||
| nonce: crypto.randomBytes(16), | ||
| memory: 47104, | ||
| passes: 1, | ||
| parallelism: 1, | ||
| tagLength: 64 | ||
| }); | ||
|
|
||
| // weak-parameters: memory and passes are below OWASP minimum | ||
| crypto.argon2("argon2id", { | ||
| message: "password", | ||
| nonce: crypto.randomBytes(16), | ||
| memory: 512, | ||
| passes: 1, | ||
| parallelism: 1, | ||
| tagLength: 64 | ||
| }); | ||
|
|
||
| // hardcoded-nonce: nonce should be randomly generated | ||
| crypto.argon2("argon2id", { | ||
| message: "password", | ||
| nonce: "hardcoded-salt", | ||
| memory: 47104, | ||
| passes: 1, | ||
| parallelism: 1, | ||
| tagLength: 64 | ||
| }); | ||
| ``` | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| // Import Third-party Dependencies | ||
| import type { ESTree } from "meriyah"; | ||
|
|
||
| // Import Internal Dependencies | ||
| import type { ProbeContext } from "../ProbeRunner.ts"; | ||
| import { CALL_EXPRESSION_DATA } from "../contants.ts"; | ||
| import { isLiteral } from "../estree/types.ts"; | ||
| import { generateWarning } from "../warnings.ts"; | ||
|
|
||
| const kOWASPMinParams: [minMemory: number, minIteration: number, minParallelism: number][] = [ | ||
| [47104, 1, 1], | ||
| [19456, 2, 1], | ||
| [12288, 3, 1], | ||
| [9216, 4, 1], | ||
| [7168, 5, 1] | ||
| ]; | ||
|
|
||
| const tracedFunctions = new Set(["crypto.argon2", "crypto.argon2Sync"]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess, just creating an array is ok here, you already know that there will be no duplicate |
||
|
|
||
| function extractNumericParam( | ||
| properties: ESTree.Property[], | ||
| names: string[] | ||
| ): number | null { | ||
| for (const prop of properties) { | ||
| if ( | ||
| prop.key.type === "Identifier" && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, it would be nice to check for identifier that are literal as well |
||
| names.includes(prop.key.name) && | ||
| prop.value.type === "Literal" && | ||
| typeof prop.value.value === "number" | ||
| ) { | ||
| return prop.value.value; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function isWeakArgon2Params(memory: number, iteration: number, parallelism: number): boolean { | ||
| for (const [minMemory, minIteration, minParallelism] of kOWASPMinParams) { | ||
| if (memory >= minMemory) { | ||
| return parallelism < minParallelism || iteration < minIteration; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| function validateNode( | ||
| _node: ESTree.Node, | ||
| ctx: ProbeContext): [boolean, any?] { | ||
| const { tracer } = ctx.sourceFile; | ||
|
|
||
| if (!tracer.importedModules.has("crypto")) { | ||
| return [false]; | ||
| } | ||
|
|
||
| return [ | ||
| tracedFunctions.has(ctx.context![CALL_EXPRESSION_DATA]?.identifierOrMemberExpr) | ||
| ]; | ||
| } | ||
|
|
||
| function initialize(ctx: ProbeContext) { | ||
| const { tracer } = ctx.sourceFile; | ||
|
|
||
| for (const identifierOrMemberExpr of tracedFunctions) { | ||
| tracer.trace(identifierOrMemberExpr, { | ||
| followConsecutiveAssignment: true, | ||
| moduleName: "crypto" | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| function main(node: ESTree.CallExpression, ctx: ProbeContext) { | ||
| const { sourceFile } = ctx; | ||
| const algorithm = node.arguments.at(0); | ||
| if (isLiteral(algorithm)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the isLiteral case will be the most common one for sure, but imo we should also check the case where it is an identifier: const algo = "argon2d";
crypto.argon2(algo, {
message: "password",
nonce: crypto.randomBytes(16),
memory: 47104,
passes: 1,
parallelism: 1,
tagLength: 64
});
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can achieve that like this: // when the type is "Identifier"
const literalIdentifier = tracer.literalIdentifiers.get(node.name); |
||
| if (algorithm.value !== "argon2id") { | ||
| sourceFile.warnings.push( | ||
| generateWarning("weak-argon2", { | ||
| value: "wrong-algorithm", | ||
|
clemgbld marked this conversation as resolved.
Outdated
|
||
| location: node.loc | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const parameters = node.arguments.at(1); | ||
| if (parameters && parameters.type === "ObjectExpression") { | ||
| const properties = parameters.properties.filter( | ||
| (prop): prop is ESTree.Property => prop.type === "Property" | ||
| ); | ||
| const memory = extractNumericParam(properties, ["memory"]); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess a better way to do it would be to extract each properties in one loop then extract the numeric value |
||
| const iteration = extractNumericParam(properties, ["passes"]); | ||
| const parallelism = extractNumericParam(properties, ["parallelism"]); | ||
|
|
||
| if (memory && iteration && parallelism) { | ||
| if (isWeakArgon2Params(memory, iteration, parallelism)) { | ||
| sourceFile.warnings.push( | ||
| generateWarning("weak-argon2", { | ||
| value: "weak-parameters", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the value it would be also more helpful to add the type of the parameters that is weak: |
||
| location: node.loc | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const nonce = properties.find( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here it should be part of just one loop rather than iterating once again |
||
| (prop) => prop.key.type === "Identifier" && prop.key.name === "nonce" | ||
| ); | ||
|
|
||
| if (nonce && isLiteral(nonce.value)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here we should check for identifier that are hardcoded |
||
| sourceFile.warnings.push( | ||
| generateWarning("weak-argon2", { | ||
| value: "hardcoded-nonce", | ||
| location: node.loc | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export default { | ||
| name: "isWeakArgon2", | ||
| validateNode, | ||
| main, | ||
| initialize, | ||
| breakOnMatch: false, | ||
| context: {} | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should use the term "probe" since it is an implementation detail of Js-x-ray