Skip to content

Commit

Permalink
Add old liftsecurity blog posts to docs/ folder
Browse files Browse the repository at this point in the history
  • Loading branch information
@pdehaan
pdehaan committed Apr 3, 2019
1 parent 008bd88 commit 3c7522c
Show file tree
Hide file tree
Showing 5 changed files with 331 additions and 5 deletions.
10 changes: 5 additions & 5 deletions README.md
View file
Expand Up @@ -43,7 +43,7 @@ npm test

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

More information: https://web.archive.org/web/20170131192028/https://blog.liftsecurity.io/2014/11/03/regular-expression-dos-and-node.js
More information: [Regular Expression DoS and Node.js](docs/regular-expression-dos-and-node.md)

#### `detect-buffer-noassert`

Expand All @@ -55,7 +55,7 @@ From the Node.js API docs: "Setting `noAssert` to true skips validation of the `

Detects 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: https://web.archive.org/web/20140903015907/https://blog.liftsecurity.io/2014/08/19/Avoid-Command-Injection-Node.js
More information: [Avoiding Command Injection in Node.js](docs/avoid-command-injection-node.md)

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

Expand All @@ -73,7 +73,7 @@ More information: http://security.stackexchange.com/questions/94017/what-are-the

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: https://blog.liftsecurity.io/2013/09/07/bypass-connect-csrf-protection-by-abusing
More information: [Bypass Connect CSRF protection by abusing methodOverride Middleware](docs/bypass-connect-csrf-protection-by-abusing.md)

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

Expand All @@ -85,7 +85,7 @@ More information: https://www.owasp.org/index.php/Path_Traversal

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

More information: https://blog.liftsecurity.io/2014/11/03/regular-expression-dos-and-node.js
More information: [Regular Expression DoS and Node.js](docs/regular-expression-dos-and-node.md)

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

Expand All @@ -97,7 +97,7 @@ More information: http://www.bennadel.com/blog/2169-where-does-node-js-and-requi

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

More information: https://web.archive.org/web/20150430062816/https://blog.liftsecurity.io/2015/01/15/the-dangers-of-square-bracket-notation
More information: [The Dangers of Square Bracket Notation](docs/the-dangers-of-square-bracket-notation.md)

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

Expand Down
85 changes: 85 additions & 0 deletions docs/avoid-command-injection-node.md
View file
@@ -0,0 +1,85 @@
# Avoiding Command Injection in Node.js

In this post we are going to learn about the proper way to call a system command using node.js to avoid a common security flaw, command injection.

A call that we often see used, due to it's symplicity is `child_process.exec`. It's got a simple pattern; pass in a command string and it calls you back with an error or the command results.

Here is a very typical way you would call a system command with `child_process.exec.`

```js
child_process.exec('ls', function (err, data) {
console.log(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 concatentation to send data from one system to another you're probably going to have a bad day.

```js
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 devestating effects if the command is including user input.

```sh
[pid 25170] execve("/bin/sh", ["/bin/sh", "-c", "ls -l user input"], [/* 16 vars */]
```
For example, an attacker could use a ; to end the statement and start another one, they could use backticks or $() to run a subcommand. Lots of potential for abuse.
## So how do we do this the right way?
Calls like `spawn` and `execFile` take additional command arguments as an array, are not executed under a shell environment, and do not manipulate the originally intended command to run.
Let's modify our example to use `execFile` and `spawn` and see how the system calls differ, and why it isn't vulnerable to command injection.
### `child_process.execFile`
```js
var child_process = require('child_process');

var path = "."
child_process.execFile('/bin/ls', ['-l', path], function (err, result) {
console.log(result)
});
```
System call that is run
```sh
[pid 25565] execve("/bin/ls", ["/bin/ls", "-l", "."], [/* 16 vars */]
```
### `child_process.spawn`
Similar example using `spawn` instead.
```js
var child_process = require('child_process');

var path = "."
var ls = child_process.spawn('/bin/ls', ['-l', path])
ls.stdout.on('data', function (data) {
console.log(data.toString());
});
```
System call that is run
```sh
[pid 26883] execve("/bin/ls", ["/bin/ls", "-l", "."], [/* 16 vars */
```
When using `spawn` or `execFile`, our target program is the first argument to execve. This means that a user cannot run subcommands in the shell, because <kbd>/bin/ls</kbd> has no idea what to do with backticks or pipes or ;. It's <kbd>/bin/bash</kbd> that is going to be interpreting those commands. It's similar to using parameterized vs string-based SQL queries, if you are familiar with that.
This does however come with a caveat: using `spawn` or `execFile` is not always a safe thing. For example, running <kbd>/bin/find</kbd> with `spawn` or `execFile` and passing user input in directly could still lead to complete system takeover. The <kbd>find</kbd> command has options that allow for arbitrary file read/write.
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.
42 changes: 42 additions & 0 deletions docs/bypass-connect-csrf-protection-by-abusing.md
View file
@@ -0,0 +1,42 @@
# Bypass Connect CSRF protection by abusing methodOverride Middleware

