New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP parameter pollution in Express can aid attackers in bypassing security filters. #1824

Closed
skepticfx opened this Issue Nov 22, 2013 · 12 comments

Comments

Projects
None yet
6 participants
@skepticfx
Copy link

skepticfx commented Nov 22, 2013

If this is the first time you're hearing this term, HTTP Parameter Pollution aka HPP, then you need to read the links given below, to get an idea on what HPP can really do to a web application.

TL;DR:

If you give a query as ?name=123&name=abc
Then express returns 123,abc

Here are the links

With that being said, lets look at how popular web applications treat the same problem.

params

Apparently, only ASP and a few others spit back all the occurrences of the the same parameter name value.

Provided, express is the de-facto nodejs web app framework, I think its about time we take this seriously and make sure that only one of the parameter name is harnessed. Either the first or the last occurrence would do.

@skepticfx

This comment has been minimized.

Copy link

skepticfx commented Nov 22, 2013

I guess this might go down to Connect as well. But I don't think Connect should care about this, but a complete web app framework like Express should.

@defunctzombie

This comment has been minimized.

Copy link
Contributor

defunctzombie commented Nov 22, 2013

Um. Returning an array is the correct behavior since this is how forms
submit multi select boxes and checkboxes.
On Nov 22, 2013 2:58 PM, "No One" notifications@github.com wrote:

I guess this might go down to Connect as well. But I don't think Connect
should care about this, but a complete web app framework like Express
should.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1824#issuecomment-29104276
.

@skepticfx

This comment has been minimized.

Copy link

skepticfx commented Nov 22, 2013

@defunctzombie I agree with you on that. However, if this is the default behavior then the application using Express, would definitely suffer from security side effects. For instance, it can bypass the Chrome's XSS Filter in certain circumstances.

We would have to use something like req.query.name.getAll() or similar to get the array values. Returning the array by default, is generally not a secure way of treating HTTP parameters.

@jonathanong

This comment has been minimized.

Copy link
Member

jonathanong commented Nov 22, 2013

You should file this issue here: https://github.com/visionmedia/node-querystring

@skepticfx

This comment has been minimized.

Copy link

skepticfx commented Nov 22, 2013

Alright. Thanks.

@markstos

This comment has been minimized.

Copy link

markstos commented Sep 15, 2016

This issue is still open in the sense that it was punted to the node-querystring queue, and nothing happened with the issue there. I don't think the querystring library is right place to solve this. It's reasonable for the query string library to simply take multiple parameters in a query string and convert them to an array. That module is not involved in validation.

There is a simple API solution that Express can provide to address this, providing developers a "safe by default" solution.

  1. Provide an API that is safe for the common case. The common case is returning a single value for a given key, so provide an API that always return a single value, the first value.
  2. Still allow the exceptional case by providing a method that allows you get an array of values when explicitly ask for one.

Here's an example API for values in the query string:

req.query.single('always-one-value');
req.query.multi('may-have-multiple-values')

The query string cause is distinct from the "body" case, where you can pass complex objects as well as multiple values. It's also distinct from the "param" case, where the number of "params" passed are controlled by the server side route file. With a query string, only strings are every passed, the remote user controls them, and their two options are to pass a single value or multiple values.

The Perl CGI.pm module used a simple API when that project addressed this issue in 2014.

@markstos

This comment has been minimized.

Copy link

markstos commented Sep 15, 2016

HTTP parameter pollution in Express as recently highlighted as "Issue #4" in this security-focused blog post by Nodesource. The solution shouldn't be that every developer everywhere adds extra checks to nearly every parameter they receive to check if it's a single value or not. The framework should provide an easy secure-by-default option that avoid a widespread issue with Express apps being vulnerable to DoS attacks simply by sending them requests with multiple parameters.

@blakeembrey

This comment has been minimized.

Copy link
Member

blakeembrey commented Sep 15, 2016

@markstos It's very simple to build middleware which can correctly validation and sanitize the parameters based on a schema. To be clear, this isn't really a validation problem to me - but a deserialization issue. This is sadly a pretty common issue I've found with JavaScript development in general and it's definitely not restricted to just query string parsing.

A nice solution to this may be to move query string parsing into middleware by default. It's currently configurable by a flag and you can very easily replace use your own schema/deserializer. I'd be happy to build some middleware and modules around this, since I'm very interested in solving this in a more general way.

Edit: Here's a project I built for a previous employer to do this. It's https://github.com/mulesoft/osprey and more specifically https://github.com/mulesoft-labs/osprey-method-handler/blob/master/osprey-method-handler.js#L197-L221.

@markstos

This comment has been minimized.

Copy link

markstos commented Sep 15, 2016

@blakeembrey Thanks for the response. I completely agree on the value of using a declarative schema to validate and sanitize inputs. I helped write and maintain the Data::FormValidator which did that for Perl, and later helped integrate the concept with a Perl web framework with the CGI::Application::Plugin::ValidateRM and CGI::Application::Plugin::ValidateQuery modules.

Personally, I would still rather see a method for pulling a single-value-by-default from the query string built into Express. It adds a level of safety for all the different Express middleware validation solutions that might be built. If it's anything like the Perl ecosystem, some people will want complex solutions, others will want simple solutions. It would also help secure the sites for people who opt not use any fancy validation. The bottom-line concept is safe by default, which the current design is not.

If safe-by-default methods not going to be added, at a bare minimum the docs should be updated to explain that's a danger in assuming that a single value has been passed and it should always be checked. That leaves every developer to check if they got single values like they expected, so I see it as second rate solution.

Either way, I would love to have a good declarative for solution for declaring which parameters are required, optional and any constraints they must pass to be considered valid.

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Sep 15, 2016

Hi @markstos , I'm sorry you stumbled upon a 3 year old issue, as you probably haven't run across the various discussions we've had since this time (though to be fair, not all of them are on GitHub). I would absolutely love to see something made better around this area in the Express 5.0 release, of course! Would you be interested in putting together a pull request against our 5.0 branch implementing the simple API solution you have proposed above? That would be an amazing way to help the Express project address the concerns you have :)! This way, we can understand what your API proposal looks like, evaluate how compatible it is with the existing ecosystem, and if we like it, you can help us evangelize the change everyone has to make to their Express apps to upgrade to continue to get security updates. That would be 💯 👍

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Sep 15, 2016

@markstos and if you need help or have questions getting started on contributing, please don't hesitate to ask! Sadly our documentation in this area is still very much a work in progress, but I am trying to get that built out. I'd be happy to answer any questions you have and I can pretty much say that any questions you have around this will be very helpful as I'm trying to write them up, since it's not easy to know what questions new contributor actually have :) I look forward to collaborating together between all of us on this issue to see what we can accomplish together!

@markstos

This comment has been minimized.

Copy link

markstos commented Sep 16, 2016

Thanks for the invitation, Doug. I'll put together a Pull Request as time permits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment