Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/yummy-llamas-wear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nodesecure/js-x-ray": minor
---

add weak-argon2 detection probe
49 changes: 49 additions & 0 deletions docs/weak-argon2.md
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:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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


- **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
});
```
7 changes: 5 additions & 2 deletions workspaces/js-x-ray/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ Alternatively, you can use `EntryFilesAnalyser` directly for multi-file analysis
type OptionalWarningName =
| "synchronous-io"
| "log-usage"
| "weak-scrypt";
| "weak-scrypt"
| "weak-argon2";

type WarningName =
| "parsing-error"
Expand Down Expand Up @@ -177,14 +178,15 @@ const scanner = new AstAnalyser({

// Or enable specific optional warnings
const scannerSpecific = new AstAnalyser({
optionalWarnings: ["synchronous-io", "log-usage", "weak-scrypt"]
optionalWarnings: ["synchronous-io", "log-usage", "weak-scrypt", "weak-argon2"]
});
```

The following warnings are optional:
- `synchronous-io` - Detects synchronous I/O operations that could impact performance
- `log-usage` - Tracks usage of logging functions (console.log, logger.info, etc.)
- `weak-scrypt` - Detects weak scrypt parameters (low cost, short or hardcoded salt)
- `weak-argon2` - Detects weak Argon2 parameters (wrong algorithm, weak parameters, hardcoded nonce)

### Internationalization (i18n)

