You are viewing an historical archive of past issues. Please report new issues to the appropriate project issue tracker on GitHub.

Bug #1280: Rails forgery protection tokens leaking onto cached pages, breaking the search form

Kind bug
Product wincent.dev
When Created , updated
Status closed
Reporter Greg Hurrell
Tags no tags

Description

This is really a Rails bug but I expect that if I file a bug for it with them I'll be told it's not a bug, so I am going to make a note of it here so as not to forget about it.

The problem is that forgery protection tokens are getting leaked onto every statically-cached page which has a form. This is almost every cached page in the site, because almost every cached page has, at least, a search form on it.

As an example, look at basically any of the tweets pages.

I expect the Rails folks will tell me this is not a bug because the tokens are derived using a hash of the current user's session key. This means that an attacker snooping the pages doesn't really have any chance of brute forcing and finding out what the victim's session key is. So as far as information leakage goes, this doesn't have a very large security impact.

But it's still a problem because it means that the search form is effectively broken for almost everybody who tries using it on one of those cached pages, expect the first user who tried accessing it (and thus caused the page to be cached in the first place); and even that user will find that the search form on that page stops working as soon as their session runs out.

The breakage stems from the fact that all these people who try to use the form will get 422 errors because the token won't match up with their session key. Even worse is the fact that my 422 page tries to be helpful and tell them that "The most likely cause is that your session expired due to inactivity. Try going back, then refreshing the page (this will create a new session), and then resubmitting."

So, it tries to help with the most common case (expired sessions), but in this case the cause is not an expired session at all, so going back and refreshing will have absolutely no effect.

As it turns out, the search controller doesn't take part in the request forgery protection machinery anyway seeing as that's the only way to make the search form on static pages like the 404 page work, and all search requests are side-effect free anyway.

Workarounds:

  • Patch Rails so as to provide an API to opt out of the automatic embedding of hidden request forgery protection tokens for certain forms.
  • Don't page cache: evidently this is not a serious option.
  • Don't put a search form, or any other form in fact, on pages which might be cached: again, obviously not a serious option.
  • Instead of embedding HTML forms in pages which might be cached, dynamically insert the forms via JavaScript instead: not a very attractive option because it means that browsers without JavaScript will completely miss out on the forms rather than degrading gracefully.
  • Don't use the Rails form helpers to make forms which might end up getting page cached; use hand-written HTML instead.

The quickest solution in the short term is just to hand-code the search form. But in the long term I need to be very careful not to inadvertently run into this problem on other pages which get page-cached. Basically any page with a comment form on it can't be page-cached, for instance.

Comments

  1. Greg Hurrell

    Status changed:

    • From: New
    • To: Open
  2. Greg Hurrell

    Ok, I've fixed this issue in the application layout. Will be up for the next deployment (and seeing as we get a new public folder for every deployment all the old cached pages with tokens embedded in them will become inaccessible).

    For now the only other thing I can do is be very careful about cached pages with forms on them. As far as I can tell right now, the search form is the only culprit for now.

  3. Greg Hurrell

    Status changed:

    • From: Open
    • To: Closed
  4. Greg Hurrell

    I think the way to handle comment forms on page-cached pages will be to have a "Comment" button which retrieves the comment form via AJAX, or when gracefully degrading will instead take the user to a new (not-cached) page (comments#new or some such).

  5. anonymous

    Hi Wincent

    I would agree there is some strong correlation between page caching and static pages with forms on them using Rails helpers.

    I inadvertently faced the InvalidAuthenticityToken problem when I introduced page caching into the welcome page of my app. And my app has 2 forms on the welcome page - 1 for login, 1 for signup. With just the few lines of caching code, my app suddenly broke. Commenting out the protect_from_forgery line got it working. With protect_from_forgery, I had to remove page caching and I also had to manually delete the cached 'index' page in the public folder.

    I was wondering - what if protect_from_forgery is not commented in application_controller.rb while simultaneously declaring a protect_from_forgery filter with :except => [:new_user, :login] - basically the two actions hit by forms pertaining to signing in and login.

    Would this work ? Is there a flaw I do not see deeper down in the code or controller/view interactions ?

    Looking forward to any input on this.

    Rajive

  6. Greg Hurrell

    I don't know if it would work but it certainly might seeing as many other such controller-level declarations are overridable in subclasses.

    As I commented above, the solution I went with was to hand-code the form rather than use a Rails form helper such as form_for or similar. This was the easiest/cleanest approach because we were just talking about a search form which is "idempotent" so doesn't really require request from forgery protection anyway.

    I'm using Haml, so the form looks something like this (but you could do the same in ERB or any other templating markup): 

    %form{:id => 'search', :action => search_index_path, :method => 'post'}
  =text_field_tag :query, params[:query], :size => 20, :id => :search_box
Add a comment

Comments are now closed for this issue.

Menu