local-uploads: Start running authentication checks on file requests.

From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: #320 and #291 partially.

docs/production/security-model.md

@@ -200,27 +200,53 @@ strength allowed is controlled by two settings in
 
 ## User-uploaded content
 
-* Zulip supports user-uploaded files; ideally they should be hosted
+* 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.
+  various same-domain attacks (e.g. zulip-user-content.example.com).
 
-  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)).
+  We support two ways of hosting them: the basic `LOCAL_UPLOADS_DIR`
+  file storage backend, where they are stored in a directory on the
+  Zulip server's filesystem, and the S3 backend, where the files are
+  stored in Amazon S3.  It would not be difficult to add additional
+  supported backends should there be a need; see
+  `zerver/lib/upload.py` for the full interface.
 
-  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.
+  For both backends, the URLs used to access uploaded files are long,
+  random strings, providing one layer of security against unauthorized
+  users accessing files uploaded in Zulip (an authorized user would
+  need to share the URL with an unauthorized user in order for the
+  file to be accessed by the unauthorized user; and of course, any
+  such authorized user could have just downloaded and sent the file
+  instead of the URL, so this is arguably the best protection
+  possible).  However, to help protect against consequences accidental
+  sharing of URLs to restricted files (e.g. by forwarding a
+  missed-message email or leaks involving the Referer header), we
+  provide additional layers of protection in both backends as well.
+
+  In the Zulip S3 backend, the random URLs to access files that are
+  presented to users don't actually host the content.  Instead, the S3
+  backend verifies that the user has a valid Zulip session in the
+  relevant realm (and that has access to a Zulip message linking to
+  the file), and if so, then redirects the browser to a temporary S3
+  URL for the file that expires a short time later.  In this way,
+  possessing a URL to a secret file in Zulip does not provide
+  unauthorized users with access to that file.
+
+  We have a similar protection for the `LOCAL_UPLOADS_DIR` backend,
+  that is currently only available in Ubuntu Xenial (this is the one
+  place in Zulip where behavior is currently different between Ubuntu
+  Trusty and Ubuntu Xenial).  On Ubuntu Xenial, every access to an
+  uploaded file has access control verified verified (confirming that
+  the browser is logged into a Zulip account that has received the
+  uploaded file in question).
+
+  On Ubuntu Trusty, because the older version of `nginx` available
+  there doesn't have proper Unicode support for the `X-Accel-Redirect`
+  feature, the `LOCAL_UPLOADS_DIR` backend only has the single layer
+  of security described at the beginning of this section (long,
+  randomly generated secret URLs).  This could be fixed with further
+  engineering, but given the upcoming end-of-life of Ubuntu Trusty, we
+  have no plans to do that further work.
 
 * 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,12 +12,6 @@ server {
     ssl_certificate /etc/ssl/certs/zulip.combined-chain.crt;
     ssl_certificate_key /etc/ssl/private/zulip.key;
 
-    location /user_uploads {
-        add_header X-Content-Type-Options nosniff;
-        include /etc/nginx/zulip-include/uploads.types;
-        alias /home/zulip/uploads/files;
-    }
-
     location /user_avatars {
         add_header X-Content-Type-Options nosniff;
         include /etc/nginx/zulip-include/uploads.types;
@@ -30,4 +24,5 @@ server {
 
     include /etc/nginx/zulip-include/certbot;
     include /etc/nginx/zulip-include/app;
+    include /etc/nginx/zulip-include/uploads.route;
 }

puppet/zulip/files/nginx/zulip-include-maybe/uploads-route.direct

@@ -0,0 +1,5 @@
+location /user_uploads {
+    add_header X-Content-Type-Options nosniff;
+    include /etc/nginx/zulip-include/uploads.types;
+    alias /home/zulip/uploads/files;
+}

puppet/zulip/files/nginx/zulip-include-maybe/uploads-route.internal

@@ -0,0 +1,6 @@
+location /serve_uploads {
+    internal;
+    add_header X-Content-Type-Options nosniff;
+    include /etc/nginx/zulip-include/uploads.types;
+    alias /home/zulip/uploads/files;
+}

puppet/zulip/manifests/nginx.pp

@@ -14,6 +14,29 @@ class zulip::nginx {
     notify => Service["nginx"],
   }
 
+  # Nginx versions 1.4.6 and older do not support quoted URLs with the
+  # X-Accel-Redirect / "sendfile" feature, which are required for
+  # unicode support in filenames.  As a result, we use the fancier
+  # django-sendfile behavior only when a sufficiently current version
+  # of nginx is present (e.g.. Xenial).  Older versions (e.g. Trusty)
+  # retain the older, less secure, file upload behavior; we expect
+  # that this will stop being relevant when we drop Trusty support
+  # from Zulip altogether, no later than when Trusty reaches EOL in 2019.
+  $uploads_route = $zulip::base::release_name ? {
+    'trusty' => 'puppet:///modules/zulip/nginx/zulip-include-maybe/uploads-route.direct',
+    default  => 'puppet:///modules/zulip/nginx/zulip-include-maybe/uploads-route.internal',
+  }
+
+  file { "/etc/nginx/zulip-include/uploads.route":
+    require => Package["nginx-full"],
+    ensure => file,
+    owner  => "root",
+    group  => "root",
+    mode => 644,
+    notify => Service["nginx"],
+    source => $uploads_route,
+  }
+
   file { "/etc/nginx/nginx.conf":
     require => Package["nginx-full"],
     ensure => file,

requirements/common.in

@@ -188,3 +188,6 @@ twilio==6.10.3
 
 # Needed for processing payments (in zilencer)
 stripe==1.77.2
+
+# Needed for serving uploaded files from nginx but perform auth checks in django.
+django-sendfile==0.3.11

requirements/dev.txt

@@ -47,6 +47,7 @@ django-formtools==2.1     # via django-two-factor-auth
 django-otp==0.4.1.1       # via django-two-factor-auth
 django-phonenumber-field==1.3.0  # via django-two-factor-auth
 django-pipeline==1.6.14
+django-sendfile==0.3.11
 django-statsd-mozilla==0.4.0
 django-two-factor-auth==1.7.0
 django-webpack-loader==0.5.0

requirements/prod.txt

@@ -35,6 +35,7 @@ django-formtools==2.1     # via django-two-factor-auth
 django-otp==0.4.1.1       # via django-two-factor-auth
 django-phonenumber-field==1.3.0  # via django-two-factor-auth
 django-pipeline==1.6.14
+django-sendfile==0.3.11
 django-statsd-mozilla==0.4.0
 django-two-factor-auth==1.7.0
 django-webpack-loader==0.5.0

zerver/tests/test_upload.py

@@ -30,7 +30,7 @@ from zerver.lib.actions import (
     internal_send_private_message,
 )
 
-from zerver.views.upload import upload_file_backend
+from zerver.views.upload import upload_file_backend, serve_local
 
 import urllib
 from PIL import Image
@@ -47,7 +47,9 @@ import datetime
 import requests
 import base64
 from datetime import timedelta
+from django.http import HttpRequest
 from django.utils.timezone import now as timezone_now
+from sendfile import _get_sendfile
 
 from typing import Any, Callable, Text
 
@@ -538,6 +540,27 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
             self.assertEqual(b"zulip!", data)
             self.logout()
 
+    def test_serve_local(self) -> None:
+        def check_xsend_links(name: Text, name_str_for_test: Text) -> None:
+            with self.settings(SENDFILE_BACKEND='sendfile.backends.nginx'):
+                _get_sendfile.clear()  # To clearout cached version of backend from djangosendfile
+                self.login(self.example_email("hamlet"))
+                fp = StringIO("zulip!")
+                fp.name = name
+                result = self.client_post("/json/user_uploads", {'file': fp})
+                uri = result.json()['uri']
+                fp_path_id = re.sub('/user_uploads/', '', uri)
+                fp_path = os.path.split(fp_path_id)[0]
+                response = self.client_get(uri)
+                _get_sendfile.clear()
+                test_upload_dir = os.path.split(settings.LOCAL_UPLOADS_DIR)[1]
+                self.assertEqual(response['X-Accel-Redirect'],
+                                 '/serve_uploads/../../'  + test_upload_dir +
+                                 '/files/' + fp_path + '/' + name_str_for_test)
+
+        check_xsend_links('zulip.txt', 'zulip.txt')
+        check_xsend_links('áéБД.txt', '%C3%A1%C3%A9%D0%91%D0%94.txt')
+
     def tearDown(self) -> None:
         destroy_uploads()
 

zerver/views/upload.py

@@ -12,22 +12,17 @@ from zerver.lib.upload import upload_message_image_from_request, get_local_file_
 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: HttpRequest, url_path: str) -> HttpResponse:
     uri = get_signed_upload_url(url_path)
     return redirect(uri)
 
-# TODO: Rewrite this once we have django-sendfile
-def serve_local(request: HttpRequest, path_id: str) -> FileResponse:
-    import os
-    import mimetypes
+def serve_local(request: HttpRequest, path_id: str) -> HttpResponse:
     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: HttpRequest, user_profile: UserProfile,

zproject/dev_settings.py

@@ -75,3 +75,7 @@ EMAIL_HOST_USER = ""
 # Two factor authentication: Use the fake backend for development.
 TWO_FACTOR_CALL_GATEWAY = 'two_factor.gateways.fake.Fake'
 TWO_FACTOR_SMS_GATEWAY = 'two_factor.gateways.fake.Fake'
+
+# Make sendfile use django to serve files in development
+SENDFILE_BACKEND = 'sendfile.backends.development'
+SENDFILE_ROOT = os.path.join(LOCAL_UPLOADS_DIR, 'files')

zproject/settings.py

@@ -180,6 +180,9 @@ DEFAULT_SETTINGS = {
     'REMOTE_POSTGRES_HOST': '',
     'REMOTE_POSTGRES_SSLMODE': '',
     'THUMBOR_HOST': '',
+    'SENDFILE_BACKEND': 'sendfile.backends.nginx',
+    'SENDFILE_ROOT': '/home/zulip/uploads/files',
+    'SENDFILE_URL': '/serve_uploads',
 
     # ToS/Privacy templates
     'PRIVACY_POLICY': None,