Repository navigation
querystring.unescape() accepting input that is not URL escaped #10727
Description
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.
- Version: v.6.9.1
- Platform: Linux unknown-device 4.7.0-1-amd64 deps: update openssl to 1.0.1j #1 SMP Debian 4.7.2-1 (2016-08-28) x86_64 GNU/Linux
- Subsystem:
Activity
mscdex commentedon 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 commentedon 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)
amoskopp commentedon Jan 10, 2017
@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 commentedon Jan 11, 2017
Yes, almost certainly.
amoskopp commentedon Jan 11, 2017
@not-an-aardvark if you are so certain, could you provide an example?
not-an-aardvark commentedon Jan 11, 2017
For example, this code will cause a server to return a 500 error on certain inputs if
querystring.unescapethrows.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.unescapeon 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.amoskopp commentedon Jan 11, 2017
@not-an-aardvark fixing such a bug certainly changes behavior – and I think that software should return a
400 Bad Requestin that case, not a500 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 commentedon Jan 11, 2017
If a client passes a malformed querystring and
querystring.unescapestarts 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.)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.
amoskopp commentedon 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 commentedon Jan 11, 2017
@nmoskopp The first step would be to submit a PR.
TimothyGu commentedon Mar 20, 2017
For what it's worth, the URL Standard's percent decoder explicitly accepts broken encoding. querystring is IMO doing the right thing here.
Trott commentedon Jul 30, 2017
Should this remain open?
TimothyGu commentedon Jul 30, 2017
No, IMO. Both of our application/x-www-form-urlencoded decoders are acting according to the spec.