Since our platform isn't setup for advisories that are not specific to a particular module version, but rather a use / configuration of a certain module, we will announce this issue here and get it into the database at a later date.

This issue was found and reported to us by [Luca Carettoni](http://twitter.com/_ikki) (who we consider one of the node security projects core advisers in many areas)

## Affected Component

Connect, methodOverride middleware

### 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".**

As the declaration order of middlewares determines the execution stack in Connect, it is possible to abuse this functionality in order to bypass the standard Connect's anti-CSRF protection.

Considering the following code:

```js
...
app.use express.csrf()
...
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:

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

### Mitigation Factors:

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

Thanks to the same origin policy enforced by modern browsers in XMLHttpRequest and other mechanisms, the exploitability of this issue abusing "x-http-method-override" header is significantly reduced.

There is also an [ESLint plugin](https://github.com/evilpacket/eslint-rules) that you can use to help identify this.
83 changes: 83 additions & 0 deletions docs/regular-expression-dos-and-node.md
View file
@@ -0,0 +1,83 @@
# Regular Expression DoS and Node.js

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.

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.

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

```js
validateEmailFormat: function( string ) {
var emailExpression = /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/;

return emailExpression.test( string );
}
```

With the example above, we can use this test script to show how bad input can impact server responsiveness:

```js
start = process.hrtime();
console.log(validateEmailFormat("baldwin@andyet.net"));
console.log(process.hrtime(start));

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

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

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

Here are the results of running that script:

```sh
true
[ 0, 9694442 ] <- Match on good data takes little time
false
[ 0, 49849962 ] <- Initial bad input baseline
false
[ 0, 55123953 ] <- Added 1 character to the input and you see minimal spike
false
[ 8, 487126563 ] <- Added 12 characters and you see it bumps up significantly
```

One way you can check regular expressions for badness in an automated way is by using a module from [substack](https://twitter.com/substack) called [safe-regex](https://www.npmjs.org/package/safe-regex). It's prone to false positives, however, it can be useful to point to potentially vulnerable regular expressions you would have otherwise missed in your code.

Here is a rule for eslint that you can use to test your JavaScript regular expressions:

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

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

if (nodeType === "RegularExpression") {
if (!safe(nodeValue)) {
context.report(node, "Possible Unsafe Regular Expression");
}
}
}
};
};
```

Additionally, OWASP has a [list of regular expressions](https://www.owasp.org/index.php/OWASP_Validation_Regex_Repository) for common validations that might be useful to you.

As part of our ongoing effort to increase the overall security of the Node.js ecosystem, we have conducted automated analysis of every module on npm. We did identify 56 unique vulnerable regular expressions and over 120 modules containing vulnerable regular expressions. Considering that there are now over 100k modules, the results were not alarming. We're working closely with the maintainers of each module to get the issues resolved, once that's done, advisories will be published to the [npm Security advisories](https://www.npmjs.com/advisories) site.
116 changes: 116 additions & 0 deletions docs/the-dangers-of-square-bracket-notation.md
View file
@@ -0,0 +1,116 @@
# The Dangers of Square Bracket Notation

We are going to be looking at some peculiar and potentially dangerous implications of JavaScript's square bracket notation in this post: where you shouldn't use this style of object access and why, as well how to use it safely when needed.

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.*

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]
```

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.*__

```js
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.

### 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() {
this.name = 'jon';
//An empty user constructor.
};

function handler(userInput) {
var anyVal = 'anyVal'; // This can be any attribute, and does not need to be user controlled.
user[anyVal] = user[userInput[0]](userInput[1]);
}
```

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.

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

handler(userInputStageOne);
handler(userInputStageTwo);
}
```

Let's break it down.

The first time handler is run, it looks something like this:

```js
userInput[0] = 'constructor';
userInput[1] = 'require("child_process").exec(arguments[0],console.log)';

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

And when handler is run a second time:

```js
user.anyVal = user.anyVal('date');
```

What we end up with is this:

![](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?

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?

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.

Another option is to create a whitelist of allowed property names, and filter each user input through a helper function to check before allowing it to be used. This is a great option in situations where you know specifically what property names to allow.

In cases where you don't have a strictly defined data model ( which isn't ideal, but there are cases where it has to be so ) then using the same method as above, but with a blacklist of disallowed properties instead is a valid choice.

If you are using the `--harmony` flag or [io.js](https://iojs.org/), you also have the option of using [ECMAScript 6 direct proxies](http://wiki.ecmascript.org/doku.php?id=harmony:direct_proxies), which can stand in front of your real object ( private API ) and expose a limited subset of the object ( public API ). This is probably the best approach if you are using this pattern, as it is most consistent with typical object oriented programming paradigms.

0 comments on commit 3c7522c

Please sign in to comment.