Skip to content

Commit

Permalink
docs: automate docs with eslint-doc-generator (#101)
Browse files Browse the repository at this point in the history
  • Loading branch information
bmish authored Dec 14, 2022
1 parent 657921a commit 263bed9
Show file tree
Hide file tree
Showing 35 changed files with 2,795 additions and 198 deletions.
9 changes: 9 additions & 0 deletions .eslint-doc-generatorrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const { format } = require('prettier');
const { prettier: prettierRC } = require('./.prettierrc.json');

/** @type {import('eslint-doc-generator').GenerateOptions} */
const config = {
postprocess: (doc) => format(doc, { ...prettierRC, parser: 'markdown' }),
};

module.exports = config;
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
},
"rules": {
"eslint-plugin/prefer-message-ids": "off", // TODO: enable
"eslint-plugin/require-meta-docs-description": ["error", { "pattern": "^(Detects|Enforces|Requires|Disallows) .+\\.$" }],
"eslint-plugin/require-meta-docs-url": [
"error",
{
"pattern":
"https://github.com/nodesecurity/eslint-plugin-security#{{name}}",
"https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/rules/{{name}}.md",
},
],
"eslint-plugin/require-meta-schema": "off", // TODO: enable
Expand Down
4 changes: 4 additions & 0 deletions .markdownlint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"line-length": false,
"no-inline-html": { "allowed_elements": ["kbd"]}
}
3 changes: 3 additions & 0 deletions .markdownlintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
CHANGELOG.md
LICENSE
node_modules
117 changes: 36 additions & 81 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,19 @@ ESLint rules for Node Security

This project will help identify potential security hotspots, but finds a lot of false positives which need triage by a human.

### Installation
## Installation

`npm install --save-dev eslint-plugin-security` or `yarn add eslint-plugin-security --dev`
```sh
npm install --save-dev eslint-plugin-security
```

or

```sh
yarn add --dev eslint-plugin-security
```

### Usage
## Usage

Add the following to your `.eslintrc` file:

