Added random salt and hashing to oauth state parameter

This ensures that content of state is generated by CI, but doesn't prevent replay attacks on state parameter.

3 files changed, 106 insertions, 2 deletions

diff --git a/app/controllers/user_sessions_controller.rb b/app/controllers/user_sessions_controller.rb
index 2f972bc..e486b24 100644
--- a/app/controllers/user_sessions_controller.rb
+++ b/app/controllers/user_sessions_controller.rb
@@ -1,4 +1,6 @@
 class UserSessionsController < ApplicationController
+  include UserSessionsHelper
+
   before_filter :authenticate_user!, except: [:new, :callback, :auth]
 
   def show
@@ -11,18 +13,24 @@ class UserSessionsController < ApplicationController
   def auth
     redirect_to client.auth_code.authorize_url({
       redirect_uri: callback_user_sessions_url,
-      state: params[:return_to]
+      state: generate_oauth_state(params[:return_to])
     })
   end
 
   def callback
+    unless is_oauth_state_valid?(params[:state])
+      redirect_to new_user_sessions_path
+      return
+    end
+
     token = client.auth_code.get_token(params[:code], redirect_uri: callback_user_sessions_url).token
     
     @user_session = UserSession.new
     user = @user_session.authenticate(access_token: token)
 
     if user && sign_in(user)
-      redirect_to(params[:state] || root_path)
+      return_to = get_ouath_state_return_to(params[:state])
+      redirect_to(return_to || root_path)
     else
       @error = 'Invalid credentials'
       render :new
diff --git a/app/helpers/user_sessions_helper.rb b/app/helpers/user_sessions_helper.rb
index 2018402..e5853b5 100644
--- a/app/helpers/user_sessions_helper.rb
+++ b/app/helpers/user_sessions_helper.rb
@@ -1,2 +1,29 @@
 module UserSessionsHelper
+  def generate_oauth_salt
+    SecureRandom.hex(16)
+  end
+
+  def generate_oauth_secret(salt, return_to)
+    return unless return_to
+    message = GitlabCi::Application.config.secret_key_base + salt + return_to
+    Digest::SHA256.hexdigest message
+  end
+
+  def generate_oauth_state(return_to)
+    return unless return_to
+    salt = generate_oauth_salt
+    secret = generate_oauth_secret(salt, return_to)
+    "#{salt}:#{secret}:#{return_to}"
+  end
+
+  def get_ouath_state_return_to(state)
+    state.split(':', 3)[2] if state
+  end
+
+  def is_oauth_state_valid?(state)
+    return true unless state
+    salt, secret, return_to = state.split(':', 3)
+    return false unless return_to
+    secret == generate_oauth_secret(salt, return_to)
+  end
 end
diff --git a/spec/helpers/user_sessions_helper_spec.rb b/spec/helpers/user_sessions_helper_spec.rb
new file mode 100644
index 0000000..1fbbad4
--- /dev/null
+++ b/spec/helpers/user_sessions_helper_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe UserSessionsHelper do
+  describe :generate_oauth_secret do
+    let (:salt) { "a" }
+    let (:salt2) { "b" }
+    let (:return_to) { "b" }
+
+    it "should return null if return_to is also null" do
+      generate_oauth_secret(salt, nil).should be_nil
+    end
+
+    it "should return not null if return_to is also not null" do
+      generate_oauth_secret(salt, return_to).should_not be_nil
+    end
+
+    it "should return different secrets for different salts" do
+      secret1 = generate_oauth_secret(salt, return_to)
+      secret2 = generate_oauth_secret(salt, return_to)
+      secret1.should eq(secret2)
+    end
+  end
+
+  describe :generate_oauth_state do
+    let (:return_to) { "b" }
+
+    it "should return null if return_to is also null" do
+      generate_oauth_state(nil).should be_nil
+    end
+
+    it "should return two different states for same return_to" do
+      state1 = generate_oauth_state(return_to)
+      state2 = generate_oauth_state(return_to)
+      state1.should_not eq(state2)
+    end
+  end
+
+  describe :get_ouath_state_return_to do
+    let (:return_to) { "a" }
+    let (:state) { generate_oauth_state(return_to) }
+
+    it "should return return_to" do
+      get_ouath_state_return_to(state).should eq(return_to)
+    end
+  end
+
+  describe :is_oauth_state_valid? do
+    let (:return_to) { "a" }
+    let (:state) { generate_oauth_state(return_to) }
+    let (:forged) { "forged#{state}" }
+    let (:invalid) { "aa" }
+    let (:invalid2) { "aa:bb" }
+    let (:invalid3) { "aa:bb:" }
+
+    it "should validate oauth state" do
+      is_oauth_state_valid?(state).should be_true
+    end
+
+    it "should not validate forged state" do
+      is_oauth_state_valid?(forged).should be_false
+    end
+
+    it "should not validate invalid state" do
+      is_oauth_state_valid?(invalid).should be_false
+      is_oauth_state_valid?(invalid2).should be_false
+      is_oauth_state_valid?(invalid3).should be_false
+    end
+  end
+end