SECURITY: Require POST with CSRF token for OmniAuth request phase

SECURITY: Require POST with CSRF token for OmniAuth request phase

diff --git a/app/assets/javascripts/discourse/lib/ajax.js.es6 b/app/assets/javascripts/discourse/lib/ajax.js.es6
index e4ca8df..1cec514 100644
--- a/app/assets/javascripts/discourse/lib/ajax.js.es6
+++ b/app/assets/javascripts/discourse/lib/ajax.js.es6
@@ -40,6 +40,12 @@ function handleRedirect(data) {
   }
 }
 
+export function updateCsrfToken() {
+  return ajax("/session/csrf").then(result => {
+    Discourse.Session.currentProp("csrfToken", result.csrf);
+  });
+}
+
 /**
   Our own $.ajax method. Makes sure the .then method executes in an Ember runloop
   for performance reasons. Also automatically adjusts the URL to support installs
@@ -157,10 +163,7 @@ export function ajax() {
     !Discourse.Session.currentProp("csrfToken")
   ) {
     promise = new Ember.RSVP.Promise((resolve, reject) => {
-      ajaxObj = $.ajax(Discourse.getURL("/session/csrf"), {
-        cache: false
-      }).done(result => {
-        Discourse.Session.currentProp("csrfToken", result.csrf);
+      ajaxObj = updateCsrfToken().then(() => {
         performAjax(resolve, reject);
       });
     });
diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6
index a916663..e00bd96 100644
--- a/app/assets/javascripts/discourse/models/login-method.js.es6
+++ b/app/assets/javascripts/discourse/models/login-method.js.es6
@@ -1,4 +1,5 @@
 import computed from "ember-addons/ember-computed-decorators";
+import { updateCsrfToken } from "discourse/lib/ajax";
 
 const LoginMethod = Ember.Object.extend({
   @computed
@@ -30,8 +31,10 @@ const LoginMethod = Ember.Object.extend({
       }
 
       if (reconnect || fullScreenLogin || this.full_screen_login) {
-        document.cookie = "fsl=true";
-        window.location = authUrl;
+        LoginMethod.buildPostForm(authUrl).then(form => {
+          document.cookie = "fsl=true";
+          form.submit();
+        });
       } else {
         this.set("authenticate", name);
         const left = this.lastX - 400;
@@ -44,31 +47,51 @@ const LoginMethod = Ember.Object.extend({
           authUrl += authUrl.includes("?") ? "&" : "?";
           authUrl += "display=popup";
         }
-
-        const windowState = window.open(
-          authUrl,
-          "_blank",
-          "menubar=no,status=no,height=" +
-            height +
-            ",width=" +
-            width +
-            ",left=" +
-            left +
-            ",top=" +
-            top
-        );
-
-        const timer = setInterval(() => {
-          if (!windowState || windowState.closed) {
-            clearInterval(timer);
-            this.set("authenticate", null);
-          }
-        }, 1000);
+        LoginMethod.buildPostForm(authUrl).then(form => {
+          const windowState = window.open(
+            "about:blank",
+            "auth_popup",
+            `menubar=no,status=no,height=${height},width=${width},left=${left},top=${top}`
+          );
+
+          form.target = "auth_popup";
+          form.submit();
+
+          const timer = setInterval(() => {
+            // If the process is aborted, reset state in this window
+            if (!windowState || windowState.closed) {
+              clearInterval(timer);
+              this.set("authenticate", null);
+            }
+          }, 1000);
+        });
       }
     }
   }
 });
 
+LoginMethod.reopenClass({
+  buildPostForm(url) {
+    // Login always happens in an anonymous context, with no CSRF token
+    // So we need to fetch it before sending a POST request
+    return updateCsrfToken().then(() => {
+      const form = document.createElement("form");
+      form.setAttribute("style", "display:none;");
+      form.setAttribute("method", "post");
+      form.setAttribute("action", url);
+
+      const input = document.createElement("input");
+      input.setAttribute("name", "authenticity_token");
+      input.setAttribute("value", Discourse.Session.currentProp("csrfToken"));
+      form.appendChild(input);
+
+      document.body.appendChild(form);
+
+      return form;
+    });
+  }
+});
+
 let methods;
 
 export function findAll() {
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index b23cc59..c35ae5a 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -18,6 +18,10 @@ class Users::OmniauthCallbacksController < ApplicationController
   # will not have a CSRF token, however the payload is all validated so its safe
   skip_before_action :verify_authenticity_token, only: :complete
 
+  def confirm_request
+    self.class.find_authenticator(params[:provider])
+  end
+
   def complete
     auth = request.env["omniauth.auth"]
     raise Discourse::NotFound unless request.env["omniauth.auth"]
diff --git a/app/views/users/omniauth_callbacks/confirm_request.html.erb b/app/views/users/omniauth_callbacks/confirm_request.html.erb
new file mode 100644
index 0000000..6abfe3c
--- /dev/null
+++ b/app/views/users/omniauth_callbacks/confirm_request.html.erb
@@ -0,0 +1,11 @@
+<div id='simple-container'>
+  <h2><%= t('login.omniauth_confirm_title', provider:(t "js.login.#{params[:provider]}.name")) %></h2>
+  <br/>
+
+  <%= form_tag do %>
+    <input type="hidden" name="full_screen_login" value="true">
+    <%= button_tag(type: "submit", class: "btn btn-primary") do %>
+        <%= SvgSprite.raw_svg('fa-plug') %><%= t 'login.omniauth_confirm_button' %>
+    <% end %>
+  <% end %>
+</div>
diff --git a/config/initializers/009-omniauth.rb b/config/initializers/009-omniauth.rb
index 6acc285..415e3b3 100644
--- a/config/initializers/009-omniauth.rb
+++ b/config/initializers/009-omniauth.rb
@@ -7,3 +7,4 @@ require "middleware/omniauth_bypass_middleware"
 Rails.application.config.middleware.use Middleware::OmniauthBypassMiddleware
 
 OmniAuth.config.logger = Rails.logger
+OmniAuth.config.allowed_request_methods = [:post]
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index ba940c7..5c4d884 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2226,6 +2226,8 @@ en:
     something_already_taken: "Something went wrong, perhaps the username or email is already registered. Try the forgot password link."
     omniauth_error: "Sorry, there was an error authorizing your account. Perhaps you did not approve authorization?"
     omniauth_error_unknown: "Something went wrong processing your log in, please try again."
+    omniauth_confirm_title: "Log in using %{provider}"
+    omniauth_confirm_button: "Continue"
     authenticator_error_no_valid_email: "No email addresses associated with %{account} are allowed. You may need to configure your account with a different email address."
     new_registrations_disabled: "New account registrations are not allowed at this time."
     password_too_long: "Passwords are limited to 200 characters."
diff --git a/config/routes.rb b/config/routes.rb
index 68b1b00..fb0cda2 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -599,6 +599,7 @@ Discourse::Application.routes.draw do
     end
   end
 
+  get "/auth/:provider", to: "users/omniauth_callbacks#confirm_request"
   match "/auth/:provider/callback", to: "users/omniauth_callbacks#complete", via: [:get, :post]
   match "/auth/failure", to: "users/omniauth_callbacks#failure", via: [:get, :post]
   get "/associate/:token", to: "users/associate_accounts#connect_info", constraints: { token: /\h{32}/ }
diff --git a/lib/csrf_token_verifier.rb b/lib/csrf_token_verifier.rb
new file mode 100644
index 0000000..1c18424
--- /dev/null
+++ b/lib/csrf_token_verifier.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+# Provides a way to check a CSRF token outside of a controller
+class CSRFTokenVerifier
+  include ActiveSupport::Configurable
+  include ActionController::RequestForgeryProtection
+
+  # Use config from ActionController::Base
+  config.each_key do |configuration_name|
+    undef_method configuration_name

[... diff too long, it was truncated ...]

GitHub sha: 3b8c4688

1 Like

When will this hit stable?

This is a security enhancement, rather than a patch for a specific exploit. It introduces a backwards incompatible change to the authentication endpoints, and therefore it will not be backported to stable.

1 Like

It introduces a backwards incompatible change to the authentication endpoints

Where can we read more about this? I suspect this change has broken our SAML setup.

All the information on the change is here in the commit. If you would like to discuss it in more detail, please feel free to open a support topic on https://meta.discourse.org/

1 Like