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

[amoskopp]

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:

[mscdex]

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]

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]

@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]

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

Yes, almost certainly.

[amoskopp]

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

[not-an-aardvark]

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.

[amoskopp]

@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]

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.

[amoskopp]

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]

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

[TimothyGu]

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

[Trott]

Should this remain open?

[TimothyGu]

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