Merge branch 'secure-oauth-state' into 'master'

Added random salt and hashing to oauth state parameter

This ensures signs state parameter. The generated state is built like this:
```
salt = random_hex(16bytes)
secret = sha256_hex(gitlab_ci_secret + salt + return_to)
state = "salt:secret:return_to"
```

This prevents from faking the state and forcing redirect to provided URL. However this doesn't prevent replay attacks if you know the valid `state` parameter for specific `return_to`. Should we be concerned about it?

/cc @vsizov @jacobvosmaer

See merge request !192

6 files changed, 115 insertions, 5 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9852736..0d27134 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -1,4 +1,6 @@
 class ApplicationController < ActionController::Base
+  include UserSessionsHelper
+
   rescue_from Network::UnauthorizedError, with: :invalid_token
   before_filter :default_headers
   before_filter :check_config
@@ -39,7 +41,7 @@ class ApplicationController < ActionController::Base
   def authenticate_public_page!
     unless project.public
       unless current_user
-        redirect_to(new_user_sessions_path(return_to: request.fullpath)) and return
+        redirect_to(new_user_sessions_path(state: generate_oauth_state(request.fullpath))) and return
       end
 
       unless current_user.can_access_project?(project.gitlab_id)
diff --git a/app/controllers/user_sessions_controller.rb b/app/controllers/user_sessions_controller.rb
index 2f972bc..23e4182 100644
--- a/app/controllers/user_sessions_controller.rb
+++ b/app/controllers/user_sessions_controller.rb
@@ -9,20 +9,31 @@ class UserSessionsController < ApplicationController
   end
 
   def auth
+    unless is_oauth_state_valid?(params[:state])
+      redirect_to new_user_sessions_path
+      return
+    end
+
     redirect_to client.auth_code.authorize_url({
       redirect_uri: callback_user_sessions_url,
-      state: params[:return_to]
+      state: params[:state]
     })
   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..df158c6 100644
--- a/app/helpers/user_sessions_helper.rb
+++ b/app/helpers/user_sessions_helper.rb
@@ -1,2 +1,30 @@
 module UserSessionsHelper
+  def generate_oauth_salt
+    SecureRandom.hex(16)
+  end
+
+  def generate_oauth_hmac(salt, return_to)
+    return unless return_to
+    digest = OpenSSL::Digest.new('sha256')
+    key = GitlabCi::Application.config.secret_key_base + salt
+    OpenSSL::HMAC.hexdigest(digest, key, return_to)
+  end
+
+  def generate_oauth_state(return_to)
+    return unless return_to
+    salt = generate_oauth_salt
+    hmac = generate_oauth_hmac(salt, return_to)
+    "#{salt}:#{hmac}:#{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, hmac, return_to = state.split(':', 3)
+    return false unless return_to
+    hmac == generate_oauth_hmac(salt, return_to)
+  end
 end
diff --git a/app/views/projects/_public.html.haml b/app/views/projects/_public.html.haml
index 11eccbd..9662cc9 100644
--- a/app/views/projects/_public.html.haml
+++ b/app/views/projects/_public.html.haml
@@ -3,7 +3,7 @@
     Public projects
 
 .bs-callout
-  = link_to new_user_sessions_path(return_to: request.fullpath) do
+  = link_to new_user_sessions_path(state: generate_oauth_state(request.fullpath)) do
     %strong Login with GitLab
   to see your private projects
 
diff --git a/app/views/user_sessions/new.html.haml b/app/views/user_sessions/new.html.haml
index b457e93..c5be95b 100644
--- a/app/views/user_sessions/new.html.haml
+++ b/app/views/user_sessions/new.html.haml
@@ -4,5 +4,5 @@
     Make sure you have account on GitLab server
     = link_to GitlabCi.config.gitlab_server.url, GitlabCi.config.gitlab_server.url, no_turbolink
   %hr
-  = link_to "Login with GitLab", auth_user_sessions_path(return_to: params[:return_to]), no_turbolink.merge( class: 'btn btn-login btn-success' )
+  = link_to "Login with GitLab", auth_user_sessions_path(state: params[:state]), no_turbolink.merge( class: 'btn btn-login btn-success' )
 
diff --git a/spec/helpers/user_sessions_helper_spec.rb b/spec/helpers/user_sessions_helper_spec.rb
new file mode 100644
index 0000000..a2ab1f1
--- /dev/null
+++ b/spec/helpers/user_sessions_helper_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe UserSessionsHelper do
+  describe :generate_oauth_hmac do
+    let (:salt) { 'a' }
+    let (:salt2) { 'b' }
+    let (:return_to) { 'b' }
+
+    it 'should return null if return_to is also null' do
+      generate_oauth_hmac(salt, nil).should be_nil
+    end
+
+    it 'should return not null if return_to is also not null' do
+      generate_oauth_hmac(salt, return_to).should_not be_nil
+    end
+
+    it 'should return different hmacs for different salts' do
+      secret1 = generate_oauth_hmac(salt, return_to)
+      secret2 = generate_oauth_hmac(salt2, return_to)
+      secret1.should_not 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