Merge branch 'dir-traversal' into 'master'

Fix directory traversal vulnerabilities

Fixes gitlab/gitlab-ee#272.

As @joern mentions:

> This is not exploitable via the front-end nginx. But nevertheless this issue should be addressed.

See merge request !1760

3 files changed, 38 insertions, 5 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 93dfa28d974..0ecde5ef89c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,8 @@ v 7.10.0 (unreleased)
   - Fix broken file browsing with a submodule that contains a relative link (Stan Hu)
   - Fix persistent XSS vulnerability around profile website URLs.
   - Fix project import URL regex to prevent arbitary local repos from being imported.
+  - Fix directory traversal vulnerability around uploads routes.
+  - Fix directory traversal vulnerability around help pages.
   - 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/app/controllers/help_controller.rb b/app/controllers/help_controller.rb
index fbd9e67e6df..0e5567c7734 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,35 @@ class HelpController < ApplicationController
 
   def ui
   end
+
+  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)
+
+    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 == '.'
+
+      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
diff --git a/config/routes.rb b/config/routes.rb
index de21f418329..63eb7d0fe85 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
@@ -487,7 +487,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