From 93133f4da9d950f47ca0ba2e437f9c004567e6f7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 10 Apr 2015 18:07:31 +0200 Subject: Fix directory traversal vulnerability around uploads routes. --- CHANGELOG | 2 ++ config/routes.rb | 8 ++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 0878c03207b..a126850c184 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.10.0 (unreleased) + - Fix directory traversal vulnerability around uploads routes. + - Fix broken file browsing with a submodule that contains a relative link (Stan Hu) - Fix bug where Wiki pages that included a '/' were no longer accessible (Stan Hu) - Fix bug where error messages from Dropzone would not be displayed on the issues page (Stan Hu) - Add ability to configure Reply-To address in gitlab.yml (Stan Hu) diff --git a/config/routes.rb b/config/routes.rb index c1b85b025b5..29207ed4d9b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -91,18 +91,18 @@ Gitlab::Application.routes.draw do # Note attachments and User/Group/Project avatars get ":model/:mounted_as/:id/:filename", to: "uploads#show", - constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /.+/ } + constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ } # Project markdown uploads get ":namespace_id/:project_id/:secret/:filename", to: "projects/uploads#show", - constraints: { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: /.+/ } + constraints: { namespace_id: /[a-zA-Z.0-9_\-]+/, project_id: /[a-zA-Z.0-9_\-]+/, filename: /[^\/]+/ } end # Redirect old note attachments path to new uploads path. get "files/note/:id/:filename", to: redirect("uploads/note/attachment/%{id}/%{filename}"), - constraints: { filename: /.+/ } + constraints: { filename: /[^\/]+/ } # # Explore area @@ -485,7 +485,7 @@ Gitlab::Application.routes.draw do resources :uploads, only: [:create] do collection do - get ":secret/:filename", action: :show, as: :show, constraints: { filename: /.+/ } + get ":secret/:filename", action: :show, as: :show, constraints: { filename: /[^\/]+/ } end end end -- cgit v1.2.1 From edd05fc48c748ba0945652c8c83e7617ccc029fc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 10 Apr 2015 18:16:46 +0200 Subject: Fix directory traversal vulnerability around help pages. --- CHANGELOG | 1 + app/controllers/help_controller.rb | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index a126850c184..9f838ede787 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.10.0 (unreleased) - Fix directory traversal vulnerability around uploads routes. + - Fix directory traversal vulnerability around help pages. - Fix broken file browsing with a submodule that contains a relative link (Stan Hu) - Fix bug where Wiki pages that included a '/' were no longer accessible (Stan Hu) - Fix bug where error messages from Dropzone would not be displayed on the issues page (Stan Hu) diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index fbd9e67e6df..0010caad773 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -3,7 +3,7 @@ class HelpController < ApplicationController end def show - @filepath = params[:filepath] + @filepath = clean_path_info(params[:filepath]) @format = params[:format] respond_to do |format| @@ -36,4 +36,22 @@ class HelpController < ApplicationController def ui end + + # Taken from ActionDispatch::FileHandler + PATH_SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) + + def clean_path_info(path_info) + parts = path_info.split PATH_SEPS + + clean = [] + + parts.each do |part| + next if part.empty? || part == '.' + part == '..' ? clean.pop : clean << part + end + + clean.unshift '/' if parts.empty? || parts.first.empty? + + ::File.join(*clean) + end end -- cgit v1.2.1 From 988b703548a87f4c9d5d25eb767046a2e39069d7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 11 Apr 2015 11:40:28 +0200 Subject: Fix a whoopsie daisy in the changelog. --- CHANGELOG | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 9f838ede787..fdd0a4466cc 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,7 +3,6 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.10.0 (unreleased) - Fix directory traversal vulnerability around uploads routes. - Fix directory traversal vulnerability around help pages. - - Fix broken file browsing with a submodule that contains a relative link (Stan Hu) - Fix bug where Wiki pages that included a '/' were no longer accessible (Stan Hu) - Fix bug where error messages from Dropzone would not be displayed on the issues page (Stan Hu) - Add ability to configure Reply-To address in gitlab.yml (Stan Hu) -- cgit v1.2.1 From 5e2f25c32ee36ed5a4ad137c299b60d91b7ebdeb Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 14 Apr 2015 13:07:15 +0200 Subject: Add explanation to HelpController#clean_path_info. --- app/controllers/help_controller.rb | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index 0010caad773..0e5567c7734 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -37,21 +37,34 @@ class HelpController < ApplicationController def ui end - # Taken from ActionDispatch::FileHandler PATH_SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) + # Taken from ActionDispatch::FileHandler + # Cleans up the path, to prevent directory traversal outside the doc folder. def clean_path_info(path_info) - parts = path_info.split PATH_SEPS + parts = path_info.split(PATH_SEPS) clean = [] + # Walk over each part of the path parts.each do |part| + # Turn `one//two` or `one/./two` into `one/two`. next if part.empty? || part == '.' - part == '..' ? clean.pop : clean << part + + if part == '..' + # Turn `one/two/../` into `one` + clean.pop + else + # Add simple folder names to the clean path. + clean << part + end end + # If the path was an absolute path (i.e. `/` or `/one/two`), + # add `/` to the front of the clean path. clean.unshift '/' if parts.empty? || parts.first.empty? + # Join all the clean path parts by the path separator. ::File.join(*clean) end end -- cgit v1.2.1