undefined method `detect' for nil:NilClass #50

Closed
blambeau opened this Issue Apr 4, 2013 · 24 comments

Projects

None yet

3 participants

@blambeau
blambeau commented Apr 4, 2013

Hi,

I'm currently observing a recurrent error in an production app whose backtrace seems rooted in rack-protection. I'm still investigating and trying to reproduce, but if you have any idea...

from vendor/bundle/ruby/1.9.1/gems/rack-protection-1.5.0/lib/rack/protection/base.rb:107:in `html?'
        from vendor/bundle/ruby/1.9.1/gems/rack-protection-1.5.0/lib/rack/protection/frame_options.rb:32:in `call'
        from vendor/bundle/ruby/1.9.1/gems/rack-1.5.2/lib/rack/nulllogger.rb:9:in `call'
        from vendor/bundle/ruby/1.9.1/gems/rack-1.5.2/lib/rack/head.rb:11:in `call'
        from vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/base.rb:172:in `call'
        from vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/base.rb:1947:in `call'
        from vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/base.rb:1610:in `block in call'
        from vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/base.rb:1693:in `synchronize'
        from vendor/bundle/ruby/1.9.1/gems/sinatra-1.4.2/lib/sinatra/base.rb:1610:in `call'

Thanks!

@rkh
Member
rkh commented Apr 4, 2013

That would indicate that @app.call(env) is not returning a rack tuple, as the second element is nil instead of a header hash.

@rkh
Member
rkh commented Apr 4, 2013

Which, of course, is strange. You can only reproduce this in production?

@blambeau
blambeau commented Apr 4, 2013

I'm still trying to reproduce it outside our production environment... I'll let you know what's wrong and if it's related to sinatra or rack or due to an error on our side.

@bugant
Contributor
bugant commented Apr 6, 2013

Could it be due to the fact that JsonCsrf protection does not check for nil response?
https://github.com/rkh/rack-protection/blob/master/lib/rack/protection/json_csrf.rb#L22

Base, does it correctly instead: https://github.com/rkh/rack-protection/blob/master/lib/rack/protection/base.rb#L47-L49

Suppose you have set the default reaction to ":drop_session", then that react will return nil if I'm not missing something... Do you think it make sense? If it is the case I could send you a PR (with a test this time :D).

Hope it helps

@blambeau
blambeau commented Apr 6, 2013

@bugant might be. could you try to reproduce your scenario with a test?

@bugant bugant added a commit to bugant/rack-protection that referenced this issue Apr 8, 2013
@bugant bugant Add regression test for issue #50
Running specs you get

Failures:

  1) Rack::Protection::JsonCsrf with drop_session as default reaction reset the session
     Failure/Error: get('/', {}, 'HTTP_REFERER' => 'http://evil.com', 'rack.session' => session)
     NoMethodError:
       undefined method `detect' for nil:NilClass
     # ./lib/rack/protection/base.rb:107:in `html?'
     # ./lib/rack/protection/frame_options.rb:32:in `call'
     # ./spec/json_csrf_spec.rb:54:in `block (3 levels) in <top (required)>'
d1f0300
@bugant bugant added a commit to bugant/rack-protection that referenced this issue Apr 8, 2013
@bugant bugant FIX: check for nil response on JsonCsrf protection
Some reaction do not return a response, think for example drop_session. In that case a nil response
would be returned, see issue #50.
eb7e4c9
@bugant
Contributor
bugant commented Apr 10, 2013

@blambeau Does this fix resolve your issue? It did for mine Sinatra app.

@blambeau

@bugant yes it does! thanks :-)

@blambeau

@bugant @rkh may I have a naive question however? Why does Sinatra use a :drop_session default reaction whereas the JsonCsrf class configures a :deny? Is the attack effectively prevented by dropping the session?

@rkh
Member
rkh commented Apr 10, 2013

If you drop the session, no confidential data can be accessed.

@bugant
Contributor
bugant commented Apr 10, 2013

@rkh right, but drop_session does send the response anyway, so the first request can actually access confidential data. Am I right?

@blambeau

@bugant that's what I had in mind indeed, the attack seems successful in my opinion.

@rkh
Member
rkh commented Apr 10, 2013

Yes, that seems correct. We might need to change Sinatra or something. Note that the protection currently doesn't fully protect against the attack anyways (and other frameworks like Rails don't protect against this at all).

I have to think about a good solution.

@rkh
Member
rkh commented Apr 10, 2013

Suggestions welcome. :)

@blambeau

@rkh I think the default reaction of each class should be safe (best effort to prevent the attack) and used by Sinatra by default (unless another reaction is specified by the OP). There should be a way to override the reaction in response to specific attacks (specific countermeasures) without changing the default behavior of others.

My use case as an example: I'd like to trust Sinatra/rack-protection counter-measures for all attacks that I don't know/understand. I'm also sure that I want to deny requests on every JsonCsrf attack now that I'm facing them and understand their effect on my system.

@rkh
Member
rkh commented Apr 10, 2013

Seems legit, but that would mean we need to loosen the default reactions in here, as they are too strict for a normal Sinatra app.

@blambeau

as they are too strict for a normal Sinatra app.

why? could you provide specific examples? an alternative would be to use the new overriding system in sinatra so as to weaken default reactions here, isn't?

@rkh
Member
rkh commented Apr 10, 2013

POST requests without a CSRF token. If they'd trigger a rejection, that would break web APIs.

@blambeau

Not sure to understand. Why not modifying protection setup in sinatra instead (https://github.com/sinatra/sinatra/blob/v1.4.2/lib/sinatra/base.rb#L1653)?

Either way, I think that a broken API will appear in either Sinatra or rack-protection. You choose which one if not both.

@rkh
Member
rkh commented Apr 10, 2013

Broken API?

@blambeau

@rkh broken behavior, I mean, weakening default reactions.

@rkh
Member
rkh commented Apr 10, 2013

I still don't understand what's broken where.

@rkh
Member
rkh commented Apr 10, 2013

I mean, the JSON CSRF protection, obviously.

@blambeau

we need to loosen the default reactions in here

If you do that, everyone that is currently expecting strict default reaction will experience a broken behavior IMHO (such as, for example, not rejecting the request). Unless there is something I don't understand?

@rkh
Member
rkh commented Apr 10, 2013

I agree, but then we need Sinatra to change the reaction. As it does right now.

@rkh rkh added a commit that closed this issue Oct 21, 2013
@rkh rkh let json_csrf always deny, fixes #50 00968ce
@rkh rkh closed this in 00968ce Oct 21, 2013
@zzak zzak pushed a commit that referenced this issue Aug 12, 2016
@bugant bugant Add regression test for issue #50
Running specs you get

Failures:

  1) Rack::Protection::JsonCsrf with drop_session as default reaction reset the session
     Failure/Error: get('/', {}, 'HTTP_REFERER' => 'http://evil.com', 'rack.session' => session)
     NoMethodError:
       undefined method `detect' for nil:NilClass
     # ./lib/rack/protection/base.rb:107:in `html?'
     # ./lib/rack/protection/frame_options.rb:32:in `call'
     # ./spec/json_csrf_spec.rb:54:in `block (3 levels) in <top (required)>'
ac79948
@zzak zzak pushed a commit that referenced this issue Aug 12, 2016
@bugant bugant FIX: check for nil response on JsonCsrf protection
Some reaction do not return a response, think for example drop_session. In that case a nil response
would be returned, see issue #50.
fa58e05
@zzak zzak pushed a commit that referenced this issue Aug 12, 2016
@rkh rkh let json_csrf always deny, fixes #50 5d4f1d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment