Skip to content

Commit

Permalink
chore: Clean up source code formatting
Browse files Browse the repository at this point in the history
Fixes #4
Closes #78
  • Loading branch information
nzakas committed Apr 18, 2022
1 parent 0a6631e commit 78292e0
Show file tree
Hide file tree
Showing 38 changed files with 429 additions and 330 deletions.
12 changes: 11 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
{
"extends": "nodesecurity"
"extends": [
"eslint:recommended",
"prettier"
],
"parserOptions": {
"ecmaVersion": "latest"
},
"env": {
"node": true,
"es2020": true
}
}
52 changes: 26 additions & 26 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,38 @@ jobs:
name: Lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
with:
node-version: '16.x'
- name: Install Packages
run: npm install
- name: Lint Files
run: npm run lint
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
with:
node-version: '16.x'

- name: Install Packages
run: npm install

- name: Lint Files
run: npm run lint

test:
name: Test
strategy:
matrix:
os: [ubuntu-latest]
node: [17.x, 16.x, 14.x, 12.x, "12.22.0"]
node: [17.x, 16.x, 14.x, 12.x, '12.22.0']
include:
- os: windows-latest
node: "16.x"
- os: macOS-latest
node: "16.x"
- os: windows-latest
node: '16.x'
- os: macOS-latest
node: '16.x'
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node }}
- name: Install Packages
run: npm install
- name: Test
run: npm test
- uses: actions/checkout@v2

- uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node }}

- name: Install Packages
run: npm install

- name: Test
run: npm test
7 changes: 7 additions & 0 deletions .prettierrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"trailingComma": "es5",
"tabWidth": 2,
"semi": true,
"singleQuote": true,
"printWidth": 180
}
63 changes: 29 additions & 34 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,34 +1,29 @@
1.4.0 / 2017-06-12
==================

* Add recommended ruleset to the usage example
* Removes filenames from error output

1.3.0 / 2017-02-09
==================

* README.md - document detect-disable-mustache-escape rule
* README.md - documentation detect-new-buffer rule
* Fixed crash with `detect-no-csrf-before-method-override` rule.
* Style guide applied to all the code involving the tests
* Removing a repeated test and style changes
* ESLint added to the workflow
* Removed not needed variables
* Fix to a problem with a rule detected implementing the tests
* Test engine with tests for all the rules
* Add additional information to README for each rule

1.2.0 / 2016-01-21
==================

* updated to check for new RegExp too

1.1.0 / 2016-01-06
==================

* adding eslint rule to detect new buffer hotspot

1.0.0 / 2015-11-15
==================

* rules disabled by default
# 1.4.0 / 2017-06-12

- Add recommended ruleset to the usage example
- Removes filenames from error output

# 1.3.0 / 2017-02-09

- README.md - document detect-disable-mustache-escape rule
- README.md - documentation detect-new-buffer rule
- Fixed crash with `detect-no-csrf-before-method-override` rule.
- Style guide applied to all the code involving the tests
- Removing a repeated test and style changes
- ESLint added to the workflow
- Removed not needed variables
- Fix to a problem with a rule detected implementing the tests
- Test engine with tests for all the rules
- Add additional information to README for each rule

# 1.2.0 / 2016-01-21

- updated to check for new RegExp too

# 1.1.0 / 2016-01-06

- adding eslint rule to detect new buffer hotspot

# 1.0.0 / 2015-11-15

