[core] Prevent RCE when using --exec with %q (CVE-2024-22423)

The shell escape function now properly escapes `%`, `\\` and `\n`. `utils.Popen` as well as `%q` output template expansion have been patched accordingly.

Prior to this fix using `--exec` together with `%q` when on Windows could cause remote code to execute. See https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-hjq6-52gw-2g7p for more details.

Authored by: Grub4K
This commit is contained in:
Simon Sawicki 2024-04-08 23:18:04 +02:00
parent 216f6a3cb5
commit ff07792676
No known key found for this signature in database
5 changed files with 53 additions and 23 deletions

View file

@ -142,5 +142,10 @@
"when": "e3a3ed8a981d9395c4859b6ef56cd02bc3148db2", "when": "e3a3ed8a981d9395c4859b6ef56cd02bc3148db2",
"short": "[cleanup:ie] No `from` stdlib imports in extractors", "short": "[cleanup:ie] No `from` stdlib imports in extractors",
"authors": ["pukkandan"] "authors": ["pukkandan"]
},
{
"action": "add",
"when": "9590cc6b4768e190183d7d071a6c78170889116a",
"short": "[priority] Security: [[CVE-2024-22423](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-22423)] [Prevent RCE when using `--exec` with `%q` on Windows](https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-hjq6-52gw-2g7p)\n - The shell escape function now properly escapes `%`, `\\` and `\\n`.\n - `utils.Popen` has been patched accordingly."
} }
] ]

View file

@ -2069,6 +2069,10 @@ def run_shell(args):
# Test escaping # Test escaping
assert run_shell(['echo', 'test"&']) == '"test""&"\n' assert run_shell(['echo', 'test"&']) == '"test""&"\n'
assert run_shell(['echo', '%CMDCMDLINE:~-1%&']) == '"%CMDCMDLINE:~-1%&"\n'
assert run_shell(['echo', 'a\nb']) == '"a"\n"b"\n'
assert run_shell(['echo', '"']) == '""""\n'
assert run_shell(['echo', '\\']) == '\\\n'
# Test if delayed expansion is disabled # Test if delayed expansion is disabled
assert run_shell(['echo', '^!']) == '"^!"\n' assert run_shell(['echo', '^!']) == '"^!"\n'
assert run_shell('echo "^!"') == '"^!"\n' assert run_shell('echo "^!"') == '"^!"\n'

View file

@ -25,7 +25,7 @@
from .cache import Cache from .cache import Cache
from .compat import functools, urllib # isort: split from .compat import functools, urllib # isort: split
from .compat import compat_os_name, compat_shlex_quote, urllib_req_to_req from .compat import compat_os_name, urllib_req_to_req
from .cookies import LenientSimpleCookie, load_cookies from .cookies import LenientSimpleCookie, load_cookies
from .downloader import FFmpegFD, get_suitable_downloader, shorten_protocol_name from .downloader import FFmpegFD, get_suitable_downloader, shorten_protocol_name
from .downloader.rtmp import rtmpdump_version from .downloader.rtmp import rtmpdump_version
@ -102,7 +102,6 @@
UserNotLive, UserNotLive,
YoutubeDLError, YoutubeDLError,
age_restricted, age_restricted,
args_to_str,
bug_reports_message, bug_reports_message,
date_from_str, date_from_str,
deprecation_warning, deprecation_warning,
@ -141,6 +140,7 @@
sanitize_filename, sanitize_filename,
sanitize_path, sanitize_path,
sanitize_url, sanitize_url,
shell_quote,
str_or_none, str_or_none,
strftime_or_none, strftime_or_none,
subtitles_filename, subtitles_filename,
@ -823,7 +823,7 @@ def warn_if_short_id(self, argv):
self.report_warning( self.report_warning(
'Long argument string detected. ' 'Long argument string detected. '
'Use -- to separate parameters and URLs, like this:\n%s' % 'Use -- to separate parameters and URLs, like this:\n%s' %
args_to_str(correct_argv)) shell_quote(correct_argv))
def add_info_extractor(self, ie): def add_info_extractor(self, ie):
"""Add an InfoExtractor object to the end of the list.""" """Add an InfoExtractor object to the end of the list."""
@ -1355,7 +1355,7 @@ def create_key(outer_mobj):
value, fmt = escapeHTML(str(value)), str_fmt value, fmt = escapeHTML(str(value)), str_fmt
elif fmt[-1] == 'q': # quoted elif fmt[-1] == 'q': # quoted
value = map(str, variadic(value) if '#' in flags else [value]) value = map(str, variadic(value) if '#' in flags else [value])
value, fmt = ' '.join(map(compat_shlex_quote, value)), str_fmt value, fmt = shell_quote(value, shell=True), str_fmt
elif fmt[-1] == 'B': # bytes elif fmt[-1] == 'B': # bytes
value = f'%{str_fmt}'.encode() % str(value).encode() value = f'%{str_fmt}'.encode() % str(value).encode()
value, fmt = value.decode('utf-8', 'ignore'), 's' value, fmt = value.decode('utf-8', 'ignore'), 's'

