feat(js-x-ray): add weak-argon2 detection probe#592
feat(js-x-ray): add weak-argon2 detection probe#592HoyeongJeon wants to merge 2 commits intoNodeSecure:masterfrom
Conversation
🦋 Changeset detectedLatest commit: efc68e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| function main(node: ESTree.CallExpression, ctx: ProbeContext) { | ||
| const { sourceFile } = ctx; | ||
| const algorithm = node.arguments.at(0); | ||
| if (isLiteral(algorithm)) { |
There was a problem hiding this comment.
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
});There was a problem hiding this comment.
you can achieve that like this:
// when the type is "Identifier"
const literalIdentifier = tracer.literalIdentifiers.get(node.name);
|
|
||
| ## Introduction | ||
|
|
||
| Detect usage of **weak Argon2** parameters with the Node.js core `crypto.argon2()` / `crypto.argon2Sync()` functions. This probe checks for: |
There was a problem hiding this comment.
| Detect usage of **weak Argon2** parameters with the Node.js core `crypto.argon2()` / `crypto.argon2Sync()` functions. This probe checks for: | |
| Detect usage of **weak Argon2** parameters with the Node.js core `crypto.argon2()` / `crypto.argon2Sync()` functions. | |
| Checks for: |
I don't think we should use the term "probe" since it is an implementation detail of Js-x-ray
| (prop) => prop.key.type === "Identifier" && prop.key.name === "nonce" | ||
| ); | ||
|
|
||
| if (nonce && isLiteral(nonce.value)) { |
There was a problem hiding this comment.
same here we should check for identifier that are hardcoded
| const properties = parameters.properties.filter( | ||
| (prop): prop is ESTree.Property => prop.type === "Property" | ||
| ); | ||
| const memory = extractNumericParam(properties, ["memory"]); |
There was a problem hiding this comment.
i guess a better way to do it would be to extract each properties in one loop then extract the numeric value
| ): number | null { | ||
| for (const prop of properties) { | ||
| if ( | ||
| prop.key.type === "Identifier" && |
There was a problem hiding this comment.
same here, it would be nice to check for identifier that are literal as well
| [7168, 5, 1] | ||
| ]; | ||
|
|
||
| const tracedFunctions = new Set(["crypto.argon2", "crypto.argon2Sync"]); |
There was a problem hiding this comment.
i guess, just creating an array is ok here, you already know that there will be no duplicate
| } | ||
| } | ||
|
|
||
| const nonce = properties.find( |
There was a problem hiding this comment.
same here it should be part of just one loop rather than iterating once again
| if (isWeakArgon2Params(memory, iteration, parallelism)) { | ||
| sourceFile.warnings.push( | ||
| generateWarning("weak-argon2", { | ||
| value: "weak-parameters", |
There was a problem hiding this comment.
in the value it would be also more helpful to add the type of the parameters that is weak:
```value: weak-parameter: ${type}`;
…k-argon2 warningadd weak-argon2 detection probe
2 participants
Adds an optional weak-argon2 probe to detect insecure usage of crypto.argon2() / crypto.argon2Sync().
Detection targets:
The warning is optional and disabled by default.
Partially addresses #506 (Argon2 part)