Never Trust Express

Some Background

Nowadays, a lot of people are leaning towards replacing the typical LAMP/LEMP stack with something more modern. Particuluarly, many people are leaning towards adopting Node.js. While I love that people are adopting new technology and moving away from shudders PHP, there is reasonable skepticism when choosing new technologies because we don’t want vulnerabilities.

So What’s The Problem With Express?

Express is a great library. It has plenty of features, it makes doing practically anything a dead simple task, all while staying reasonably simplistic in design. However, express isn’t totally perfect in design.

For example, when it comes to querystring parsing, Express uses qs. qs is a nice library! It works as expected almost always, has a reasonable degree of “loose” parsing, and is simple for our needs. But, many people don’t know that express uses qs. And therein lies the issue.

You see, qs supports this thing called objects, I’m sure you’ve heard of them. Javascript loves objects, they’re great. But theres one problem with them: type coercion can lead to unexpected behavior when an unexperienced developer is writing Javascript. If you still don’t understand what I mean by objects, let me give an example:

  {
    "name": "John",
    "age": 24,
    "birthdate": "1995-04-03T04:49:58+00:00"
  }

Looks like a great way to express data, right? No need for structs, objects are entirely dynamic, and anything that isn’t defined, well, isn’t defined. But there’s a problem with dynamic objects, they’re dynamic. Being dynamic, this means you have to clutter your code verifying value types at every step this object is passed along to ensure you aren’t using anything incorrectly. It’s pretty simple to do this, but if you even slightly forget about it you will run into a thing called type coercion. In javascript, type coercion can turn strings to numbers, numbers to strings, strings to booleans, etc. This is great for when you want that behavior, but it’s horrible when you don’t expect it.

Specifically, the worst type coercion in my opinion is objectstring. This happens when you, say, log an object to the console, or perform any operation which takes in a string. Under the hood, coercing anything into a string results in this:

  String( obj )

which, in turn, results in this:

  obj.toString()

Typically .toString() on an object just results in [object Object], but this assumes that your object does not have a property named .toString. And here lies the issue. Possibly hundreds of apps have been made entirely unaware to the fact that qs supports objects. Many people may assume querystrings are, well, strings??? But, according to qs, querystrings can contain objects. They’re notated like so:

  /path?parameter=value&parameter2[key]=value

When you parse this, you get the following object:

  {
    "parameter": "value",
    "parameter2": {
      "key": "value"
    }
  }

Being able to input arbitrary objects isn’t always an issue, since most of the time we are dealing in json anyhow, and anything goes there. But when people don’t know that it is possible, they might not bother checking the types of values in their querystring.

But Is This Even Exploitable?

I suppose you could argue that there is a very small exploitation scope for this issue, and I haven’t personally found any RCEs or major vulnerabilities from it. But, in my opinion, the biggest issue here is that express applications output full stacktraces by default. Remember how we talked about coercion earlier? That’s going to be important now.

Let’s assume we have the following querystring:

  /color/submit?name=justyn&favorite_color=red

Here’s what that parses out to:

  {
    "name": "justyn",
    "color": "red"
  }

And here’s what we do on the server:

  console.log(`${req.query.name}'s favorite color is ${req.query.color}`);

With our current payload, there’s no issues running that code. It works entirely as expected. But what happens if we introduce some objects into the mix? Let’s try it.

  /color/submit?name[toString]=break_the_code&favorite_color=red

This parses out as:

  {
    "name": {
      "toString": "break_the_code"
    },
    "favorite_color": "red"
  }

And here’s where the problem comes in. Our code assumes the querystring parameters will either be undefined or strings, but they can be objects. Javascript will try its best to turn that object into a string when we print it out but, unfortunately for us, javascript is going to fail horribly. Here’s what it has to say to our code:

  TypeError: can't convert req.query.name to string

A big nasty exception, which is probably unhandled (at least by us directly). This makes express spill its guts out like this:

    at Function.Utils.sha256 (/var/www/█████.██████.███/src/utils.js:28:50)
    at /var/www/█████.██████.███/src/routes/transactions.js:98:21
    at Layer.handle [as handle_request] (/var/www/█████.██████.███/node_modules/express/lib/router/layer.js:95:5)
    at next (/var/www/█████.██████.███/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/var/www/█████.██████.███/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/var/www/█████.██████.███/node_modules/express/lib/router/layer.js:95:5)
    at /var/www/█████.██████.███/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/var/www/█████.██████.███/node_modules/express/lib/router/index.js:335:12)
    at next (/var/www/█████.██████.███/node_modules/express/lib/router/index.js:275:10)
    at next (/var/www/█████.██████.███/node_modules/express/lib/router/route.js:127:14)
    at /var/www/█████.██████.███/src/routes/submission.js:60:3
    at Layer.handle [as handle_request] (/var/www/█████.██████.███/node_modules/express/lib/router/layer.js:95:5)
    at next (/var/www/█████.██████.███/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/var/www/█████.██████.███/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/var/www/█████.██████.███/node_modules/express/lib/router/layer.js:95:5)
    at /var/www/█████.██████.███/node_modules/express/lib/router/index.js:281:22

And in even worse cases, it may even spit out it’s code:

TypeError: /home/admin/steam/views/auth.ejs:245
    243|                 <div class="OpenID_MainHeader">
    244|                     <div class="logo"><img src="/images/sits_landing.png"  width="114" height="43" border="0"></div>
 >> 245|                     <h1><%= trans.headerauthtext.replace("%site%", site) %></h1>
    246|                     <div>
    247|                         <div style="float:left; padding: 6px 6px 20px 18px">
    248|                             <img src="/images/icon_info.png"  width="26" height="26" border="0" />

Cannot convert object to primitive value
    at String.replace (<anonymous>)
    at eval (eval at compile (/home/admin/steam/node_modules/ejs/lib/ejs.js:618:12), <anonymous>:125:47)
    at returnedFn (/home/admin/steam/node_modules/ejs/lib/ejs.js:653:17)
    at tryHandleCache (/home/admin/steam/node_modules/ejs/lib/ejs.js:251:36)
    at View.exports.renderFile [as engine] (/home/admin/steam/node_modules/ejs/lib/ejs.js:482:10)
    at View.render (/home/admin/steam/node_modules/express/lib/view.js:135:8)
    at tryRender (/home/admin/steam/node_modules/express/lib/application.js:640:10)
    at Function.render (/home/admin/steam/node_modules/express/lib/application.js:592:3)
    at ServerResponse.render (/home/admin/steam/node_modules/express/lib/response.js:1008:7)
    at /home/admin/steam/index.js:63:14

While the output of these may seem harmless, it’s honestly an attack surface that i’d rather not trust. If I can avoid it, I don’t want an attacker knowing line numbers, paths, or even worse, the code itself.

How Can It Be Fixed?

As it stands currently, there is no way to disable objects entirely in qs, at least to my knowledge. You can limit the depth of objects and length of arrays, but that is the extent to which you can change it. In my opinion, the larger issue is with the fact that the web is designed to be dynamic and being dynamic introduces vulnerabilities when used incorrectly.