Expand All @@ -31,86 +39,33 @@ Add the following to your `.eslintrc` file:
npm run-script cont-int
```

### Tests
## Tests

```sh
npm test
```

### Rules

#### `detect-unsafe-regex`

Locates potentially unsafe regular expressions, which may take a very long time to run, blocking the event loop.

More information: [Regular Expression DoS and Node.js](docs/regular-expression-dos-and-node.md)

#### `detect-buffer-noassert`

Detect calls to [`buffer`](https://nodejs.org/api/buffer.html) with `noAssert` flag set.

From the Node.js API docs: "Setting `noAssert` to true skips validation of the `offset`. This allows the `offset` to be beyond the end of the `Buffer`."

#### `detect-child-process`

Detect instances of [`child_process`](https://nodejs.org/api/child_process.html) & non-literal [`exec()`](https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback)

More information: [Avoiding Command Injection in Node.js](docs/avoid-command-injection-node.md)

#### `detect-disable-mustache-escape`

Detects `object.escapeMarkup = false`, which can be used with some template engines to disable escaping of HTML entities. This can lead to Cross-Site Scripting (XSS) vulnerabilities.

More information: [OWASP XSS](<https://www.owasp.org/index.php/Cross-site_Scripting_(XSS)>)

#### `detect-eval-with-expression`

Detects `eval(variable)` which can allow an attacker to run arbitrary code inside your process.

More information: [What are the security issues with eval in JavaScript?](http://security.stackexchange.com/questions/94017/what-are-the-security-issues-with-eval-in-javascript)

#### `detect-no-csrf-before-method-override`

Detects Express `csrf` middleware setup before `method-override` middleware. This can allow `GET` requests (which are not checked by `csrf`) to turn into `POST` requests later.

More information: [Bypass Connect CSRF protection by abusing methodOverride Middleware](docs/bypass-connect-csrf-protection-by-abusing.md)

#### `detect-non-literal-fs-filename`

Detects variable in filename argument of `fs` calls, which might allow an attacker to access anything on your system.

More information: [OWASP Path Traversal](https://www.owasp.org/index.php/Path_Traversal)

#### `detect-non-literal-regexp`

Detects `RegExp(variable)`, which might allow an attacker to DOS your server with a long-running regular expression.

More information: [Regular Expression DoS and Node.js](docs/regular-expression-dos-and-node.md)

#### `detect-non-literal-require`

Detects `require(variable)`, which might allow an attacker to load and run arbitrary code, or access arbitrary files on disk.

More information: [Where does Node.js and require look for modules?](http://www.bennadel.com/blog/2169-where-does-node-js-and-require-look-for-modules.htm)

#### `detect-object-injection`

Detects `variable[key]` as a left- or right-hand assignment operand.

More information: [The Dangers of Square Bracket Notation](docs/the-dangers-of-square-bracket-notation.md)

#### `detect-possible-timing-attacks`

Detects insecure comparisons (`==`, `!=`, `!==` and `===`), which check input sequentially.

More information: [A lesson in timing attacks](https://codahale.com/a-lesson-in-timing-attacks/)

#### `detect-pseudoRandomBytes`

Detects if `pseudoRandomBytes()` is in use, which might not give you the randomness you need and expect.

More information: [Randombytes vs pseudorandombytes](http://stackoverflow.com/questions/18130254/randombytes-vs-pseudorandombytes)

#### `detect-new-buffer`

Detect instances of new Buffer(argument) where argument is any non-literal value.
## Rules

<!-- begin auto-generated rules list -->

⚠️ Configurations set to warn in.\
✅ Set in the `recommended` configuration.

| Name                                  | Description | ⚠️ |
| :------------------------------------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------- | :-- |
| [detect-buffer-noassert](docs/rules/detect-buffer-noassert.md) | Detects calls to "buffer" with "noAssert" flag set. ||
| [detect-child-process](docs/rules/detect-child-process.md) | Detects instances of "child_process" & non-literal "exec()" calls. ||
| [detect-disable-mustache-escape](docs/rules/detect-disable-mustache-escape.md) | Detects "object.escapeMarkup = false", which can be used with some template engines to disable escaping of HTML entities. ||
| [detect-eval-with-expression](docs/rules/detect-eval-with-expression.md) | Detects "eval(variable)" which can allow an attacker to run arbitrary code inside your process. ||
| [detect-new-buffer](docs/rules/detect-new-buffer.md) | Detects instances of new Buffer(argument) where argument is any non-literal value. ||
| [detect-no-csrf-before-method-override](docs/rules/detect-no-csrf-before-method-override.md) | Detects Express "csrf" middleware setup before "method-override" middleware. ||
| [detect-non-literal-fs-filename](docs/rules/detect-non-literal-fs-filename.md) | Detects variable in filename argument of "fs" calls, which might allow an attacker to access anything on your system. ||
| [detect-non-literal-regexp](docs/rules/detect-non-literal-regexp.md) | Detects "RegExp(variable)", which might allow an attacker to DOS your server with a long-running regular expression. ||
| [detect-non-literal-require](docs/rules/detect-non-literal-require.md) | Detects "require(variable)", which might allow an attacker to load and run arbitrary code, or access arbitrary files on disk. ||
| [detect-object-injection](docs/rules/detect-object-injection.md) | Detects "variable[key]" as a left- or right-hand assignment operand. ||
| [detect-possible-timing-attacks](docs/rules/detect-possible-timing-attacks.md) | Detects insecure comparisons (`==`, `!=`, `!==` and `===`), which check input sequentially. ||
| [detect-pseudoRandomBytes](docs/rules/detect-pseudoRandomBytes.md) | Detects if "pseudoRandomBytes()" is in use, which might not give you the randomness you need and expect. ||
| [detect-unsafe-regex](docs/rules/detect-unsafe-regex.md) | Detects potentially unsafe regular expressions, which may take a very long time to run, blocking the event loop. ||

<!-- end auto-generated rules list -->
6 changes: 3 additions & 3 deletions docs/bypass-connect-csrf-protection-by-abusing.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ This issue was found and reported to us by [Luca Carettoni](http://twitter.com/_

Connect, methodOverride middleware

### Description:
### Description

**Connect's "methodOverride" middleware allows an HTTP request to override the method of the request with the value of the "\_method" post key or with the header "x-http-method-override".**

Expand All @@ -25,15 +25,15 @@ app.use express.methodOverride()

Connect's CSRF middleware does not check csrf tokens in case of idempotent verbs (GET/HEAD/OPTIONS, see lib/middleware/csrf.js). As a result, it is possible to bypass this security control by sending a GET request with a POST MethodOverride header or key.

### Example:
### Example

```sh
GET / HTTP/1.1
[..]
_method=POST
```

### Mitigation Factors:
### Mitigation Factors

Disable methodOverride or make sure that it takes precedence over other middleware declarations.

Expand Down
9 changes: 9 additions & 0 deletions docs/rules/detect-buffer-noassert.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Detects calls to "buffer" with "noAssert" flag set (`security/detect-buffer-noassert`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

Detect calls to [`buffer`](https://nodejs.org/api/buffer.html) with `noAssert` flag set.

From the Node.js API docs: "Setting `noAssert` to true skips validation of the `offset`. This allows the `offset` to be beyond the end of the `Buffer`."
9 changes: 9 additions & 0 deletions docs/rules/detect-child-process.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Detects instances of "child_process" & non-literal "exec()" calls (`security/detect-child-process`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

Detect instances of [`child_process`](https://nodejs.org/api/child_process.html) & non-literal [`exec()`](https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback)

More information: [Avoiding Command Injection in Node.js](../avoid-command-injection-node.md)
9 changes: 9 additions & 0 deletions docs/rules/detect-disable-mustache-escape.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Detects "object.escapeMarkup = false", which can be used with some template engines to disable escaping of HTML entities (`security/detect-disable-mustache-escape`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

This can lead to Cross-Site Scripting (XSS) vulnerabilities.

More information: [OWASP XSS](<https://www.owasp.org/index.php/Cross-site_Scripting_(XSS)>)
7 changes: 7 additions & 0 deletions docs/rules/detect-eval-with-expression.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects "eval(variable)" which can allow an attacker to run arbitrary code inside your process (`security/detect-eval-with-expression`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [What are the security issues with eval in JavaScript?](http://security.stackexchange.com/questions/94017/what-are-the-security-issues-with-eval-in-javascript)
5 changes: 5 additions & 0 deletions docs/rules/detect-new-buffer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Detects instances of new Buffer(argument) where argument is any non-literal value (`security/detect-new-buffer`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->
9 changes: 9 additions & 0 deletions docs/rules/detect-no-csrf-before-method-override.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Detects Express "csrf" middleware setup before "method-override" middleware (`security/detect-no-csrf-before-method-override`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

This can allow `GET` requests (which are not checked by `csrf`) to turn into `POST` requests later.

More information: [Bypass Connect CSRF protection by abusing methodOverride Middleware](../bypass-connect-csrf-protection-by-abusing.md)
7 changes: 7 additions & 0 deletions docs/rules/detect-non-literal-fs-filename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects variable in filename argument of "fs" calls, which might allow an attacker to access anything on your system (`security/detect-non-literal-fs-filename`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [OWASP Path Traversal](https://www.owasp.org/index.php/Path_Traversal)
7 changes: 7 additions & 0 deletions docs/rules/detect-non-literal-regexp.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects "RegExp(variable)", which might allow an attacker to DOS your server with a long-running regular expression (`security/detect-non-literal-regexp`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [Regular Expression DoS and Node.js](../regular-expression-dos-and-node.md)
7 changes: 7 additions & 0 deletions docs/rules/detect-non-literal-require.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects "require(variable)", which might allow an attacker to load and run arbitrary code, or access arbitrary files on disk (`security/detect-non-literal-require`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [Where does Node.js and require look for modules?](http://www.bennadel.com/blog/2169-where-does-node-js-and-require-look-for-modules.htm)
7 changes: 7 additions & 0 deletions docs/rules/detect-object-injection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects "variable[key]" as a left- or right-hand assignment operand (`security/detect-object-injection`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [The Dangers of Square Bracket Notation](../the-dangers-of-square-bracket-notation.md)
5 changes: 5 additions & 0 deletions docs/rules/detect-possible-timing-attacks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Detects insecure comparisons (`==`, `!=`, `!==` and `===`), which check input sequentially (`security/detect-possible-timing-attacks`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->
5 changes: 5 additions & 0 deletions docs/rules/detect-pseudoRandomBytes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Detects if "pseudoRandomBytes()" is in use, which might not give you the randomness you need and expect (`security/detect-pseudoRandomBytes`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->
7 changes: 7 additions & 0 deletions docs/rules/detect-unsafe-regex.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Detects potentially unsafe regular expressions, which may take a very long time to run, blocking the event loop (`security/detect-unsafe-regex`)

⚠️ This rule _warns_ in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

More information: [Regular Expression DoS and Node.js](../regular-expression-dos-and-node.md)
14 changes: 7 additions & 7 deletions docs/the-dangers-of-square-bracket-notation.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ _Note: These examples are simple, and seemingly obvious - we will take a look at

Let's take a look at why this could be a problem.

### Issue #1: Bracket object notation with user input grants access to every property available on the object.
## Issue #1: Bracket object notation with user input grants access to every property available on the object

```js
exampleClass[userInput[1]] = userInput[2];
```

I won't spend much time here, as I believe this is fairly well known. If exampleClass contains a sensitive property, the above code will allow it to be edited.

### Issue #2: Bracket object notation with user input grants access to every property available on the object, **_including prototypes._**
## Issue #2: Bracket object notation with user input grants access to every property available on the object, **_including prototypes._**

```js
userInput = ['constructor', '{}'];
Expand All @@ -25,7 +25,7 @@ exampleClass[userInput[1]] = userInput[2];

