Fix Uploads world readable

docs/roadmap.md

@@ -179,9 +179,9 @@ of its size, it takes work to keep it that way.
 
 * [Add support for 2-factor authentication on all
   platforms](https://github.com/zulip/zulip/pull/1747)
-* [Add support for stronger security controls for uploaded files (The
+* <strike>[Add support for stronger security controls for uploaded files (The
   LOCAL_UPLOADS_DIR file uploads backend only supports world-readable
-  uploads)](https://github.com/zulip/zulip/issues/320)
+  uploads)](https://github.com/zulip/zulip/issues/320)</strike>
 * [Fix requirement to set a password when creating account via
   Google](https://github.com/zulip/zulip/issues/1633)
 * [Add a retention policy feature that automatically deletes old

docs/security-model.md

@@ -184,24 +184,9 @@ your organization.
 * Zulip supports user-uploaded files; ideally they should be hosted
   from a separate domain from the main Zulip server to protect against
   various same-domain attacks (e.g. zulip-user-content.example.com)
-  using the S3 integration.
-
-  The URLs of user-uploaded files are secret; if you are using the
-  "local file upload" integration, anyone with the URL of an uploaded
-  file can access the file.  This means the local uploads integration
-  is vulnerable to a subtle attack where if a user clicks on a link in
-  a secret .PDF or .HTML file that had been uploaded to Zulip, access
-  to the file might be leaked to the other server via the Referrer
-  header (see [the "Uploads world readable" issue on
-  GitHub](https://github.com/zulip/zulip/issues/320)).
-
-  The Zulip S3 file upload integration is relatively safe against that
-  attack, because the URLs of files presented to users don't host the
-  content.  Instead, the S3 integration checks the user has a valid
-  Zulip session in the relevant realm, and if so then redirects the
-  browser to a one-time S3 URL that expires a short time later.
-  Keeping the URL secret is still important to avoid other users in
-  the Zulip realm from being able to access the file.
+  using the S3 integration. The uploaded files could be viewed by only
+  those users who have access to them. Simple possession of a URL to
+  the uploaded file doesn't qualify as a right to view such a file.
 
 * Zulip supports using the Camo image proxy to proxy content like
   inline image previews that can be inserted into the Zulip message

puppet/zulip/files/nginx/sites-available/zulip-enterprise

@@ -12,7 +12,8 @@ server {
     ssl_certificate /etc/ssl/certs/zulip.combined-chain.crt;
     ssl_certificate_key /etc/ssl/private/zulip.key;
 
-    location /user_uploads {
+    location /serve_uploads {
+        internal;
         add_header X-Content-Type-Options nosniff;
         include /etc/nginx/zulip-include/uploads.types;
         alias /home/zulip/uploads/files;

requirements/common.txt

@@ -186,3 +186,8 @@ ijson==2.3
 # Needed for link preview
 beautifulsoup4==4.5.3
 git+https://github.com/rafaelmartins/pyoembed.git@eb9901917c2a44b49e2887c077ead84a722c50dc#egg=pyoembed
+
+# Needed for serving uploaded files from nginx but perform auth checks in django.
+# We are using a commit from a PR https://github.com/johnsensible/django-sendfile/pull/64
+# in Django-sendfile repo as it might be sometime before it gets merged.
+git+https://github.com/adnrs96/django-sendfile.git@f45fac5fdd83e7d42a8d5275038e33f8c2cdb920#egg=django-sendfile==0.3.12

version.py

@@ -1,2 +1,2 @@
 ZULIP_VERSION = "1.5.1+git"
-PROVISION_VERSION = '5.0'
+PROVISION_VERSION = '5.01'

zerver/tests/test_upload.py

@@ -24,7 +24,7 @@
 from zerver.lib.actions import do_delete_old_unclaimed_attachments
 from zilencer.models import Deployment
 
-from zerver.views.upload import upload_file_backend
+from zerver.views.upload import upload_file_backend, serve_local
 
 import ujson
 from six.moves import urllib
@@ -43,6 +43,8 @@
 import base64
 from datetime import timedelta
 from django.utils.timezone import now as timezone_now
+from django.http import HttpRequest
+from sendfile import _get_sendfile
 
 from moto import mock_s3
 
@@ -580,6 +582,21 @@ def test_file_download_authorization_public(self):
             self.assertEqual(b"zulip!", data)
             self.logout()
 
+    def test_serve_local(self):
+        # type: () -> None
+        with self.settings(SENDFILE_BACKEND='sendfile.backends.nginx',
+                           NGINX_VERSION='1.4.6'):
+            _get_sendfile.clear()  # To clearout cached version of backend from djangosendfile
+            filename = os.path.join(settings.LOCAL_UPLOADS_DIR, 'files', 'á.jpg')
+            basedir = os.path.dirname(filename)
+            if not os.path.exists(basedir):
+                os.makedirs(basedir)
+            open(filename, 'w').close()
+            responce = serve_local(HttpRequest(), 'á.jpg')
+            _get_sendfile.clear()
+            test_upload_dir = os.path.split(settings.LOCAL_UPLOADS_DIR)[1]
+            self.assertEqual(responce['X-Accel-Redirect'], '/serve_uploads/../../' + test_upload_dir + '/files/á.jpg')
+
     def tearDown(self):
         # type: () -> None
         destroy_uploads()

zerver/views/upload.py

@@ -14,24 +14,19 @@
 from zerver.lib.validator import check_bool
 from zerver.models import UserProfile, validate_attachment_request
 from django.conf import settings
+from sendfile import sendfile
 
 def serve_s3(request, url_path):
     # type: (HttpRequest, str) -> HttpResponse
     uri = get_signed_upload_url(url_path)
     return redirect(uri)
 
-# TODO: Rewrite this once we have django-sendfile
 def serve_local(request, path_id):
     # type: (HttpRequest, str) -> HttpResponse
-    import os
-    import mimetypes
     local_path = get_local_file_path(path_id)
     if local_path is None:
         return HttpResponseNotFound('<p>File not found</p>')
-    filename = os.path.basename(local_path)
-    response = FileResponse(open(local_path, 'rb'),
-                            content_type = mimetypes.guess_type(filename))
-    return response
+    return sendfile(request, local_path)
 
 @has_request_variables
 def serve_file_backend(request, user_profile, realm_id_str, filename):

zproject/dev_settings.py

@@ -52,3 +52,6 @@
 # Don't require anything about password strength in development
 PASSWORD_MIN_LENGTH = 0
 PASSWORD_MIN_ZXCVBN_QUALITY = 0
+# Make sendfile use django to serve files in development
+SENDFILE_BACKEND = 'sendfile.backends.development'
+SENDFILE_ROOT = os.path.join(LOCAL_UPLOADS_DIR, 'files')

zproject/prod_settings_template.py

@@ -171,21 +171,20 @@
 # directly on the Zulip server.  If file storage in Amazon S3 is
 # desired, you can configure that as follows:
 #
-# (1) Set s3_key and s3_secret_key in /etc/zulip/zulip-secrets.conf to
+# Set s3_key and s3_secret_key in /etc/zulip/zulip-secrets.conf to
 # be the S3 access and secret keys that you want to use, and setting
 # the S3_AUTH_UPLOADS_BUCKET and S3_AVATAR_BUCKET to be the S3 buckets
 # you've created to store file uploads and user avatars, respectively.
 # Then restart Zulip (scripts/restart-zulip).
-#
-# (2) Edit /etc/nginx/sites-available/zulip-enterprise to comment out
-# the nginx configuration for /user_uploads and /user_avatars (see
-# https://github.com/zulip/zulip/issues/291 for discussion of a better
-# solution that won't be automatically reverted by the Zulip upgrade
-# script), and then restart nginx.
 LOCAL_UPLOADS_DIR = "/home/zulip/uploads"
 #S3_AUTH_UPLOADS_BUCKET = ""
 #S3_AVATAR_BUCKET = ""
 
+# Setup the version of NGINX which is being used for this installation.
+# By default a NGINX version is automatically detected from system but it could also be
+# specified here optionally.
+# NGINX_VERSION = '1.4.6'
+
 # Maximum allowed size of uploaded files, in megabytes.  DO NOT SET
 # ABOVE 80MB.  The file upload implementation doesn't support chunked
 # uploads, so browsers will crash if you try uploading larger files.

zproject/settings.py

@@ -15,6 +15,8 @@
 import platform
 import time
 import sys
+import subprocess
+import re
 import six.moves.configparser
 
 from zerver.lib.db import TimeTrackingConnection
@@ -93,6 +95,11 @@ def get_secret(key):
 else:
     from .dev_settings import *
 
+if PRODUCTION and LOCAL_UPLOADS_DIR and 'NGINX_VERSION' not in vars():
+    NGINX_VERSION = re.search(r'([\d.]+)', subprocess.Popen(['nginx', '-v'],
+                                                            stdout=subprocess.PIPE,
+                                                            stderr=subprocess.STDOUT).stdout.read().decode('utf-8')).group(1)
+
 ########################################################################
 # DEFAULT VALUES FOR SETTINGS
 ########################################################################
@@ -138,6 +145,10 @@ def get_secret(key):
                     'RATE_LIMITING': True,
                     'REDIS_HOST': '127.0.0.1',
                     'REDIS_PORT': 6379,
+                    'SENDFILE_BACKEND': 'sendfile.backends.nginx',
+                    'SENDFILE_ROOT': '/home/zulip/uploads/files',
+                    'SENDFILE_URL': '/serve_uploads',
+                    'NGINX_VERSION': None,
                     # The following bots only exist in non-VOYAGER installs
                     'ERROR_BOT': None,
                     'NEW_USER_BOT': None,