doorkeepr gem の CSRF 脆弱性が修正されてた #Ruby

doorkeepr gem に CSRF 脆弱性があり、いつの間にか修正されてました。

詳細は Pull Request に書いてあるとおりなのですが、

phillbaker commented 8 days ago

Since the Doorkeeper controllers inherit from Doorkeeper::Application (which inherits directly from ActionController::Base) and not ApplicationController, they never call protect_from_forgery, which means that non-GET methods don’t validate CSRF tokens. Thus, it’s possible for an attacker to host a form on an arbitrary URL, and if a users is logged into a site that uses Doorkeeper visits the URL, the attacker can grant access to a application on that site.

皆さんご存知の通り、普通 Rails でアプリを作成すると、以下のように ApplicationController 内で protect_from_forgery が呼ばれます。

app/controllers/application_controller.rb

class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception
end

protect_from_forgery を呼んでおくと、フォームや Ajax のリクエストに対して Rails が勝手に token をくっつけてくれて、GET 以外のリクエストの場合、この token が想定したものと一致しない限りはセッションがリセットされます。

アプリの各コントローラは ApplicationController を継承するため、通常は特に何も意識せずに CSRF 対策が行われます。

doorkeeper の場合

doorkeeper gem では、以下のように ActionController::Base を継承した独自の ApplicationController を定義し、さらに各コントローラでこれを継承しています。

doorkeeper/app/controllers/doorkeeper/application_controller.rb

module Doorkeeper
  class ApplicationController < ActionController::Base
    include Helpers::Controller

    helper 'doorkeeper/dashboard'
  end
end

よく見ると、ここで protect_from_forgery 呼んでないですね。

ということで、上記の Pull Request で以下の様な変更が入っています。

doorkeeper/app/controllers/doorkeeper/application_controller.rb

module Doorkeeper
  class ApplicationController < ActionController::Base
    include Helpers::Controller

    if ::Rails.version.to_i < 4
      protect_from_forgery
    else
      protect_from_forgery with: :exception
    end

    helper 'doorkeeper/dashboard'
  end
end

というわけで、みなさん早くバージョンアップしましょー。

protect_from_forgery 補足

protect_from_forgery のリファレンスに以下の記載があります。

protect_from_forgery(options = {})
Turn on request forgery protection. Bear in mind that only non-GET, HTML/JavaScript requests are checked.

protect_from_forgery でチェックされるのは GET 以外の、HTML/JavaScript からのリクエストだけだよー、ということです。

みなさん基本やってないと思いますが、改めて GET に対応したアクションでリソースの更新とかやっちゃだめってことですね。