This looks pretty innocuous, even if it is an uncommon pattern. The problem here is that we can access or overwrite prototypes such as `constructor` or `__defineGetter__`, which may be used later on. The most likely outcome of this scenario would be an application crash, when a string is attempted to be called as a function.

### Issue #3: Bracket object notation with user input grants access to every property available on the object, including prototypes, **_which can lead to Remote Code Execution._**
## Issue #3: Bracket object notation with user input grants access to every property available on the object, including prototypes, **_which can lead to Remote Code Execution._**

Now here's where things get really dangerous. It's also where example code gets really implausible - bear with me.

Expand All @@ -43,7 +43,7 @@ function handler(userInput) {

In the previous section, I mentioned that constructor can be accessed from square brackets. In this case, since we are dealing with a function, the constructor we get back is the `Function` Constructor, which compiles a string of code into a function.

### Exploitation:
## Exploitation

In order to exploit the above code, we need a two stage exploit function.

Expand Down Expand Up @@ -84,19 +84,19 @@ user.anyVal = user.anyVal('date');

What we end up with is this:

![](https://cldup.com/lR_Xp0PwU9.png)
![Exploiting date screenshot](https://cldup.com/lR_Xp0PwU9.png)

Remote Code Execution. The biggest problem here is that there is very little indication in the code that this is what is going on. With something so serious, method calls tend to be very explicit - eval, child_process, etc. It's pretty difficult in node to accidentally introduce one of those into your application. Here though, without having either deep knowledge of JavaScript builtins or having done previous research, it is very easy to accidentally introduce this into your application.

### Isn't this so obscure that it doesn't matter a whole lot?
## Isn't this so obscure that it doesn't matter a whole lot?

Well, yes and no. Is this particular vector a widespread problem? No, because current JavaScript style guides don't advocate programming this way. Might it become a widespread problem in the future? Absolutely. This pattern is avoided because it isn't common, and therefore not learned and taken up as habit, not because it's a known insecure pattern.

Yes, we are talking about some fairly extreme edge cases, but don't make the assumption that your code doesn't have problems because of that - I have seen this issue in production code with some regularity. And, for the majority of node developers, a large portion of application code was not written by them, but rather included through required modules which may contain peculiar flaws like this one.

Edge cases are uncommon, but because they are uncommon the problems with them are not well known, and they frequently go un-noticed during code review. If the code works, these types of problems tend to disappear. If the code works, and the problems are buried in a module nested n-levels deep, it's likely it won't be found until it causes problems, and by then it's too late. A blind require is essentially running untrusted code in your application. Be [aware of what you require.](https://requiresafe.com)

### How do I fix it?
## How do I fix it?

The most direct fix here is going to be to **avoid the use of user input in property name fields**. This isn't reasonable in all circumstances, however, and there should be a way to safely use core language features.

Expand Down
Loading

0 comments on commit 263bed9

Please sign in to comment.