- rules disabled by default
15 changes: 8 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@ Add the following to your `.eslintrc` file:
]
```


## Developer guide

- Use [GitHub pull requests](https://help.github.com/articles/using-pull-requests).
- Conventions:
- We use our [custom ESLint setup](https://github.com/nodesecurity/eslint-config-nodesecurity).
- Please implement a test for each new rule and use this command to be sure the new code respects the style guide and the tests keep passing:
```sh
npm run-script cont-int
```
- We use our [custom ESLint setup](https://github.com/nodesecurity/eslint-config-nodesecurity).
- Please implement a test for each new rule and use this command to be sure the new code respects the style guide and the tests keep passing:

```sh
npm run-script cont-int
```

### Tests

```sh
npm test
```
Expand Down Expand Up @@ -58,7 +59,7 @@ More information: [Avoiding Command Injection in Node.js](docs/avoid-command-inj

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

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

Expand Down
18 changes: 9 additions & 9 deletions docs/avoid-command-injection-node.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ child_process.exec('ls', function (err, data) {
What happens though when you need to start getting user input for arguments into your command? The obvious solution is to take the user input and build your command out using string concatenation. But here's something I've learned over the years: When you use string concatenation to send data from one system to another you're probably going to have a bad day.

```js
var path = "user input";
var path = 'user input';
child_process.exec('ls -l ' + path, function (err, data) {
console.log(data);
});
```

## Why is string concatenation a problem?

Well, because under the hood, `child_process.exec` makes a call to execute <kbd>/bin/sh</kbd> rather than the target program. The command that was sent just gets passed along as a shell command in the newly spawned <kbd>/bin/sh</kbd> process. `child_process.exec` has a misleading name - it's a bash interpreter, not a program launcher. And that means that all shell metacharacters can have devastating effects if the command is including user input.
Well, because under the hood, `child_process.exec` makes a call to execute <kbd>/bin/sh</kbd> rather than the target program. The command that was sent just gets passed along as a shell command in the newly spawned <kbd>/bin/sh</kbd> process. `child_process.exec` has a misleading name - it's a bash interpreter, not a program launcher. And that means that all shell metacharacters can have devastating effects if the command is including user input.

```sh
[pid 25170] execve("/bin/sh", ["/bin/sh", "-c", "ls -l user input"], [/* 16 vars */]
Expand All @@ -42,9 +42,9 @@ Let's modify our example to use `execFile` and `spawn` and see how the system ca
```js
var child_process = require('child_process');

var path = "."
var path = '.';
child_process.execFile('/bin/ls', ['-l', path], function (err, result) {
console.log(result)
console.log(result);
});
```
Expand All @@ -61,8 +61,8 @@ Similar example using `spawn` instead.
```js
var child_process = require('child_process');

var path = "."
var ls = child_process.spawn('/bin/ls', ['-l', path])
var path = '.';
var ls = child_process.spawn('/bin/ls', ['-l', path]);
ls.stdout.on('data', function (data) {
console.log(data.toString());
});
Expand All @@ -80,6 +80,6 @@ This does however come with a caveat: using `spawn` or `execFile` is not always
So, here's the collective guidance for running system commands from node.js:
* Avoid using `child_process.exec`, and never use it if the command contains any input that changes based on user input.
* Try to avoid letting users pass in options to commands if possible. Typically values are okay when using spawn or execfile, but selecting options via a user controlled string is a bad idea.
* If you must allow for user controlled options, look at the options for the command extensively, determine which options are safe, and whitelist only those options.
- Avoid using `child_process.exec`, and never use it if the command contains any input that changes based on user input.
- Try to avoid letting users pass in options to commands if possible. Typically values are okay when using spawn or execfile, but selecting options via a user controlled string is a bad idea.
- If you must allow for user controlled options, look at the options for the command extensively, determine which options are safe, and whitelist only those options.
28 changes: 14 additions & 14 deletions docs/regular-expression-dos-and-node.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

Imagine you are trying to buy a ticket to your favorite JavaScript conference, and instead of getting the ticket page, you instead get `500 Internal Server Error`. For some reason the site is down. You can't do the thing that you want to do most and the conference is losing out on your purchase, all because the application is unavailable.

Availability is not often treated as a security problem, which it is, and it's impacts are immediate, and deeply felt.
Availability is not often treated as a security problem, which it is, and it's impacts are immediate, and deeply felt.

The attack surface for Node.js in regards to loss of availability is quite large, as we are dealing with a single event loop. If an attacker can control and block that event loop, then nothing else gets done.

There are many ways to block the event loop, one way an attacker can do that is with [Regular Expression Denial of Service (ReDoS)](https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS).

If user provided input finds it's way into a regular expression, or a regular expression is designed with certain attributes, such as grouping with repetition, you can find yourself in a vulnerable position, as the regular expression match could take a long time to process. [OWASP](https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS) has a deeper explanation of why this occurs.
If user provided input finds it's way into a regular expression, or a regular expression is designed with certain attributes, such as grouping with repetition, you can find yourself in a vulnerable position, as the regular expression match could take a long time to process. [OWASP](https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS) has a deeper explanation of why this occurs.

Let's look at an vulnerable example. Below we are attempting the common task of validating an email address on the server.

Expand All @@ -24,19 +24,19 @@ With the example above, we can use this test script to show how bad input can im

```js
start = process.hrtime();
console.log(validateEmailFormat("[email protected]"));
console.log(validateEmailFormat('[email protected]'));
console.log(process.hrtime(start));

start = process.hrtime();
console.log(validateEmailFormat("jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.5555555555555555555555555555555555555555{"));
console.log(validateEmailFormat('jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.5555555555555555555555555555555555555555{'));
console.log(process.hrtime(start));

start = process.hrtime();
console.log(validateEmailFormat("jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.55555555555555555555555555555555555555555{"));
console.log(validateEmailFormat('jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.55555555555555555555555555555555555555555{'));
console.log(process.hrtime(start));

start = process.hrtime();
console.log(validateEmailFormat("jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.555555555555555555555555555555555555555555555555555555{"));
console.log(validateEmailFormat('jjjjjjjjjjjjjjjjjjjjjjjjjjjj@ccccccccccccccccccccccccccccc.555555555555555555555555555555555555555555555555555555{'));
console.log(process.hrtime(start));
```

Expand All @@ -59,21 +59,21 @@ Here is a rule for eslint that you can use to test your JavaScript regular expre

```js
var safe = require('safe-regex');
module.exports = function(context) {
"use strict";
module.exports = function (context) {
'use strict';

return {
"Literal": function(node) {
Literal: function (node) {
var token = context.getTokens(node)[0],
nodeType = token.type,
nodeValue = token.value;
nodeType = token.type,
nodeValue = token.value;

if (nodeType === "RegularExpression") {
if (nodeType === 'RegularExpression') {
if (!safe(nodeValue)) {
context.report(node, "Possible Unsafe Regular Expression");
context.report(node, 'Possible Unsafe Regular Expression');
}
}
}
},
};
};
```
Expand Down
43 changes: 17 additions & 26 deletions docs/the-dangers-of-square-bracket-notation.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,33 @@ We are going to be looking at some peculiar and potentially dangerous implicatio

Square bracket notation for objects in JavaScript provides a very convenient way to dynamically access a specific property or method based on the contents of a variable. The end result of this feature is something that is very similar to Ruby's Mass Assignment: Given an object, you are able to dynamically assign and retrieve properties of this object without specifying this property should be accessible.

*Note: These examples are simple, and seemingly obvious - we will take a look at that later. For now, disregard the practicality of the examples and focus on the dangerous patterns that they reveal.*
_Note: These examples are simple, and seemingly obvious - we will take a look at that later. For now, disregard the practicality of the examples and focus on the dangerous patterns that they reveal._

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.

```js
exampleClass[userInput[1]] = userInput[2]
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',
'{}'
]
exampleClass[userInput[1]] = userInput[2]
userInput = ['constructor', '{}'];
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.
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.

```js
var user = function() {
var user = function () {
this.name = 'jon';
//An empty user constructor.
};
Expand All @@ -44,24 +41,18 @@ 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.
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:

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

```js
function exploit(cmd){
var userInputStageOne = [
'constructor',
'require("child_process").exec(arguments[0],console.log)'
];
var userInputStageTwo = [
'anyVal',
cmd
];

handler(userInputStageOne);
function exploit(cmd) {
var userInputStageOne = ['constructor', 'require("child_process").exec(arguments[0],console.log)'];
var userInputStageTwo = ['anyVal', cmd];

handler(userInputStageOne);
handler(userInputStageTwo);
}
```
Expand All @@ -80,8 +71,8 @@ user['anyVal'] = user['constructor'](userInput[1]);
Executing this code creates a function containing the payload, and assigns it to `user['anyVal']`:

```js
user['anyVal'] = function() {
require("child_process").exec(arguments[0],console.log)
user['anyVal'] = function () {
require('child_process').exec(arguments[0], console.log);
};
```

Expand Down
Loading

0 comments on commit 78292e0

Please sign in to comment.