View file

@ -27,12 +27,9 @@ def compat_etree_fromstring(text):
compat_os_name = os._name if os.name == 'java' else os.name compat_os_name = os._name if os.name == 'java' else os.name
if compat_os_name == 'nt': def compat_shlex_quote(s):
def compat_shlex_quote(s): from ..utils import shell_quote
import re return shell_quote(s)
return s if re.match(r'^[-_\w./]+$', s) else s.replace('"', '""').join('""')
else:
from shlex import quote as compat_shlex_quote # noqa: F401
def compat_ord(c): def compat_ord(c):

View file

@ -50,7 +50,6 @@
compat_expanduser, compat_expanduser,
compat_HTMLParseError, compat_HTMLParseError,
compat_os_name, compat_os_name,
compat_shlex_quote,
) )
from ..dependencies import xattr from ..dependencies import xattr
@ -836,9 +835,11 @@ def __init__(self, args, *remaining, env=None, text=False, shell=False, **kwargs
if shell and compat_os_name == 'nt' and kwargs.get('executable') is None: if shell and compat_os_name == 'nt' and kwargs.get('executable') is None:
if not isinstance(args, str): if not isinstance(args, str):
args = ' '.join(compat_shlex_quote(a) for a in args) args = shell_quote(args, shell=True)
shell = False shell = False
args = f'{self.__comspec()} /Q /S /D /V:OFF /C "{args}"' # Set variable for `cmd.exe` newline escaping (see `utils.shell_quote`)
env['='] = '"^\n\n"'
args = f'{self.__comspec()} /Q /S /D /V:OFF /E:ON /C "{args}"'
super().__init__(args, *remaining, env=env, shell=shell, **kwargs, startupinfo=self._startupinfo) super().__init__(args, *remaining, env=env, shell=shell, **kwargs, startupinfo=self._startupinfo)
@ -1637,15 +1638,38 @@ def get_filesystem_encoding():
return encoding if encoding is not None else 'utf-8' return encoding if encoding is not None else 'utf-8'
def shell_quote(args): _WINDOWS_QUOTE_TRANS = str.maketrans({'"': '\\"', '\\': '\\\\'})
quoted_args = [] _CMD_QUOTE_TRANS = str.maketrans({
encoding = get_filesystem_encoding() # Keep quotes balanced by replacing them with `""` instead of `\\"`
for a in args: '"': '""',
if isinstance(a, bytes): # Requires a variable `=` containing `"^\n\n"` (set in `utils.Popen`)
# We may get a filename encoded with 'encodeFilename' # `=` should be unique since variables containing `=` cannot be set using cmd
a = a.decode(encoding) '\n': '%=%',
quoted_args.append(compat_shlex_quote(a)) # While we are only required to escape backslashes immediately before quotes,
return ' '.join(quoted_args) # we instead escape all of 'em anyways to be consistent
'\\': '\\\\',
# Use zero length variable replacement so `%` doesn't get expanded
# `cd` is always set as long as extensions are enabled (`/E:ON` in `utils.Popen`)
'%': '%%cd:~,%',
})
def shell_quote(args, *, shell=False):
args = list(variadic(args))
if any(isinstance(item, bytes) for item in args):
deprecation_warning('Passing bytes to utils.shell_quote is deprecated')
encoding = get_filesystem_encoding()
for index, item in enumerate(args):
if isinstance(item, bytes):
args[index] = item.decode(encoding)
if compat_os_name != 'nt':
return shlex.join(args)
trans = _CMD_QUOTE_TRANS if shell else _WINDOWS_QUOTE_TRANS
return ' '.join(
s if re.fullmatch(r'[\w#$*\-+./:?@\\]+', s, re.ASCII) else s.translate(trans).join('""')
for s in args)
def smuggle_url(url, data): def smuggle_url(url, data):
@ -2849,7 +2873,7 @@ def ytdl_is_updateable():
def args_to_str(args): def args_to_str(args):
# Get a short string representation for a subprocess command # Get a short string representation for a subprocess command
return ' '.join(compat_shlex_quote(a) for a in args) return shell_quote(args)
def error_to_str(err): def error_to_str(err):