Merge branch 'upload-xss-access-control' into 'master'

Fix note attachments XSS and access control

Replaces the reverted #1528, as proposed in https://gitlab.com/gitlab-org/omnibus-gitlab/issues/434, as discussed with @dzaporozhets and as summarized in #2032.

@marin Could you take a look at the nginx config and apply it to Omnibus once this gets merged?

See merge request !1553

10 files changed, 136 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 7e6b0dff266..6e9d2dafad4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,5 @@
 v 7.8.0 (unreleased)
+  - Fix access control and protection against XSS for note attachments and other uploads.
   - Replace highlight.js with rouge-fork rugments (Stefan Tatschner)
   - Make project search case insensitive (Hannes Rosenögger)
   - Include issue/mr participants in list of recipients for reassign/close/reopen emails
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
new file mode 100644
index 00000000000..2b4da35bc7f
--- /dev/null
+++ b/app/controllers/projects/uploads_controller.rb
@@ -0,0 +1,19 @@
+class Projects::UploadsController < Projects::ApplicationController
+  layout "project"
+
+  before_filter :project
+
+  def show
+    path = File.join(project.path_with_namespace, params[:secret])
+    uploader = FileUploader.new('uploads', path)
+
+    uploader.retrieve_from_store!(params[:filename])
+
+    if uploader.file.exists?
+      # Right now, these are always images, so we can safely render them inline.
+      send_file uploader.file.path, disposition: 'inline'
+    else
+      not_found!
+    end
+  end
+end
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
new file mode 100644
index 00000000000..d5877977258
--- /dev/null
+++ b/app/controllers/uploads_controller.rb
@@ -0,0 +1,17 @@
+class UploadsController < ApplicationController
+  def show
+    model = params[:model].camelize.constantize.find(params[:id])
+    uploader = model.send(params[:mounted_as])
+
+    if uploader.file_storage?
+      if !model.respond_to?(:project) || can?(current_user, :read_project, model.project)
+        disposition = uploader.image? ? 'inline' : 'attachment'
+        send_file uploader.file.path, disposition: disposition
+      else
+        not_found!
+      end
+    else
+      redirect_to uploader.url
+    end
+  end
+end
diff --git a/config/initializers/static_files.rb b/config/initializers/static_files.rb
new file mode 100644
index 00000000000..bc4fe14bc1a
--- /dev/null
+++ b/config/initializers/static_files.rb
@@ -0,0 +1,18 @@
+begin
+  app = Rails.application
+
+  # The `ActionDispatch::Static` middleware intercepts requests for static files 
+  # by checking if they exist in the `/public` directory. 
+  # We're replacing it with our `Gitlab::Middleware::Static` that does the same,
+  # except ignoring `/uploads`, letting those go through to the GitLab Rails app.
+
+  app.config.middleware.swap(
+    ActionDispatch::Static, 
+    Gitlab::Middleware::Static, 
+    app.paths["public"].first, 
+    app.config.static_cache_control
+  )
+rescue
+  # If ActionDispatch::Static wasn't loaded onto the stack (like in production), 
+  # an exception is raised.
+end
diff --git a/config/routes.rb b/config/routes.rb
index 101c5f3c362..a6ffd28b2ea 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -75,7 +75,21 @@ Gitlab::Application.routes.draw do
     end
   end
 
+  #
+  # Uploads
+  #
 
+  scope path: :uploads 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: /.+/ }
+
+    # Project markdown uploads
+    get ":id/:secret/:filename", 
+        to:           "projects/uploads#show", 
+        constraints:  { id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/, filename: /.+/ }
+  end
 
   #
   # Explore area
diff --git a/doc/update/6.x-or-7.x-to-7.8.md b/doc/update/6.x-or-7.x-to-7.8.md
index 90d889d5113..859f4c1a6d6 100644
--- a/doc/update/6.x-or-7.x-to-7.8.md
+++ b/doc/update/6.x-or-7.x-to-7.8.md
@@ -164,8 +164,6 @@ git diff 6-0-stable:config/gitlab.yml.example 7-8-stable:config/gitlab.yml.examp
 * Make `/home/git/gitlab/config/gitlab.yml` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stable/config/gitlab.yml.example but with your settings.
 * Make `/home/git/gitlab/config/unicorn.rb` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stable/config/unicorn.rb.example but with your settings.
 * Make `/home/git/gitlab-shell/config.yml` the same as https://gitlab.com/gitlab-org/gitlab-shell/blob/v2.4.3/config.yml.example but with your settings.
-* HTTP setups: Make `/etc/nginx/sites-available/gitlab` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stable/lib/support/nginx/gitlab but with your settings.
-* HTTPS setups: Make `/etc/nginx/sites-available/gitlab-ssl` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stablef/lib/support/nginx/gitlab-ssl but with your settings.
 * Copy rack attack middleware config
 
 ```bash
@@ -178,6 +176,12 @@ sudo -u git -H cp config/initializers/rack_attack.rb.example config/initializers
 sudo cp lib/support/logrotate/gitlab /etc/logrotate.d/gitlab
 ```
 
+### Change Nginx settings
+
+* HTTP setups: Make `/etc/nginx/sites-available/gitlab` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stable/lib/support/nginx/gitlab but with your settings.
+* HTTPS setups: Make `/etc/nginx/sites-available/gitlab-ssl` the same as https://gitlab.com/gitlab-org/gitlab-ce/blob/7-8-stablef/lib/support/nginx/gitlab-ssl but with your settings.
+* A new `location /uploads/` section has been added that needs to have the same content as the existing `location @gitlab` section.
+
 ## 9. Start application
 
     sudo service gitlab start
