Merge branch 'uploads-700' into 'master'

Restrict permissions on public/uploads

Based on https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/631

See merge request !2764

3 files changed, 11 insertions, 8 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 49e4b694399..9661600458a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -7,6 +7,7 @@ v 8.5.1
   - Issues can now be dragged & dropped into empty milestone lists. This is also
     possible with MRs
   - Fix an issue where MRs weren't sortable
+  - Restrict permissions on public/uploads
 
 v 8.5.0
   - Fix duplicate "me" in tooltip of the "thumbsup" awards Emoji (Stan Hu)
diff --git a/doc/install/installation.md b/doc/install/installation.md
index 096783ec38b..10f036d0d44 100644
--- a/doc/install/installation.md
+++ b/doc/install/installation.md
@@ -265,8 +265,9 @@ sudo usermod -aG redis git
     # Create the public/uploads/ directory
     sudo -u git -H mkdir public/uploads/
 
-    # Make sure GitLab can write to the public/uploads/ directory
-    sudo chmod -R u+rwX  public/uploads
+    # Make sure only the GitLab user has access to the public/uploads/ directory
+    # now that files in public/uploads are served by gitlab-workhorse
+    sudo chmod 0700 public/uploads
 
     # Change the permissions of the directory where CI build traces are stored
     sudo chmod -R u+rwX builds/
diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake
index 81099cb8ba9..d59872dc3a2 100644
--- a/lib/tasks/gitlab/check.rake
+++ b/lib/tasks/gitlab/check.rake
@@ -266,7 +266,7 @@ namespace :gitlab do
       unless File.directory?(Rails.root.join('public/uploads'))
         puts "no".red
         try_fixing_it(
-          "sudo -u #{gitlab_user} mkdir -m 750 #{Rails.root}/public/uploads"
+          "sudo -u #{gitlab_user} mkdir #{Rails.root}/public/uploads"
         )
         for_more_information(
           see_installation_guide_section "GitLab"
@@ -278,21 +278,22 @@ namespace :gitlab do
       upload_path = File.realpath(Rails.root.join('public/uploads'))
       upload_path_tmp = File.join(upload_path, 'tmp')
 
-      if File.stat(upload_path).mode == 040750
+      if File.stat(upload_path).mode == 040700
         unless Dir.exists?(upload_path_tmp)
           puts 'skipped (no tmp uploads folder yet)'.magenta
           return
         end
 
-        # if tmp upload dir has incorrect permissions, assume others do as well
-        if File.stat(upload_path_tmp).mode == 040755 && File.owned?(upload_path_tmp) # verify drwxr-xr-x permissions
+        # If tmp upload dir has incorrect permissions, assume others do as well
+        # Verify drwx------ permissions
+        if File.stat(upload_path_tmp).mode == 040700 && File.owned?(upload_path_tmp)
           puts "yes".green
         else
           puts "no".red
           try_fixing_it(
             "sudo chown -R #{gitlab_user} #{upload_path}",
             "sudo find #{upload_path} -type f -exec chmod 0644 {} \\;",
-            "sudo find #{upload_path} -type d -not -path #{upload_path} -exec chmod 0755 {} \\;"
+            "sudo find #{upload_path} -type d -not -path #{upload_path} -exec chmod 0700 {} \\;"
           )
           for_more_information(
             see_installation_guide_section "GitLab"
@@ -302,7 +303,7 @@ namespace :gitlab do
       else
         puts "no".red
         try_fixing_it(
-          "sudo chmod 0750 #{upload_path}",
+          "sudo find #{upload_path} -type d -not -path #{upload_path} -exec chmod 0700 {} \\;"
         )
         for_more_information(
           see_installation_guide_section "GitLab"