"match" in Rails and CSRF

Related:
CSRF afterparty & MUST READ rules
Playing With Referer & Origin + disqus.com and yfrog.com Vulnerability)

Sometimes developers use GET for state-changing requests by purpose. This determines low-skilled developer. In this post I want to describe another vector of attack - GET Accessible Actions(GAA) - when framework abstractions + bad practices make "Good Guys' apps" insecure.

There is a thing I found exploring DSL's and engines' internals - many frameworks do "dirty work" for developer - they merge GET(stored in URL string)+POST(stored in Body) params into one unified hash. IMO it's cool and awesome to use - "params" for Rails and $_REQUEST for PHP.

But! As I found out it often confuses mechanism of handling GET requests from others(POST-like - PUT/DELETE/etc).
Same "state-changing" code is accessible via GET either, it is GAA, opening the easiest to use hole: <img src=URL/create_message?message[body]=SPAM>

php:

If you use $_REQUEST in your PHP app I'm pretty sure if Hacker sends GET request with same params in URL instead of Post Body it will skip CSRF verification filter(I hope you have one haha) and you're pwned. Most likely using $_REQUEST in your app is insecure. You better to stop it.
Separate routes for GET/others carefully, don't let them to intersect or share same code(I mean :action)! Different Verbs - different Actions!  Well, whitelist them.

showcase - yfrog.com. If you try http://yfrog.com/twitter/follow.json?screen_name=homakov because yfrog uses $who_to_follow = $_REQUEST['screen_name'] and that's sad...

rails:

Introduced CSRF protection by default

Default generated routes.rb contains these lines:


# This is a legacy wild controller route that's not recommended for RESTful applications.
# Note: This route will make all actions in every controller accessible via GET requests.
# match ':controller(/:action(/:id))(.:format)'

Obviously, these lines don't make any sense. Let me to show what should be written there:

# WARNING: This route will expose all actions in every controller to CSRF attacks.

# DO NOT USE

# match ':controller(/:action(/:id))(.:format)'

because sentence "all actions in every controller accessible via GET requests" means/explains nothing "insecure" or dangerous for person who reads it. It's just "Note" + http://guides.rubyonrails.org/security.html contains no single line related to the issue(I hope rails team will add at least slight mention). They use match and need your advice.

But all rails 2.3 apps use map.connect == are vulnerable(holy shi~!)   and I see no single reason to keep insecure and useless generated route in modern RESTful rails 4.0 routes.rb, thus these lines should be removed + rails team should clean up contents of default templates.

Showcases:

old one with heroku.com 

<img src=xxx/update?site_name[name]=yyy> will rename your xxx.herokuapp.com to yyy.herokuapp.com

Administration via GET, because they used match "update", to: "sites#update" and it's awful :(

stumbleupon.com

http://www.stumbleupon.com/follow/stumbler?userid=10572172&_method=create&status=follow

following works via GET

tumblr.com

http://www.tumblr.com/like/GvWiOtPL?id=20450314081&formkey=123&redirect_to=/

liking via GET

justin.tv

http://www.justin.tv/iammatrix/follow and http://www.twitch.tv/community/add_favorite/24216117

following and favorites via POST w/o token verification because they skipped the filter!

posterous.com vuln still works coz fixed poorly. PROTIP: simple adding authenticity_token to URL won't work because rails passes all request.get? by default. I guess you wanted to make "hacker" think that you have protection? :)

+ ~5 other rails sites from previous post

Again, Why To Deprecate(and than make it private later) "match":

1. It is a security hole. It skips protection_from_forgery and even innocent <img src=...> has malicious impact. "Mass assignment of routing" :).
2. It is a bad practice. There are just no single reason to use this method!
   
   You always know which HTTP Verb is used for specific "controller#action"! If you don't - you are doing it wrong.
   If you have: "request.post?" or even worse "params[:some_param_to_detect_post]" conditions - I bet controller code sucks - please refactor it.
   
   If you use match to: 'blah#blah', :via => :post I just wonder why you make additional efforts instead of using clean & pretty post to: 'blah#blah' They are equal(browse teh codez to make sure)
   
   If you use match for redirects or catching 404 - just replace it with 'get'! Voila, it works the same!
   
   Useless but dangerous method.
3. It's not RESTful.  "resources" is a very safe method if you use it properly. PROTIP - RESTful = awesome & secure. Look at this - exactly what I expect to see in every routes.rb(using namespaces would make it even prettier but I don't mind)

I hope to have constructive discussion with the community, I will update the post to answer your questions/arguments(contact if have ones). Also this PR does necessary removing of insecure, useless, legacy and dangerous code. UPDATE: merged in 3 minutes wow ;D

Thanks for reading.