Expand Down Expand Up @@ -231,6 +233,7 @@ Click on the warning **name** for detailed documentation and examples.
| [sql-injection](https://github.com/NodeSecure/js-x-ray/blob/master/docs/sql-injection.md) | No | Potential SQL injection vulnerability detected |
| [monkey-patch](https://github.com/NodeSecure/js-x-ray/blob/master/docs/monkey-patch.md) | No | Modification of built-in JavaScript prototype properties |
| [weak-scrypt](https://github.com/NodeSecure/js-x-ray/blob/master/docs/weak-scrypt.md) ⚠️ | **Yes** | Usage of weak scrypt parameters (low cost, short or hardcoded salt) |
| [weak-argon2](https://github.com/NodeSecure/js-x-ray/blob/master/docs/weak-argon2.md) ⚠️ | **Yes** | Usage of weak Argon2 parameters (wrong algorithm, weak parameters, hardcoded nonce) |

#### Information Severity

Expand Down
4 changes: 3 additions & 1 deletion workspaces/js-x-ray/src/ProbeRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import isMonkeyPatch from "./probes/isMonkeyPatch.ts";
import isRandom from "./probes/isRandom.ts";
import isPrototypePollution from "./probes/isPrototypePollution.ts";
import isWeakScrypt from "./probes/isWeakScrypt.ts";
import isWeakArgon2 from "./probes/isWeakArgon2.ts";

import type { TracedIdentifierReport } from "./VariableTracer.ts";
import type { SourceFile } from "./SourceFile.ts";
Expand Down Expand Up @@ -114,7 +115,8 @@ export class ProbeRunner {
"synchronous-io": isSyncIO,
"log-usage": logUsage,
"insecure-random": isRandom,
"weak-scrypt": isWeakScrypt
"weak-scrypt": isWeakScrypt,
"weak-argon2": isWeakArgon2
};

constructor(
Expand Down
3 changes: 2 additions & 1 deletion workspaces/js-x-ray/src/i18n/arabic.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ const sast_warnings = {
sql_injection: "قالب نصي (Template literal) يحتوي على تعبيرات مدرجة في استعلامات SQL (SELECT, INSERT, UPDATE, DELETE) بدون معالجة صحيحة، مما يخلق ثغرات حقن SQL محتملة.",
monkey_patch: "تعديل النماذج الأصلية (native prototypes) أو الكائنات العالمية في وقت التشغيل، مما يؤدي إلى مخاطر أمنية تشمل اختطاف التدفق، والآثار الجانبية العالمية، والإخفاء المحتمل للأنشطة الضارة.",
insecure_random: "استخدام توليد أرقام عشوائية غير آمن باستخدام Math.random(). إن Math.random() ليس آمناً من الناحية التشفيرية ولا ينبغي استخدامه للعمليات الحساسة أمنياً.",
weak_scrypt: "استخدام crypto.scrypt() أو crypto.scryptSync() مع معلمات غير آمنة مثل ملح مضمن في الكود، أو ملح قصير (أقل من 16 بايت)، أو معلمة تكلفة غير كافية (أقل من 16384). هذه التكوينات الضعيفة تُضعف أمان اشتقاق المفاتيح المبني على كلمة المرور."
weak_scrypt: "استخدام crypto.scrypt() أو crypto.scryptSync() مع معلمات غير آمنة مثل ملح مضمن في الكود، أو ملح قصير (أقل من 16 بايت)، أو معلمة تكلفة غير كافية (أقل من 16384). هذه التكوينات الضعيفة تُضعف أمان اشتقاق المفاتيح المبني على كلمة المرور.",
weak_argon2: "استخدام crypto.argon2() أو crypto.argon2Sync() مع معلمات غير آمنة: استخدام argon2d أو argon2i بدلاً من argon2id، أو استخدام nonce (ملح) مضمن في الكود، أو قيم memory/passes/parallelism أقل من الحد الأدنى الموصى به من OWASP. هذه التكوينات الضعيفة تُضعف أمان اشتقاق المفاتيح المبني على كلمة المرور."
};

export default {
Expand Down
3 changes: 2 additions & 1 deletion workspaces/js-x-ray/src/i18n/english.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ const sast_warnings = {
sql_injection: "Template literals with interpolated expressions in SQL queries (SELECT, INSERT, UPDATE, DELETE) without proper parameterization, creating potential SQL injection vulnerabilities.",
monkey_patch: "Modification of native prototypes or global objects at runtime, which introduces security risks including flow hijacking, global side effects, and potential concealment of malicious activities.",
insecure_random: "Usage of insecure random number generation using Math.random(). Math.random() is not cryptographically secure and should not be used for security-sensitive operations.",
weak_scrypt: "Usage of crypto.scrypt() or crypto.scryptSync() with insecure parameters such as hardcoded salt, short salt (less than 16 bytes), or insufficient cost parameter (below 16384). These weak configurations compromise the security of password-based key derivation."
weak_scrypt: "Usage of crypto.scrypt() or crypto.scryptSync() with insecure parameters such as hardcoded salt, short salt (less than 16 bytes), or insufficient cost parameter (below 16384). These weak configurations compromise the security of password-based key derivation.",
weak_argon2: "Usage of crypto.argon2() or crypto.argon2Sync() with insecure parameters: use of argon2d or argon2i instead of argon2id, hardcoded nonce (salt), or memory/passes/parallelism values below OWASP minimum recommendations. These weak configurations compromise the security of password-based key derivation."
};

export default {
Expand Down
3 changes: 2 additions & 1 deletion workspaces/js-x-ray/src/i18n/french.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion workspaces/js-x-ray/src/i18n/turkish.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

142 changes: 142 additions & 0 deletions workspaces/js-x-ray/src/probes/isWeakArgon2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
// 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"]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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" &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 (algorithm && algorithm.type === "Identifier") {
const algorithmName = sourceFile.tracer.literalIdentifiers.get(algorithm.name)?.value;
if (algorithmName && algorithmName !== "argon2id") {
sourceFile.warnings.push(
generateWarning("weak-argon2", {
value: `wrong-algorithm : ${algorithmName}`,
location: node.loc
})
);
}
}

if (isLiteral(algorithm)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 : ${algorithm.value}`,
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"]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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:
```value: weak-parameter: ${type}`;

location: node.loc
})
);
}
}

const nonce = properties.find(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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: {}
};
9 changes: 8 additions & 1 deletion workspaces/js-x-ray/src/warnings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ export type OptionalWarningName =
| "synchronous-io"
| "log-usage"
| "insecure-random"
| "weak-scrypt";
| "weak-scrypt"
| "weak-argon2";

export type WarningName =
| "parsing-error"
Expand All @@ -34,6 +35,7 @@ export type WarningName =
| "monkey-patch"
| "insecure-random"
| "prototype-pollution"
| "weak-argon2"
| OptionalWarningName;

export interface Warning<T = WarningName> {
Expand Down Expand Up @@ -152,6 +154,11 @@ export const warnings = Object.freeze({
i18n: "sast_warnings.weak_scrypt",
severity: "Warning",
experimental: true
},
"weak-argon2": {
i18n: "sast_warnings.weak_argon2",
severity: "Warning",
experimental: true
}
}) satisfies Record<WarningName, Pick<Warning, "experimental" | "i18n" | "severity">>;

Expand Down
Loading
Loading