Adding a counter to the decoder to prevent hashflooding?

[alessioalex]

Arnaut (`3rdEden) came up with this monkey-patch idea for the querystring native module, but I think we can implement it here also, here's what he said on the google group:

The fastest workaround for this would just be adding a counter to the decoder, and have it break out the forEach loop after
it has stored about 1k keys in the obj.

[tj]

i still find it funny that they haven't seen the attack in practice haha, theoretically there are all sorts of attacks one could perform. I'm fine with this though and we could default it to something reasonable (cant imagine anyone even going over 50 really but maybe 1000 as a default)

[buschtoens]

Cap or throw?

This is again one of the cases where I'd really like to throw, as the resulting object is jibberish and a DOS attempt anyways, but on the other hand I don't want to kill all legacy apps.

So do we just cap or return {}? The limit would also be a thing for #89.

[rlidwka]

Throw 414 without any doubt.

Otherwise if there are people affected by this, they would have nice time figuring out why their app doesn't work.

[buschtoens]

(Of course this problem exists solely server-side)

Sadly we can't just throw a nice 414 as we don't have access to the response object. But I guess you meant throw new Error("414: Request-URI too long");.

Good frameworks capture those and pass them on to the client. Smelly homebrewn code probably doesn't even have process.on("error"). In this case we will crash a good bit of apps, if they get DOSd.

The alternative either would be returning {}, which will provoke some errors later on or pass on the blah blah wich will likely cause the same.

Throwing doesn't sound too bad.

[buschtoens]

(Can't edit on the iPhone, please ignore poor grammar and spelling 😉 )

[rlidwka]

Sadly we can't just throw a nice 414 as we don't have access to the response object

Oh sorry, I completely forgot that qs is not a connect-only module. :)

But error.status and error.code are widely used for this kind of things.

[buschtoens]

Your argument itself is still valid though. I feel really like we should throw. This is a case where we should force the user to take action.

What does our dearest @visionmedia say? :)

[tj]

throwing in node stuff is generally bad if you can avoid it, you never know where they'll call qs.parse(), if it's not immediately in a route etc it likely won't get caught and will explode your program. I'd probably just cap it to a reasonable depth/size

[buschtoens]

And this is the reason I would want to throw. To enforce better security on the user. Otherwise 90% won't do shit. The request is lost anyway and returning a capped qs doesn't really make sense either. And if the app blows up, chances are high it's an app the user can restart almost immediately.

If you still don't want to throw, I'd vote on counting the & before actually parsing and then return {}. So we don't waste precious time parsing garbage and blocking everything else.

[buschtoens]

Anyway, you have the last word. Cap, count and {}, or throw?
I'll implement it.

[eivindfjeldstad]

I was under the impression that Google already fixed this by using a random seed for hash tables. Either way, I think throwing should be avoided. Maybe just break the loop and print a warning or something?

nodejs/node-v0.x-archive@146b2e2

[tj]

yeah the hash thing was a different issue, right now we just allow arbitrarily long query-strings so you could still get away with impacting a service, just like if you were to sent 500mb of json or something and didn't have a limit on request bodes.

I'd be ok with capping or {}, both are potentially confusing for legitimate requests if a dev just sends something large expecting it to work, maybe we should add a console.warn() in there

[eivindfjeldstad]

ah, I see. I think the node querystring module has a default limit of 1000 keys as well. seems like a reasonable default to me.

[buschtoens]

Okay, so no throwing, but console.warn().
Now there are two options left: Cap or count the delimeters before trying to parse them.

[eivindfjeldstad]

counting the delimiters won't help if someone is doing blah[blah][blah].....[blah][blah]=1

[buschtoens]

True. So we'll go with capping and console.warn().