Skip to content
This repository

Adding a counter to the decoder to prevent hashflooding? #28

Open
alessioalex opened this Issue · 16 comments

5 participants

Alexandru Vlăduţu TJ Holowaychuk Jan Buschtöns Alex Kocharin Eivind Fjeldstad
Alexandru Vlăduţu

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 Holowaychuk
Owner

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)

Jan Buschtöns
Collaborator

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.

Alex Kocharin

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.

Jan Buschtöns
Collaborator

(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.

Jan Buschtöns
Collaborator

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

Alex Kocharin

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.

Jan Buschtöns
Collaborator

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 Holowaychuk
Owner

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

Jan Buschtöns
Collaborator

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.

Jan Buschtöns
Collaborator

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

Eivind Fjeldstad

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?

joyent/node@146b2e2

TJ Holowaychuk
Owner

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

Eivind Fjeldstad

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.

Jan Buschtöns
Collaborator

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

Eivind Fjeldstad

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

Jan Buschtöns
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.