Merge pull request from GHSA-jq22-wqf5-666p

* Serve files in old file-viewer through FileResponse.

Fixes bug 1566954

The old file-viewer will be going away "soon" anyway so we should be
using the same technique as for the new one.

The problem here specifically is that when going through Nginx via
X-Accel-Redirect nginx isn't setting proper CSP headers for the
download. Serving the files ourselves allows us to much easier restrict
CSP in the future even further.

* Explicitly restrict CSP config on download endpoints.

* Set report uri in settings.py too

* Explicitly add frame-ancestors

* Remove outdated test

settings.py

@@ -124,6 +124,7 @@ ALLOWED_FXA_CONFIGS = ['default', 'amo', 'local', 'code-manager']
 
 # CSP report endpoint which returns a 204 from addons-nginx in local dev.
 CSP_REPORT_URI = '/csp-report'
+RESTRICTED_DOWNLOAD_CSP['REPORT_URI'] = CSP_REPORT_URI
 
 # Allow GA over http + www subdomain in local development.
 HTTP_GA_SRC = 'http://www.google-analytics.com'

src/olympia/files/tests/test_views.py

@@ -493,7 +493,47 @@ class TestFileViewer(FilesBase, TestCase):
         res = self.client.get(self.files_serve(binary) + '?token=' + token)
         assert res.status_code == 200
         assert res['Content-Type'] == 'application/octet-stream'
-        assert 'X-Sendfile' in res
+
+        # Not sending through nginx but directly using djangos FileResponse
+        # to be able to restrict and test CSP related settings.
+        assert 'X-Sendfile' not in res
+        assert (
+            res['Content-Disposition'] ==
+            'attachment; filename="ar.dic"')
+
+        content = res.getvalue().decode('utf-8')
+        assert content.startswith(
+            '52726\n####الأدوات(واستثناءات)+الأفعال+الأسماء')
+
+    @override_settings(CSP_REPORT_ONLY=False)
+    def test_serve_respects_csp(self):
+        self.file_viewer.extract()
+        res = self.client.get(self.files_redirect(binary))
+        assert res.status_code == 302
+        url = res['Location']
+        token = urlparse(url).query.split('=')[1]
+
+        res = self.client.get(self.files_serve(binary) + '?token=' + token)
+        assert res.status_code == 200
+        assert res['Content-Type'] == 'application/octet-stream'
+
+        # Make sure a default-src is set.
+        assert "default-src 'none'" in res['content-security-policy']
+        # Make sure things are as locked down as possible,
+        # as per https://bugzilla.mozilla.org/show_bug.cgi?id=1566954
+        assert "object-src 'none'" in res['content-security-policy']
+        assert "base-uri 'none'" in res['content-security-policy']
+        assert "form-action 'none'" in res['content-security-policy']
+        assert "frame-ancestors 'none'" in res['content-security-policy']
+
+        # The report-uri should be set.
+        assert "report-uri" in res['content-security-policy']
+
+        # Other properties that we defined by default aren't set
+        assert "style-src" not in res['content-security-policy']
+        assert "font-src" not in res['content-security-policy']
+        assert "frame-src" not in res['content-security-policy']
+        assert "child-src" not in res['content-security-policy']
 
     def test_serve_obj_not_exist(self):
         self.file_viewer.extract()
@@ -519,13 +559,6 @@ class TestFileViewer(FilesBase, TestCase):
         res = self.client.get(url)
         assert res.status_code == 403
 
-    def test_bounce(self):
-        self.file_viewer.extract()
-        res = self.client.get(self.files_redirect(binary), follow=True)
-        assert res.status_code == 200
-        assert res[settings.XSENDFILE_HEADER] == (
-            self.file_viewer.get_files().get(binary)['full'])
-
     @patch.object(settings, 'FILE_VIEWER_SIZE_LIMIT', 5)
     def test_file_size(self):
         self.file_viewer.extract()

src/olympia/files/views.py

@@ -1,3 +1,4 @@
+from django.conf import settings
 from django import http, shortcuts
 from django.db.transaction import non_atomic_requests
 from django.utils.translation import ugettext
@@ -5,13 +6,15 @@ from django.views.decorators.cache import never_cache
 from django.views.decorators.csrf import csrf_exempt
 from django.views.decorators.http import condition
 
