Skip to content
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

querystring.unescape() accepting input that is not URL escaped #10727

Closed
nmoskopp opened this issue Jan 10, 2017 · 13 comments
Closed

querystring.unescape() accepting input that is not URL escaped #10727

nmoskopp opened this issue Jan 10, 2017 · 13 comments
Labels
querystring Issues and PRs related to the built-in querystring module.

Comments

@nmoskopp
Copy link

nmoskopp commented Jan 10, 2017

The documentation for querystring.unescape() says:

By default, the querystring.unescape() method will attempt to use the JavaScript built-in decodeURIComponent() method to decode. If that fails, a safer equivalent that does not throw on malformed URLs will be used.

Accepting invalid input usually makes software less safe, not more. Example: In hapijs/hapi#3422 data that is not URL encoded is passed to an application that should accept only url-encoded data (application/x-www-form-urlencoded), leading to a failing test. Quote myself:

Every piece of software that takes input contains a de facto recognizer for accepting valid input and rejecting invalid input. Parser differentials – when two programs parse things differently, one accepting data and another rejecting it – silently invalidate assumptions programmers have about data safety and can lead to security issues. I think a good example of what this can result in is the Android master key vulnerability.

For more information why accepting invalid input is a bad idea, read The Seven Turrets of Babel and other LANGSEC papers.

I suggest to not catch the exception thrown in querystring.unescape() so that Hapi can return a 400 Bad Request.

@mscdex mscdex added the querystring Issues and PRs related to the built-in querystring module. label Jan 10, 2017
@mscdex
Copy link
Contributor

mscdex commented Jan 10, 2017

If you're interested in catching the error yourself, why not use your own custom decoder? If you're using it via querystring.parse(), you can pass in your own decoder there.

@jasnell
Copy link
Member

jasnell commented Jan 10, 2017

While I happen to agree that the current behavior is not ideal, changing is not likely to happen, as unfortunate as that is. The new WHATWG URL includes improved handing of querystring content but even it currently still falls back on the querystring module (but only after it performs better handling of the pct-encoded content)

@nmoskopp
Copy link
Author

@jasnell what are the arguments for not changing the behaviour here? Is some software relying on node accepting data that should be URL encoded, but is actually not URL encoded?

@not-an-aardvark
Copy link
Contributor

Is some software relying on node accepting data that should be URL encoded, but is actually not URL encoded?

Yes, almost certainly.

@nmoskopp
Copy link
Author

@not-an-aardvark if you are so certain, could you provide an example?

@not-an-aardvark
Copy link
Contributor

For example, this code will cause a server to return a 500 error on certain inputs if querystring.unescape throws.

I haven't looked at any popular packages yet (I just found this example from a GitHub search), but if we changed the behavior, any server that uses querystring.unescape on untrusted user input would be liable to return a 500 error and/or crash when it encounters a malformed querystring. Even if throwing an error for malformed querystrings would be slightly more intuitive in this case, I don't think it would be a good idea to start making it throw now.

@nmoskopp
Copy link
Author

@not-an-aardvark fixing such a bug certainly changes behavior – and I think that software should return a 400 Bad Request in that case, not a 500 Internal Server Error – but I do not see how anything in that code depends on accepting invalid input.

Could a fix for this issue be scheduled for the next major version of Node to prevent suprises?

@not-an-aardvark
Copy link
Contributor

but I do not see how anything in that code depends on accepting invalid input.

If a client passes a malformed querystring and querystring.unescape starts throwing an error, the server will start crashing. This is a DoS risk. (Admittedly, that wouldn't happen in this particular case since Sails has graceful error handling, but it's very likely that people are running servers using similar behavior without graceful error handling.)

Could a fix for this issue be scheduled for the next major version of Node to prevent suprises?

It could be released in a major version, but personally I don't think it would be worth it due to the potential for new DoS vulnerabilities when a server upgrades. People typically don't write tests for malformed querystrings.

@nmoskopp
Copy link
Author

nmoskopp commented Jan 11, 2017

You are right: People usually do not write tests for invalid input. That does not mean that it is a good idea to silently correct invalid input, as that can create subtle vulnerabilities that depend on parser differentials (like the Android master key vulnerability).

Since the patch is trivial … do you know how to get a bugfix scheduled for a major version?

@mscdex
Copy link
Contributor

mscdex commented Jan 11, 2017

@nmoskopp The first step would be to submit a PR.

@TimothyGu
Copy link
Member

For what it's worth, the URL Standard's percent decoder explicitly accepts broken encoding. querystring is IMO doing the right thing here.

@Trott
Copy link
Member

Trott commented Jul 30, 2017

Should this remain open?

@TimothyGu
Copy link
Member

No, IMO. Both of our application/x-www-form-urlencoded decoders are acting according to the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

No branches or pull requests

6 participants