feat(js-x-ray): add weak-argon2 detection probe - #699
Conversation
🦋 Changeset detectedLatest commit: e274bab 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 |
a13ddb8 to
1aae4de
Compare
| } | ||
|
|
||
| if (nonce !== null) { | ||
| sourceFile.warnings.push( |
There was a problem hiding this comment.
In the case when we generate multiple warning for the same function call it would be better to generate only one aggregated warning a bit like we do in the finalize of the probe data-exfiltration
There was a problem hiding this comment.
Thanks! I will check the probe!
| | 9216 (9 MiB) | 4 | | | ||
| | 7168 (7 MiB) | 5 | | | ||
|
|
||
| Argon2i is data-independent and therefore weaker against time-memory trade-off attacks, which additional passes mitigate — see [RFC 9106 section 7.3](https://www.rfc-editor.org/rfc/rfc9106.html#section-7.3). The two lowest-pass rows are consequently unavailable to it, so `argon2i` with `passes` below 3 is reported regardless of how much memory is allocated. |
There was a problem hiding this comment.
should we really accepts argon2i when passes >= 3 ? @clemgbld
There was a problem hiding this comment.
idk i feel like we should only accept what is in the recommendation t=2 2 GBI and t=3 64 MIB
https://www.rfc-editor.org/rfc/rfc9106.html#name-recommendations
WDYT ?
There was a problem hiding this comment.
tha's fine, i thought to a more defensive approach to not allow this argon2 variant at all. ok thanks
There was a problem hiding this comment.
i think both RFC recommendations are for Argon2id, and they don't say much about argon2i. Honestly, I'm not sure what's the better one. Keeping the current passes >= 3, or flagging argon2i as weak-algorithm like argon2d. The only thing I'd avoid is replacing the OWASP table with RFC values, cuz that would raise the minimum memory to 64MiB, and isWeakScrypt uses the same OWASP table. @ErwanRaulo @clemgbld wdyt?
| * TemplateLiteral assignments are ignored because the tracer stores them with | ||
| * their interpolations replaced by placeholders. | ||
| */ | ||
| function resolveString( |
There was a problem hiding this comment.
please rebase on master, this function is now a shared helper.
There was a problem hiding this comment.
but add this limitation fix :
return literal?.type === "Literal" ? literal.value : null;
a test is also missing for this template literal guard.
| * Read a number out of a node, following identifiers assigned a numeric | ||
| * literal. The tracer stores every literal as a string, hence the conversion. | ||
| */ | ||
| function resolveNumber( |
There was a problem hiding this comment.
same, this function is now a shared helper on master
| const { tracer } = sourceFile; | ||
|
|
||
| const algorithm = resolveString(node.arguments.at(0), tracer); | ||
| if (algorithm === null) { |
There was a problem hiding this comment.
do have to make weak param detection dependent on algothm ? if yes, we lose warnings for algorithm variants.
There was a problem hiding this comment.
do have to make weak param detection dependent on algothm ? if yes, we lose warnings for algorithm variants.
Agree, I'll check the params and algorithm independently
| * @description | ||
| * Resolves a string literal, or an identifier tracked back to a string literal assignment | ||
| * Resolves a string literal, or an identifier tracked back to a string literal assignment. | ||
| * If the identifier is tracked back to a template literal, it will return null. |
There was a problem hiding this comment.
- Resolves a string literal, or an identifier tracked back to a string literal assignment or a template literal.
3 participants
Adds an optional weak-argon2 probe to detect insecure usage of crypto.argon2() and crypto.argon2Sync().
Detection targets:
proof-of-work applications with no threats from side-channel timing attacks; §4 recommends
Argon2id when in doubt
Argon2i)"; RFC 9106 §7.3 notes that 3 passes is almost optimal for Argon2i
RECOMMENDED for password hashing
Identifiers assigned a literal are resolved through the
VariableTracer, soconst algo = "argon2d"; crypto.argon2(algo, ...)is reported as well.Parameters that cannot be resolved statically (variables, spread elements, keys computed from
variables) are skipped rather than reported.
parallelismis not checked, since raising it doesnot reduce the total memory cost.
The warning is optional and disabled by default.
Partially addresses #506 (argon2 part)