Security issue: Don't read the host of the application from the HTTP header

[barbogast]

mean.js reads the host of the application from the HTTP header and stores it in req.locals (see https://github.com/meanjs/mean/blob/master/config/lib/express.js#L43). This value is then used for example to generate the password reset link: https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.password.server.controller.js#L62

The HTTP host header field can be spoofed by an attacker. This could be used for example to let the server generate a password reset email with an url which contains the token but points to a completely different domain. More details: http://www.skeletonscribe.net/2013/05/practical-http-host-header-attacks.html

We solved this problem by explicitly specifying the domain of the app in the config and use this value whenever we want to generate urls.

[lirantal]

@Koblaid the code https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.password.server.controller.js#L62 is simply creating the password reset URL, at that point it's not sent to anyone and it is created per the user who views that link.

Can you explain how would an attacker exploit that then?

[lirantal]

@Koblaid nevermind I think I understand what you were referring to.
In my opinion we can handle this by:

1. introducing a config variable for users to setup their domain/host which will be used across the site
2. If (1) doesn't exist then we fallback to the req.headers.host or maybe the request's originating IP is more secure?

WDYT?

[barbogast]

I would prefer 1 and refuse to start if the config option does not exist.

Falling back to req.headers.host would mean to accept the security risk if the user forgets the option or is too lazy and does not understand the security implications. I think enforcing security is a good thing, especially if its done with a simple config variable.

The requests originating IP would be the IP-address of the requesting host, right? This doesn't help anything, does it? Or do you mean the IP-address under which the server is running? I'm not sure if/where it could be accessed in a non-spoofable-manner. In addition, the IP of the actual server could differ from the IP under which the site is accessable from the outside (for example on Heroku). Also the IP would look suspicious in the email and using HTTPS would result in a browser warning.

So I think enforcing the configuration option is the only secure way to handle this.

[lirantal]

Well "refuse" is a bit harsh.
Some may choose to use MEAN.JS as an internal application and will not give much weight as to security considerations like that.

Can you offer another option without restricting the domain config?

[barbogast]

The only way of a fallback is using the HTTP header like it is done at the moment.

I think in the end it's a question of convenience vs security. If you prefer convenience for the users falling back to HTTP header is the best option.

My opinion is that software facing the web should prefer to add too much security in the case of doubt. I think users (me including :-) ) except frameworks such as mean.js to be secure by default. Also there is already a configuration system and most users probably have to configure something, so another option shouldn't really hurt.

Concerning internal application: I think that especially in big companies there is a change in the way they secure internal applications. Because more and more networks get intruded without being detected the internal network can no longer be treaded as secure. So internal applications have to be secure too.

[barbogast]

Well instead of refusing to start maybe a red warning would be sufficient if the config is not set:

To prevent HTTP Host header attacks please set the domain / ip-address of this app the config.

[jloveland]

👍

[jloveland]

maybe put the following in default.js:
hostname: process.env.HOST || 'localhost',

and in production.js:
hostname: process.env.HOST || 'www.yourdomain.com',

then, add to options when creating the server:
https://github.com/meanjs/mean/blob/master/config/lib/socket.io.js#L55

WDYT?

[barbogast]

I think a default for production is dangerous, since there'll be no warning if the user forgets to change it.

How about this:

default.js:
hostname: process.env.HOST

developtment.js:
hostname: process.env.HOST || 'localhost'

express.js:

  // Passing the request url to environment locals
  app.use(function (req, res, next) {
    var host = config.hostname || req.headers.host;
    res.locals.host = req.protocol + '://' + host;
    res.locals.url = req.protocol + '://' + host + req.originalUrl;
    next();
  });

EDIT: and somewhere the red warning if the hostname is not set in production mode

[jloveland]

good call, the default for productions would be dangerous. I like the idea of a warning.

maybe we also put a comment in the production.js to remind people to set it?
// hostname: process.env.HOST || '<INSERT YOUR DOMAIN>'

[lirantal]

Sounds good to me

[mleanos]

👍 I like the conclusion that you three came up with... along with @jloveland last comment.

treating this like any other config seems like a fine idea.

[mikepc]

I agree with the setting being required to start, but it would be helpful if defaults were pre-configured to handle localhost for the dev environment

[ilanbiala]

@lirantal @Koblaid can we finalize this so we can merge it in for 0.4.2?

[barbogast]

AFAICS the only thing missing are tests. Unfortunately I don't have time to write tests at the moment. Maybe it's accectable to merge it without tests.

[ilanbiala]

@Koblaid we can't merge without tests, we need to make sure everything we add works.

[mleanos]

Fixed with #1469