security fix for course imports

TNL-2270

cms/djangoapps/contentstore/management/commands/export_convert_format.py

@@ -7,6 +7,7 @@ Sample invocation: ./manage.py export_convert_format mycourse.tar.gz ~/newformat
 import os
 from path import path
 from django.core.management.base import BaseCommand, CommandError
+from django.conf import settings
 
 from tempfile import mkdtemp
 import tarfile
@@ -32,8 +33,8 @@ class Command(BaseCommand):
         output_path = args[1]
 
         # Create temp directories to extract the source and create the target archive.
-        temp_source_dir = mkdtemp()
-        temp_target_dir = mkdtemp()
+        temp_source_dir = mkdtemp(dir=settings.DATA_DIR)
+        temp_target_dir = mkdtemp(dir=settings.DATA_DIR)
         try:
             extract_source(source_archive, temp_source_dir)
 

cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py

@@ -3,6 +3,7 @@ Test for export_convert_format.
 """
 from unittest import TestCase
 from django.core.management import call_command, CommandError
+from django.conf import settings
 from tempfile import mkdtemp
 import shutil
 from path import path
@@ -16,7 +17,9 @@ class ConvertExportFormat(TestCase):
     """
     def setUp(self):
         """ Common setup. """
-        self.temp_dir = mkdtemp()
+        super(ConvertExportFormat, self).setUp()
+
+        self.temp_dir = mkdtemp(dir=settings.DATA_DIR)
         self.data_dir = path(__file__).realpath().parent / 'data'
         self.version0 = self.data_dir / "Version0_drafts.tar.gz"
         self.version1 = self.data_dir / "Version1_drafts.tar.gz"
@@ -53,8 +56,8 @@ class ConvertExportFormat(TestCase):
         """
         Helper function for determining if 2 archives are equal.
         """
-        temp_dir_1 = mkdtemp()
-        temp_dir_2 = mkdtemp()
+        temp_dir_1 = mkdtemp(dir=settings.DATA_DIR)
+        temp_dir_2 = mkdtemp(dir=settings.DATA_DIR)
         try:
             extract_source(file1, temp_dir_1)
             extract_source(file2, temp_dir_2)

cms/djangoapps/contentstore/views/tests/test_import_export.py

@@ -204,6 +204,19 @@ class ImportTestCase(CourseTestCase):
 
         return outside_tar
 
+    def _edx_platform_tar(self):
+        """
+        Tarfile with file that extracts to edx-platform directory.
+
+        Extracting this tarfile in directory <dir> will also put its contents
+        directly in <dir> (rather than <dir/tarname>).
+        """
+        outside_tar = self.unsafe_common_dir / "unsafe_file.tar.gz"
+        with tarfile.open(outside_tar, "w:gz") as tar:
+            tar.addfile(tarfile.TarInfo(os.path.join(os.path.abspath("."), "a_file")))
+
+        return outside_tar
+
     def test_unsafe_tar(self):
         """
         Check that safety measure work.
@@ -228,6 +241,12 @@ class ImportTestCase(CourseTestCase):
         try_tar(self._symlink_tar())
         try_tar(self._outside_tar())
         try_tar(self._outside_tar2())
+        try_tar(self._edx_platform_tar())
+
+        # test trying to open a tar outside of the normal data directory
+        with self.settings(DATA_DIR='/not/the/data/dir'):
+            try_tar(self._edx_platform_tar())
+
         # Check that `import_status` returns the appropriate stage (i.e.,
         # either 3, indicating all previous steps are completed, or 0,
         # indicating no upload in progress)

cms/envs/bok_choy.py

@@ -31,6 +31,7 @@ INSTALLED_APPS += ('django_extensions',)
 TEST_ROOT = CONFIG_ROOT.dirname().dirname() / "test_root"  # pylint: disable=no-value-for-parameter
 GITHUB_REPO_ROOT = (TEST_ROOT / "data").abspath()
 LOG_DIR = (TEST_ROOT / "log").abspath()
+DATA_DIR = TEST_ROOT / "data"
 
 # Configure modulestore to use the test folder within the repo
 update_module_store_settings(

cms/envs/test.py

@@ -51,6 +51,7 @@ TEST_ROOT = path('test_root')
 STATIC_ROOT = TEST_ROOT / "staticfiles"
 
 GITHUB_REPO_ROOT = TEST_ROOT / "data"
+DATA_DIR = TEST_ROOT / "data"
 COMMON_TEST_DATA_ROOT = COMMON_ROOT / "test" / "data"
 
 # For testing "push to lms"

common/lib/extract_tar.py

@@ -7,6 +7,7 @@ http://stackoverflow.com/questions/10060069/safely-extract-zip-or-tar-using-pyth
 """
 from os.path import abspath, realpath, dirname, join as joinpath
 from django.core.exceptions import SuspiciousOperation
+from django.conf import settings
 import logging
 
 log = logging.getLogger(__name__)
@@ -28,19 +29,23 @@ def _is_bad_path(path, base):
 
 def _is_bad_link(info, base):
     """
-    Does the file sym- ord hard-link to files outside `base`?
+    Does the file sym- or hard-link to files outside `base`?
     """
     # Links are interpreted relative to the directory containing the link
     tip = resolved(joinpath(base, dirname(info.name)))
     return _is_bad_path(info.linkname, base=tip)
 
 
-def safemembers(members):
+def safemembers(members, base):
     """
     Check that all elements of a tar file are safe.
     """
 
-    base = resolved(".")
+    base = resolved(base)
+
+    # check that we're not trying to import outside of the data_dir
+    if not base.startswith(resolved(settings.DATA_DIR)):
+        raise SuspiciousOperation("Attempted to import course outside of data dir")
 
     for finfo in members:
         if _is_bad_path(finfo.name, base):
@@ -61,8 +66,8 @@ def safemembers(members):
     return members
 
 
-def safetar_extractall(tarf, *args, **kwargs):
+def safetar_extractall(tar_file, path=".", members=None):  # pylint: disable=unused-argument
     """
-    Safe version of `tarf.extractall()`.
+    Safe version of `tar_file.extractall()`.
     """
-    return tarf.extractall(members=safemembers(tarf), *args, **kwargs)
+    return tar_file.extractall(path, safemembers(tar_file, path))