JS: Model Nest.js

[asgerf]

Adds a model of Nest.js, and makes some big changes to the Express model to make it parts of it reusable by the Nest model. Also does a few drive-by changes found by investigating data flows in Nest.js applications.

Quick overview of Nest

Nest.js uses decorators to mark methods as route handlers, and individual parameters as request inputs, for example:

@Get('foo/:x') // responds to GET requests on this path
foo(
  @Param('x') x, // path parameter 'x' is provided here
  @Query('y') y) { // query parameter 'y' is provided here
  ....
}

Nest.js allows accessing the underlying request/response objects, which by default are from Express:

@Get()
foo(
  @Req() req, // express Request objcet
  @Res() res) { // express Response object
   ...
}

If a string is returned from a route handler, it is sent back as the response and interpreted as HTML unless otherwise specified in the response object:

@Get('greeting')
hello(@Query('name') name) {
  return `<b>Hello ${name}</b>`; // XSS vulnerability
}

Changes to Express

To model @Req() and @Res() I made Express::RequestSource and Express::ResponseSource public for extension.

However, this alone would not work, as Express::RequestInputAccess expected an associated Express::RouteHandler, which we don't have. So I've refactored some of these classes to rely less on Express::RouteHandler as well as the old-school RequestExpr.

Reverted changes

I also changed RequestInputAccess to refer to req.query and req.params instead of req.query.x and req.params.x. This leads to more results in general, as an expression like _.merge({}, req.query) is actually tainted now. This part caused a huge diff in our test suite results, but I don't think that's a good reason to hold back on this change, as something had to be done about it eventually.

A more severe ramification of this change is that data-flow configurations which don't step out of properties will miss a lot of flow from req.query and req.params.

Class validation

Nest.js plays nice with the class-validator package in order to deserialize user input, while validating its structure.

class MyData {
  @IsNumber()
  x: number;
}

@Get()
@UsePipes(new ValidationPipe())
foo(@Query('x') data: MyData) {
  return `<b>${data.x}</b>`; // Safe - data.x is a number
}

Like Angular, Nest uses metadata emitted by the TypeScript compiler to access the type annotation on the parameter.

This relies on the ValidationPipe being installed. I surveyed some popular Nest.js apps and it's heavily used (see breakdown in internal issue). At the same time, many decorators are not actually sanitizing, e.g. @IsString() does not sanitize the field for the purpose of general taint tracking.

Due to the popularity of ValidationPipe we'd lose a lot of coverage if we didn't treat schema-validated inputs as sources, so instead, we treat them as sources, but don't propagate taint through reads of a field with a sanitizing decorator.

Performance

I ran into some performance issues that were fixed by caching ClassNode::Range, which seemed like a good idea anyway. It resulted in a general speed-up of about 4%.

Evaluations (internal links)

• On 25 Nest.js applications finds 60 new alerts, and close to 1000 new sources and 100 sinks.
• On default slugs on an earlier version of the PR we get a nice speed-up as a result of caching ClassNode::Range.
• On default slugs with the current version is still running, but so far we seem to get similar results.

[asgerf]

I also changed RequestInputAccess to refer to req.query and req.params instead of req.query.x and req.params.x.

I'm starting to regret including this change in the PR. I've started another evaluation without that change.

[esbena]

Generally LGTM, and amazing new results. Well done!

I dislike the new fact that we now effectively have sanitizers that prevent the untrusted sources from even appearing in the first place because they are "pre-sanitized". It makes sense given our documentation, implementation, and even UI, but I feel that it could be confusing very for users when they try to debug a lack of flow. It is simply not clear to the user if our Nest.js model is incomplete, or if the expected untrusted input is pre-sanitized.

One imperfect way to address this to let these pre-sanitized untrusted inputs exist, but not to let flow propagate out of them. I do prefer the current solution though. So I am in favor of merging as is.

I have left a few comments in parentheses, I'll create issues for them when we merge this unless you want to address them immediately.

[asgerf]

I dislike the new fact that we now effectively have sanitizers that prevent the untrusted sources from even appearing in the first place because they are "pre-sanitized". It makes sense given our documentation, implementation, and even UI, but I feel that it could be confusing very for users when they try to debug a lack of flow. It is simply not clear to the user if our Nest.js model is incomplete, or if the expected untrusted input is pre-sanitized.

Yeah we're definitely on the same page here - some queries might care about user-controlled numeric inputs, for example, but we don't have a concept to expose such inputs at the moment. But just to be clear, the inputs whose type is a class with sanitizing field decorators are still seen as remote flow sources; we use sanitizers to block flow out of the sanitized fields.

[esbena]

But just to be clear, the inputs whose type is a class with sanitizing field decorators are still seen as remote flow sources; we use sanitizers to block flow out of the sanitized fields.

I see. Thanks for clarifying.