Express: XSS in examples

[jthorpe6]

Hello,

just been playing around with the examples and i discovered XSS in the route-map example.

starting the app like so

~/express$ node examples/route-map/
get /users
delete /users
get /users/:uid
get /users/:uid/pets
delete /users/:uid/pets/:pid
Express started on port 3000

Then browsing to the following causes the injected javascript to load
http://ip:3000/users/%22%3E%3Csvg%20onload=prompt()%3E

full request

GET /users/%22%3E%3Csvg%20onload=prompt()%3E HTTP/1.1
Host: 192.168.122.246:3000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: close
Upgrade-Insecure-Requests: 1

the issue seems to be with the following in the codebase

var users = {
  list: function(req, res){
    res.send('user list');
  },

  get: function(req, res){
    res.send('user ' + req.params.uid);
  },

  delete: function(req, res){
    res.send('delete users');
  }
};

where req.params.uid is not sanitised

thanks

[wesleytodd]

While I am not too worried because the examples are just to show express features, not real use, I think it would be good to do one of two things here:

1. Add a comment about why rendering user content without escaping is bad
2. Escape the input

I would approve a PR which did either of the above.

[Caprico1]

https://www.owasp.org/index.php/Data_Validation#Sanitize

uh....

[Caprico1]

If it's within the examples odds are this is out in production somewhere. As a developer would look to examples either first starting out with the framework or just day to day. I can speak from experience on developers using example routes to build out there applications. Therefore since there is the possibility of it existing at least once within the base code, it's not a stretch for it to have been implemented into someone's production application.

[wesleytodd]

By all means, fix it and submit a PR :)

[dougwilson]

Hi @Caprico1 and no one is claiming otherwise. But of course fixing the example won't somehow fixes those theoretical production deployments, for example. That's also why the suggestion was as part of a pull request to fix the issue, to add information for the users who are looking at the examples to understand escaping and why the example is doing it, etc.

[Caprico1]

Right, it was to explain in an issue why this would be bad if in prod (owasp link)

and @jthorpe6 and I's thought process on how this is an issue.

I'll look into making a solution or at the least adding an example to sanitize inputs within routes within the framework.

[dataf3l]

Adding sanitization may not fix the existing codebases problem but it will address future users.

[krzysdz]

Speaking of security issues in the examples, it is possible to read any file (directory traversal) on a server running downloads example.

%2E%2E%2F is decoded as ../ and causes path.join() to go to a parent directory.

https://github.com/expressjs/express/blob/e1b45ebd050b6f06aa38cda5aaf0c21708b0c71e/examples/downloads/index.js#L19-L20

And for example, it could be used to read a session secret:
http://localhost:3000/files/%2E%2E%2F%2E%2E%2Fsession%2Fredis.js