Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix parsing of `?a=1&a[a]=1`? #524

Closed
djanowski opened this Issue · 39 comments
@djanowski

I started to see a few 500s on my application due to this malicious request.

I don't think it poses a major security flaw, but it is indeed annoying and I wonder if it could lead to an easy DoS.

Also, googling a bit shows that some use this technique to quickly identify Rack applications (maybe to later try other disclosed vulnerabilities?)

I see a few options:

  1. Do nothing.
  2. Ignore the assignment of a[a], keep the first a, a String.
  3. Raise a more specific exception, for easier rescue (currently it's a TypeError: expected Hash (got String).

FYI https://github.com/?a=1&a[a]=1

What do you think?

@Houdini

same error here.

@rafaelsachetto

happened a few times here on two projects in production

@avolkovi

We are seeing this on production as well, anything to worry about? or should we just wait on a patch to rack?

@Houdini

As I understand -- nothing to wary about.
Just keep things up to date.

I like option 2: Ignore the assignment of a[a], keep the first a, a String.

@rafaelsachetto

I believe that ignoring with regular expression pattern is best, the same error happen with other letters

@djanowski

@rafaelsachetto Of course, we're talking about ignoring these TypeError errors down in the parsing code. Nobody's suggesting we treat a as a special case.

@orangewolf

I think this is actually more serious that we've been giving it credit for. An uncaught exception in Rack for this means you can raise an exception all the way to the top in basically very singe rack dependent app. We saw a DDOS attack attempt this last week that tried to use this method to crash our app and thus making the attack easier.

Would a patch that catches this exception and deals with it in a sane way be accepted?

@djanowski

OK, time to summon @raggi @chneukirchen etc.

@orangewolf

@djanowski agreed. I don't think there is a disclosure issue here, since it is broadcast all over this issue list already (issue #400 is the same issue) and in several other places, but because it appears to be out of the bag, we should move on it fairly quickly.

@chneukirchen
Owner

I could imagine catching this early and sending a HTTP 400.

@orangewolf

@chneukirchen that seems reasonable to me.

@svoboda-jan

RFC3986 (URI Generic Syntax), section 3.4 mentions only that the part after ? is the query component, contains non-hierarchical data and is being used with key=value, but no recommendations.

W3C on collecting URL parameters (a draft and doesn't mention arrays):
http://www.w3.org/TR/url/#collect-url-parameters

As there is no formal specification on how to handle arrays, I don't like the idea of having HTTP 400 responses for valid URLs. These responses would not only reveal very easily the rack stack but also differ a lot from other language's URL parameters parsing implementations. I am not aware of any implementation throwing HTTP 400 for query params. Anybody ?

My suggestion is to look into how other languages / libraries handle this case and stay close to it. (possibly ignoring keys not parseable to ruby)

Just as a note, this is how CGI parses it:

irb(main):007:0> CGI.parse(URI.parse('https://github.com/?a=1&a%5Ba%5D=1&a=2').query)
=> {"a"=>["1", "2"], "a[a]"=>["1"]}
@gioele

I think it would be OK for Rack to respond with a 400 HTTP status if there were also an opt-out option somewhere that allowed applications to receive the plain, non parsed query component in case they want.

@presskey

I don't think that exception suppression is a best idea but for now you could use this approach to skip invalid parameters presskey@2b46fbe
?a=1&a[1]=1&b=2 will be parsed as {'a' => '1', 'b' => 2}

@raggi
Owner

Picking option 1.

@raggi raggi closed this
@orangewolf

@raggi that seems like a mistake to me. You're choosing to leave in trivially produced crash on every Rack based app in production.

@djanowski

@raggi Given that every single Rack app is affected, what is the proposed solution? Inserting middleware just to test for this condition and ignore TypeErrors?

@raggi
Owner

No, I don't recommend penalizing every users latency for the sake of changing the error code of a perfectly valid error.

I would rather change the way that rack handles parameters in general, but, such a thing is a MAJOR API change that really can't happen in the near term.

This isn't the first time this has come up, and it won't be the last. Most of the time, people just want "Rack to return a 400 response", but there's an important misconception there: Rack is not in control of response codes, users are. Again, this is an API restriction - Rack is a very toolkit API right now, not a framework API. It's not in control of anything, you can opt out of everything. In fact, you can create perfectly valid Rack applications without the Rack gem, as Rack is really defined by the SPEC. The rest of the code in the gem is there to assist people in writing common case Rack applications.

I am not sure that it will help people to squash this silently either. That could easily lead to bugs that won't even be caught by tests (as no errors arise, and units that test single parameters will pass). Trying to change the error to a different error code also is really an open argument. Some argue that 400 is more appropriate, but one could refute that, on the claim that such a request is perfectly valid HTTP. Some argue that other 4xx codes are appropriate, but mostly those other options pertain to entities, which these parameters are not. The 500 code is arguably more appropriate for this case, such that this is a known error causing restriction in the Rack implementation of parameter parsing. Given that Rack applications utilize the behavior of this parser, it is also generally expected that they should not generate code that causes it. Given this, one can continue the 500 argument, claiming that the language "the server encountered an unexpected condition" is appropriate.

If I created a subtype of TypeError would that make you more happy than the present situation? I don't think there is an appropriate response that goes much further, without breaking API compatibility, at which point we should be considering a whole new parser API with better semantics for arbitrary input.

@raggi raggi reopened this
@orangewolf

@raggi I'm in the camp that leans toward returning a 400 since such requests are not really defined as processable in the HTTP specs. However, I think you're narrowing in on a more universally acceptable solution. I don't think anyone here is arguing that there is an obvious or simple fix for this issue.

I think that creating a subtype of TypeError and explicitly raising that error would have significant advantages over what is now a fairly obtuse error response. That would allow frameworks like Sinatra and Rails to gracefully handle said exception in some meaningful way and help developers to stop feeling the need to report this as a new issue going forward. If Rack is going to pass the buck up the chain, doing so 'on purpose' seems like a better approach that doing so arbitrarily.

@djanowski

@raggi I agree that a more flexible parameter parsing API is what is desirable in the end.

I also understood from the beginning that the Rack toolkit wouldn't be able to simply respond with an arbitrary response code, so that's out of the question too.

However, I believe this should be seen as a form of attack, and not as a feature that users some time make use of. Just like web servers protect themselves from excessively large HTTP headers or bodies, one could argue that given the current state of affairs (the Rack toolkit using a single, hard-coded parameter parser) it would be right for it to protect against a known form of (a light) attack.

If we can't agree on this point of view, then I think that yes, raising a more specific exception would allow users and frameworks to make better decisions on how to handle this scenario.

@svoboda-jan

:+1: for at least a subtype of TypeError.

@orangewolf requests with the problematic query string are actually valid and processable from the point of view of the HTTP spec. Also they do not cause any error responses in JSP, ASP or PHP.

@raggi
Owner

@orangewolf there is nothing invalid about these requests in the HTTP spec.

@djanowski I'm not sure what you're hoping for in terms of your suggestion that the attack can be prevented. We cannot meaningfully parse arbitrary query strings in a sane way. At some point we have to treat input that we don't understand as an error. We could change the API to return a non-exception error value, however, this is particularly unusual to the language, and requires a major API change.

I'll add an error subtype, and we'll start there.

@chneukirchen
@csuhta

I have a few similar URLs which are produced by the Nikto scanner while it's looking for PHP vulnerabilities. These cause a Rails 3.2.13 + Rack 1.4.5 app to throw an exception and show the error 500 page. These are a slightly different format than ?a=1&a[a]=1 so I figured I'd post them here.


/includes/orderSuccess.inc.php?&glob=1&cart_order_id=1&glob[rootDir]=http://cirt.net/rfiinc.txt?
TypeError 
expected Hash (got String) for param `glob'
vendor/bundle/ruby/2.0.0/gems/rack-1.4.5/lib/rack/utils.rb:127:in `normalize_params'

/index.php?_REQUEST=&_REQUEST[option]=com_content&_REQUEST[Itemid]=1&GLOBALS=&mosConfig_absolute_path=http://cirt.net/rfiinc.txt?
TypeError
expected Hash (got String) for param `_REQUEST'
vendor/bundle/ruby/2.0.0/gems/rack-1.4.5/lib/rack/utils.rb:127:in `normalize_params'
@vanderhoorn

I'm seeing the errors that @csuhta mentions.

+1 for a subtype of TypeError, so we can do something with the error higher up in the chain.

@orangewolf

@csuhta the import bit is that any url which includes a key set both as a single and later as an array will cause the running process to barf.

@raggi
Owner

@orangewolf the process shouldn't "barf", if the app doesn't handle it, most web servers should return a 500.

It's not possible for us to avoid errors when error values are provided - e.g. bad encodings and so on. Users need to protect themselves from bad input and DOS by other means. Errors in ruby are slow, that's not Racks fault, but not erroring and allowing the process to continue is even worse - apps would become exploited (not just DoS'd) by such behavior. As already discussed it's impossible with the Rack API for Rack helpers to return responses. Users must see this as a slow path in their application and protect it accordingly. Rack isn't doing anything horrendous here, this isn't some kind of O(n^m) vector that can be trivially exploited to cause runaway processes. If you're DDoS'd using this path, it's quite likely that you could be more easily DDoS'd with less workers if they make valid requests to your rendering endpoints, as this exception is actually significantly faster than rendering simple ERB templates.

Final thoughts please, before close.

@raggi raggi added this to the Rack 1.6 milestone
@orangewolf

@raggi when I say barf I mean throw an exception that appears like a random bug in Rack (TypeError expected hash got string) and not any kind of deliberate "we meant for this to error" kind of response. You can't just catch all TypeErrors from Rack on the assumption that all the rest of the code is error free (as awesome as Rack code is in general). That is why I was suggesting a more descriptive or handleable error.

If the Rack team doesn't want to consider this a bug, I'm fine with closing this issue. We solved the problem on our end by monkey patching rack to throw an error we could consistently deal with up the stack and switching our configuration around so that exceptions were no longer causing threads to restart (the restart was what makes this a DDOS vector, not the exception it self).

@vanderhoorn

Exactly what @orangewolf says. It's not a problem that Rack throws an error, it's a problem that it's very cumbersome to handle the error higher up. A more specific exception would improve the ability for applications to deal with the error (if they want to).

@csuhta

Not sure if useful, but there is some ongoing Ruby and RFC 3986 work that might affect this issue. If I am reading correctly, URIs will no longer be allowed to contain brackets in the query string:

https://bugs.ruby-lang.org/issues/2542
https://twitter.com/tenderlove/status/483740028041179136

@raggi
Owner

@csuhta yep, we got that covered, at least step 1. we likely have more to do, as clients will start causing breakage in apps sooner. In general, rack doesn't parse incoming URIs, there's one exception I need to cover before 1.5.3 heads out the door.

@raggi
Owner

@orangewolf re. changing the exception type, I'm totally onboard with that.

@csuhta

@raggi @orangewolf That would be fantastic, at least for my use case. I don't really care about what the request was for, I just want to return an error 400 or or 422 to clients that send my applications junk and be done with them. A parent error class would help catch everything that appears to be malformed.

Sketching here:

RequestParseError
└─ URIParseError
└─ RequestEncodingError
@raggi raggi closed this issue from a commit
@raggi raggi ParameterTypeError for parse_nested_query
Inherits from TypeError, so existing code that checks types by is_a? and friends
should still work fine. This should enable users to be more confident that they
are only catching this error case, not other exceptional conditions.

Closes #524
167b648
@raggi raggi closed this in 167b648
@raggi
Owner

@csuhta I can't do that in a point release unfortunately. I need to keep the new error be a child of the existing error to keep existing error type checking code API compatible.

@csuhta

@raggi That's fine too, as long as its uniquely identifiable. Do you think this subclassing strategy could also be applied to exceptions raised from UTF8/percent issues, like in #337?

@raggi
Owner

@csuhta it would be nice wouldn't it? In that particular case, we're now relying on stdlib (which has been the source of >50% of the rack tickets since the change, FTR). Unfortunately this means I'm not in control of the raise location. I could catch and re-raise but that wouldn't affect the accuracy of the catch at all. TBH, neither did 167b648, but I understand why you guys want it. For a future system (rack 2 or something like that), a strong error handling policy from the start would be sensible.

@orangewolf

thanks @raggi this looks great.

@Houdini

@raggi
I think different - we should not raise error, as it's a valid request. Rack restrict specification and it's rack's problem how understand non restricted input parameters.
This looks the most correct way for me:

?a=1&a[1]=1&b=2 parsed as {'a' => '1', 'b' => 2}

But I suppose I'm late :)

@raggi
Owner

@Houdini I think there's some misunderstanding leading to your comment, but I'm not certain.

Please understand first of all that you can "use Rack" without even including the Rack gem. Rack is really a SPEC first and foremost, and the SPEC does not require the use of any code in the Rack gem.

The code in the Rack gem provides helpful utilities for doing common things.

In this case, the only time an error is raised is if a Rack user explicitly calls parse_nested_query, and the string they passed could not be parsed.

Silently swallowing errors provides users with no feedback at all and can lead to all manner of oddities, potentially even security issues (although the latter would require poor rule enforcement), so I will not do that.

As far as changing the parser semantics, everyone has their opinion. If we were to ever get to Rack 2, there would not be a nested query parser in the core, because no one in the community can agree on what it should do, and we constantly receive requests to change it.

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.