+from csp.decorators import csp as set_csp
+
 import olympia.core.logger
 
 from olympia.access import acl
 from olympia.lib.cache import Message, Token
 from olympia.amo.decorators import json_view
 from olympia.amo.urlresolvers import reverse
-from olympia.amo.utils import HttpResponseSendFile, render, urlparams
+from olympia.amo.utils import render, urlparams
 from olympia.files.decorators import (
     compare_file_view, etag, file_view, file_view_token, last_modified)
 from olympia.files.file_viewer import extract_file
@@ -179,6 +182,7 @@ def redirect(request, viewer, key):
     return http.HttpResponseRedirect(url)
 
 
+@set_csp(**settings.RESTRICTED_DOWNLOAD_CSP)
 @file_view_token
 @non_atomic_requests
 def serve(request, viewer, key):
@@ -193,5 +197,10 @@ def serve(request, viewer, key):
                   key, list(files.keys())[:10], len(files.keys()),
                   viewer.file.id))
         raise http.Http404
-    return HttpResponseSendFile(request, obj['full'],
-                                content_type=obj['mimetype'])
+
+    fobj = open(obj['full'], 'rb')
+
+    response = http.FileResponse(
+        fobj, as_attachment=True, content_type=obj['mimetype'])
+
+    return response

src/olympia/lib/settings_base.py

@@ -1409,6 +1409,15 @@ CSP_STYLE_SRC = (
     PROD_CDN_HOST,
 )
 
+RESTRICTED_DOWNLOAD_CSP = {
+    'DEFAULT_SRC': "'none'",
+    'BASE_URI': "'none'",
+    'FORM_ACTION': "'none'",
+    'OBJECT_SRC': "'none'",
+    'FRAME_ANCESTORS': "'none'",
+    'REPORT_URI': CSP_REPORT_URI
+}
+
 # Should robots.txt deny everything or disallow a calculated list of URLs we
 # don't want to be crawled?  Default is true, allow everything, toggled to
 # False on -dev and stage.

src/olympia/reviewers/tests/test_views.py

@@ -6339,6 +6339,39 @@ class TestDownloadGitFileView(TestCase):
         assert content.startswith('{')
         assert '"manifest_version": 2' in content
 
+    @override_settings(CSP_REPORT_ONLY=False)
+    def test_download_respects_csp(self):
+        user = UserProfile.objects.create(username='reviewer')
+        self.grant_permission(user, 'Addons:Review')
+        self.client.login(email=user.email)
+
+        url = reverse('reviewers.download_git_file', kwargs={
+            'version_id': self.version.pk,
+            'filename': 'manifest.json'
+        })
+
+        response = self.client.get(url)
+
+        assert response.status_code == 200
+
+        # Make sure a default-src is set.
+        assert "default-src 'none'" in response['content-security-policy']
+        # Make sure things are as locked down as possible,
+        # as per https://bugzilla.mozilla.org/show_bug.cgi?id=1566954
+        assert "object-src 'none'" in response['content-security-policy']
+        assert "base-uri 'none'" in response['content-security-policy']
+        assert "form-action 'none'" in response['content-security-policy']
+        assert "frame-ancestors 'none'" in response['content-security-policy']
+
+        # The report-uri should be set.
+        assert "report-uri" in response['content-security-policy']
+
+        # Other properties that we defined by default aren't set
+        assert "style-src" not in response['content-security-policy']
+        assert "font-src" not in response['content-security-policy']
+        assert "frame-src" not in response['content-security-policy']
+        assert "child-src" not in response['content-security-policy']
+
     def test_download_emoji_filename(self):
         new_version = version_factory(
             addon=self.addon, file_kw={'filename': 'webextension_no_id.xpi'})

src/olympia/reviewers/views.py

@@ -26,6 +26,8 @@ from rest_framework.mixins import (
     ListModelMixin, RetrieveModelMixin, CreateModelMixin, DestroyModelMixin,
     UpdateModelMixin)
 
+from csp.decorators import csp as set_csp
+
 import olympia.core.logger
 
 from olympia import amo
@@ -1159,6 +1161,7 @@ def theme_background_images(request, version_id):
 
 
 @login_required
+@set_csp(**settings.RESTRICTED_DOWNLOAD_CSP)
 def download_git_stored_file(request, version_id, filename):
     version = get_object_or_404(Version.unfiltered, id=int(version_id))