From 5ce582448ececb8d9c30c8c31f58330090ced03a Mon Sep 17 00:00:00 2001 From: Simon Sawicki Date: Tue, 2 Jul 2024 00:52:50 +0200 Subject: [PATCH] [core] Disallow unsafe extensions (CVE-2024-38519) Ref: https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-79w7-vh3h-8g4j Authored by: Grub4K --- README.md | 8 ++ devscripts/changelog_override.json | 5 ++ test/test_utils.py | 31 ++++++++ yt_dlp/YoutubeDL.py | 23 +++++- yt_dlp/__init__.py | 8 ++ yt_dlp/options.py | 2 +- yt_dlp/utils/_utils.py | 114 +++++++++++++++++++++++++++-- 7 files changed, 179 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index f265c8b55..d1fd6e4f0 100644 --- a/README.md +++ b/README.md @@ -2229,6 +2229,14 @@ For ease of use, a few more compat options are available: * `--compat-options 2022`: Same as `--compat-options 2023,playlist-match-filter,no-external-downloader-progress,prefer-legacy-http-handler,manifest-filesize-approx` * `--compat-options 2023`: Currently does nothing. Use this to enable all future compat options +The following compat options restore vulnerable behavior from before security patches: + +* `--compat-options allow-unsafe-ext`: Allow files with any extension (including unsafe ones) to be downloaded ([GHSA-79w7-vh3h-8g4j]()) + + > :warning: Only use if a valid file download is rejected because its extension is detected as uncommon + > + > **This option can enable remote code execution! Consider [opening an issue]() instead!** + ### Deprecated options These are all the deprecated options and the current alternative to achieve the same effect diff --git a/devscripts/changelog_override.json b/devscripts/changelog_override.json index f7209f3bd..ced38a0dd 100644 --- a/devscripts/changelog_override.json +++ b/devscripts/changelog_override.json @@ -175,5 +175,10 @@ "when": "e6a22834df1776ec4e486526f6df2bf53cb7e06f", "short": "[ie/orf:on] Add `prefer_segments_playlist` extractor-arg (#10314)", "authors": ["seproDev"] + }, + { + "action": "add", + "when": "6aaf96a3d6e7d0d426e97e11a2fcf52fda00e733", + "short": "[priority] Security: [[CVE-2024-10123](https://nvd.nist.gov/vuln/detail/CVE-2024-10123)] [Properly sanitize file-extension to prevent file system modification and RCE](https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-79w7-vh3h-8g4j)\n - Unsafe extensions are now blocked from being downloaded" } ] diff --git a/test/test_utils.py b/test/test_utils.py index 251739686..3ff1f8b55 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -130,6 +130,7 @@ from yt_dlp.utils import ( xpath_text, xpath_with_ns, ) +from yt_dlp.utils._utils import _UnsafeExtensionError from yt_dlp.utils.networking import ( HTTPHeaderDict, escape_rfc3986, @@ -281,6 +282,13 @@ class TestUtil(unittest.TestCase): finally: os.environ['HOME'] = old_home or '' + _uncommon_extensions = [ + ('exe', 'abc.exe.ext'), + ('de', 'abc.de.ext'), + ('../.mp4', None), + ('..\\.mp4', None), + ] + def test_prepend_extension(self): self.assertEqual(prepend_extension('abc.ext', 'temp'), 'abc.temp.ext') self.assertEqual(prepend_extension('abc.ext', 'temp', 'ext'), 'abc.temp.ext') @@ -289,6 +297,19 @@ class TestUtil(unittest.TestCase): self.assertEqual(prepend_extension('.abc', 'temp'), '.abc.temp') self.assertEqual(prepend_extension('.abc.ext', 'temp'), '.abc.temp.ext') + # Test uncommon extensions + self.assertEqual(prepend_extension('abc.ext', 'bin'), 'abc.bin.ext') + for ext, result in self._uncommon_extensions: + with self.assertRaises(_UnsafeExtensionError): + prepend_extension('abc', ext) + if result: + self.assertEqual(prepend_extension('abc.ext', ext, 'ext'), result) + else: + with self.assertRaises(_UnsafeExtensionError): + prepend_extension('abc.ext', ext, 'ext') + with self.assertRaises(_UnsafeExtensionError): + prepend_extension('abc.unexpected_ext', ext, 'ext') + def test_replace_extension(self): self.assertEqual(replace_extension('abc.ext', 'temp'), 'abc.temp') self.assertEqual(replace_extension('abc.ext', 'temp', 'ext'), 'abc.temp') @@ -297,6 +318,16 @@ class TestUtil(unittest.TestCase): self.assertEqual(replace_extension('.abc', 'temp'), '.abc.temp') self.assertEqual(replace_extension('.abc.ext', 'temp'), '.abc.temp') + # Test uncommon extensions + self.assertEqual(replace_extension('abc.ext', 'bin'), 'abc.unknown_video') + for ext, _ in self._uncommon_extensions: + with self.assertRaises(_UnsafeExtensionError): + replace_extension('abc', ext) + with self.assertRaises(_UnsafeExtensionError): + replace_extension('abc.ext', ext, 'ext') + with self.assertRaises(_UnsafeExtensionError): + replace_extension('abc.unexpected_ext', ext, 'ext') + def test_subtitles_filename(self): self.assertEqual(subtitles_filename('abc.ext', 'en', 'vtt'), 'abc.en.vtt') self.assertEqual(subtitles_filename('abc.ext', 'en', 'vtt', 'ext'), 'abc.en.vtt') diff --git a/yt_dlp/YoutubeDL.py b/yt_dlp/YoutubeDL.py index a8d0342d5..e56c3ed3c 100644 --- a/yt_dlp/YoutubeDL.py +++ b/yt_dlp/YoutubeDL.py @@ -159,7 +159,7 @@ from .utils import ( write_json_file, write_string, ) -from .utils._utils import _YDLLogger +from .utils._utils import _UnsafeExtensionError, _YDLLogger from .utils.networking import ( HTTPHeaderDict, clean_headers, @@ -172,6 +172,20 @@ if compat_os_name == 'nt': import ctypes +def _catch_unsafe_extension_error(func): + @functools.wraps(func) + def wrapper(self, *args, **kwargs): + try: + return func(self, *args, **kwargs) + except _UnsafeExtensionError as error: + self.report_error( + f'The extracted extension ({error.extension!r}) is unusual ' + 'and will be skipped for safety reasons. ' + f'If you believe this is an error{bug_reports_message(",")}') + + return wrapper + + class YoutubeDL: """YoutubeDL class. @@ -454,8 +468,9 @@ class YoutubeDL: Set the value to 'native' to use the native downloader compat_opts: Compatibility options. See "Differences in default behavior". The following options do not work when used through the API: - filename, abort-on-error, multistreams, no-live-chat, format-sort - no-clean-infojson, no-playlist-metafiles, no-keep-subs, no-attach-info-json. + filename, abort-on-error, multistreams, no-live-chat, + format-sort, no-clean-infojson, no-playlist-metafiles, + no-keep-subs, no-attach-info-json, allow-unsafe-ext. Refer __init__.py for their implementation progress_template: Dictionary of templates for progress outputs. Allowed keys are 'download', 'postprocess', @@ -1400,6 +1415,7 @@ class YoutubeDL: outtmpl, info_dict = self.prepare_outtmpl(outtmpl, info_dict, *args, **kwargs) return self.escape_outtmpl(outtmpl) % info_dict + @_catch_unsafe_extension_error def _prepare_filename(self, info_dict, *, outtmpl=None, tmpl_type=None): assert None in (outtmpl, tmpl_type), 'outtmpl and tmpl_type are mutually exclusive' if outtmpl is None: @@ -3192,6 +3208,7 @@ class YoutubeDL: os.remove(file) return None + @_catch_unsafe_extension_error def process_info(self, info_dict): """Process a single resolved IE result. (Modifies it in-place)""" diff --git a/yt_dlp/__init__.py b/yt_dlp/__init__.py index c18af7589..f88f15d70 100644 --- a/yt_dlp/__init__.py +++ b/yt_dlp/__init__.py @@ -64,6 +64,7 @@ from .utils import ( write_string, ) from .utils.networking import std_headers +from .utils._utils import _UnsafeExtensionError from .YoutubeDL import YoutubeDL _IN_CLI = False @@ -593,6 +594,13 @@ def validate_options(opts): if opts.ap_username is not None and opts.ap_password is None: opts.ap_password = getpass.getpass('Type TV provider account password and press [Return]: ') + # compat option changes global state destructively; only allow from cli + if 'allow-unsafe-ext' in opts.compat_opts: + warnings.append( + 'Using allow-unsafe-ext opens you up to potential attacks. ' + 'Use with great care!') + _UnsafeExtensionError.sanitize_extension = lambda x: x + return warnings, deprecation_warnings diff --git a/yt_dlp/options.py b/yt_dlp/options.py index b97c516ce..1b18575c1 100644 --- a/yt_dlp/options.py +++ b/yt_dlp/options.py @@ -474,7 +474,7 @@ def create_parser(): 'no-attach-info-json', 'embed-thumbnail-atomicparsley', 'no-external-downloader-progress', 'embed-metadata', 'seperate-video-versions', 'no-clean-infojson', 'no-keep-subs', 'no-certifi', 'no-youtube-channel-redirect', 'no-youtube-unavailable-videos', 'no-youtube-prefer-utc-upload-date', - 'prefer-legacy-http-handler', 'manifest-filesize-approx', + 'prefer-legacy-http-handler', 'manifest-filesize-approx', 'allow-unsafe-ext', }, 'aliases': { 'youtube-dl': ['all', '-multistreams', '-playlist-match-filter', '-manifest-filesize-approx'], 'youtube-dlc': ['all', '-no-youtube-channel-redirect', '-no-live-chat', '-playlist-match-filter', '-manifest-filesize-approx'], diff --git a/yt_dlp/utils/_utils.py b/yt_dlp/utils/_utils.py index 664675a09..b5e1e2950 100644 --- a/yt_dlp/utils/_utils.py +++ b/yt_dlp/utils/_utils.py @@ -2085,17 +2085,20 @@ def parse_duration(s): (days, 86400), (hours, 3600), (mins, 60), (secs, 1), (ms, 1))) -def prepend_extension(filename, ext, expected_real_ext=None): +def _change_extension(prepend, filename, ext, expected_real_ext=None): name, real_ext = os.path.splitext(filename) - return ( - f'{name}.{ext}{real_ext}' - if not expected_real_ext or real_ext[1:] == expected_real_ext - else f'{filename}.{ext}') + + if not expected_real_ext or real_ext[1:] == expected_real_ext: + filename = name + if prepend and real_ext: + _UnsafeExtensionError.sanitize_extension(ext, prepend=True) + return f'{filename}.{ext}{real_ext}' + + return f'{filename}.{_UnsafeExtensionError.sanitize_extension(ext)}' -def replace_extension(filename, ext, expected_real_ext=None): - name, real_ext = os.path.splitext(filename) - return f'{name if not expected_real_ext or real_ext[1:] == expected_real_ext else filename}.{ext}' +prepend_extension = functools.partial(_change_extension, True) +replace_extension = functools.partial(_change_extension, False) def check_executable(exe, args=[]): @@ -5035,6 +5038,101 @@ MEDIA_EXTENSIONS.audio += MEDIA_EXTENSIONS.common_audio KNOWN_EXTENSIONS = (*MEDIA_EXTENSIONS.video, *MEDIA_EXTENSIONS.audio, *MEDIA_EXTENSIONS.manifests) +class _UnsafeExtensionError(Exception): + """ + Mitigation exception for uncommon/malicious file extensions + This should be caught in YoutubeDL.py alongside a warning + + Ref: https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-79w7-vh3h-8g4j + """ + ALLOWED_EXTENSIONS = frozenset([ + # internal + 'description', + 'json', + 'meta', + 'orig', + 'part', + 'temp', + 'uncut', + 'unknown_video', + 'ytdl', + + # video + *MEDIA_EXTENSIONS.video, + 'avif', + 'ismv', + 'm2ts', + 'm4s', + 'mng', + 'mpeg', + 'qt', + 'swf', + 'ts', + 'vp9', + 'wvm', + + # audio + *MEDIA_EXTENSIONS.audio, + 'isma', + 'mid', + 'mpga', + 'ra', + + # image + *MEDIA_EXTENSIONS.thumbnails, + 'bmp', + 'gif', + 'heic', + 'ico', + 'jng', + 'jpeg', + 'jxl', + 'svg', + 'tif', + 'wbmp', + + # subtitle + *MEDIA_EXTENSIONS.subtitles, + 'dfxp', + 'fs', + 'ismt', + 'sami', + 'scc', + 'ssa', + 'tt', + 'ttml', + + # others + *MEDIA_EXTENSIONS.manifests, + *MEDIA_EXTENSIONS.storyboards, + 'desktop', + 'ism', + 'm3u', + 'sbv', + 'url', + 'webloc', + 'xml', + ]) + + def __init__(self, extension, /): + super().__init__(f'unsafe file extension: {extension!r}') + self.extension = extension + + @classmethod + def sanitize_extension(cls, extension, /, *, prepend=False): + if '/' in extension or '\\' in extension: + raise cls(extension) + + if not prepend: + _, _, last = extension.rpartition('.') + if last == 'bin': + extension = last = 'unknown_video' + if last.lower() not in cls.ALLOWED_EXTENSIONS: + raise cls(extension) + + return extension + + class RetryManager: """Usage: for retry in RetryManager(...):