diff --git a/doc/update/7.7-to-7.8.md b/doc/update/7.7-to-7.8.md
index 01b4fc4c992..7ca0fe65785 100644
--- a/doc/update/7.7-to-7.8.md
+++ b/doc/update/7.7-to-7.8.md
@@ -75,8 +75,9 @@ git diff origin/7-6-stable:config/gitlab.yml.example origin/7-8-stable:config/gi
 
 #### Change Nginx settings
 
-* HTTP setups: Make `/etc/nginx/sites-available/gitlab` the same as [`lib/support/nginx/gitlab`](/lib/support/nginx/gitlab) but with your settings
-* HTTPS setups: Make `/etc/nginx/sites-available/gitlab-ssl` the same as [`lib/support/nginx/gitlab-ssl`](/lib/support/nginx/gitlab-ssl) but with your setting
+* HTTP setups: Make `/etc/nginx/sites-available/gitlab` the same as [`lib/support/nginx/gitlab`](/lib/support/nginx/gitlab) but with your settings.
+* HTTPS setups: Make `/etc/nginx/sites-available/gitlab-ssl` the same as [`lib/support/nginx/gitlab-ssl`](/lib/support/nginx/gitlab-ssl) but with your settings.
+* A new `location /uploads/` section has been added that needs to have the same content as the existing `location @gitlab` section.
 
 #### Setup time zone (optional)
 
diff --git a/lib/gitlab/middleware/static.rb b/lib/gitlab/middleware/static.rb
new file mode 100644
index 00000000000..85ffa8aca68
--- /dev/null
+++ b/lib/gitlab/middleware/static.rb
@@ -0,0 +1,13 @@
+module Gitlab
+  module Middleware
+    class Static < ActionDispatch::Static
+      UPLOADS_REGEX = /\A\/uploads(\/|\z)/.freeze
+
+      def call(env)
+        return @app.call(env) if env['PATH_INFO'] =~ UPLOADS_REGEX
+
+        super
+      end
+    end
+  end
+end
diff --git a/lib/support/nginx/gitlab b/lib/support/nginx/gitlab
index c8b769ace8e..62a4276536c 100644
--- a/lib/support/nginx/gitlab
+++ b/lib/support/nginx/gitlab
@@ -1,5 +1,5 @@
 ## GitLab
-## Contributors: randx, yin8086, sashkab, orkoden, axilleas, bbodenmiller
+## Contributors: randx, yin8086, sashkab, orkoden, axilleas, bbodenmiller, DouweM
 ##
 ## Lines starting with two hashes (##) are comments with information.
 ## Lines starting with one hash (#) are configuration parameters that can be uncommented.
@@ -56,6 +56,27 @@ server {
     try_files $uri $uri/index.html $uri.html @gitlab;
   }
 
+  ## We route uploads through GitLab to prevent XSS and enforce access control.
+  location /uploads/ {
+    ## If you use HTTPS make sure you disable gzip compression
+    ## to be safe against BREACH attack.
+    # gzip off;
+
+    ## https://github.com/gitlabhq/gitlabhq/issues/694
+    ## Some requests take more than 30 seconds.
+    proxy_read_timeout      300;
+    proxy_connect_timeout   300;
+    proxy_redirect          off;
+
+    proxy_set_header    Host                $http_host;
+    proxy_set_header    X-Real-IP           $remote_addr;
+    proxy_set_header    X-Forwarded-For     $proxy_add_x_forwarded_for;
+    proxy_set_header    X-Forwarded-Proto   $scheme;
+    proxy_set_header    X-Frame-Options     SAMEORIGIN;
+
+    proxy_pass http://gitlab;
+  }
+
   ## If a file, which is not found in the root folder is requested,
   ## then the proxy passes the request to the upsteam (gitlab unicorn).
   location @gitlab {
diff --git a/lib/support/nginx/gitlab-ssl b/lib/support/nginx/gitlab-ssl
index 19af010a9f7..2aefc944698 100644
--- a/lib/support/nginx/gitlab-ssl
+++ b/lib/support/nginx/gitlab-ssl
@@ -1,5 +1,5 @@
 ## GitLab
-## Contributors: randx, yin8086, sashkab, orkoden, axilleas, bbodenmiller
+## Contributors: randx, yin8086, sashkab, orkoden, axilleas, bbodenmiller, DouweM
 ##
 ## Modified from nginx http version
 ## Modified from http://blog.phusion.nl/2012/04/21/tutorial-setting-up-gitlab-on-debian-6/
@@ -101,6 +101,28 @@ server {
     try_files $uri $uri/index.html $uri.html @gitlab;
   }
 
+  ## We route uploads through GitLab to prevent XSS and enforce access control.
+  location /uploads/ {
+    ## If you use HTTPS make sure you disable gzip compression
+    ## to be safe against BREACH attack.
+    gzip off;
+
+    ## https://github.com/gitlabhq/gitlabhq/issues/694
+    ## Some requests take more than 30 seconds.
+    proxy_read_timeout      300;
+    proxy_connect_timeout   300;
+    proxy_redirect          off;
+
+    proxy_set_header    Host                $http_host;
+    proxy_set_header    X-Real-IP           $remote_addr;
+    proxy_set_header    X-Forwarded-Ssl     on;
+    proxy_set_header    X-Forwarded-For     $proxy_add_x_forwarded_for;
+    proxy_set_header    X-Forwarded-Proto   $scheme;
+    proxy_set_header    X-Frame-Options     SAMEORIGIN;
+
+    proxy_pass http://gitlab;
+  }
+
   ## If a file, which is not found in the root folder is requested,
   ## then the proxy passes the request to the upsteam (gitlab unicorn).
   location @gitlab {