Skip to content

Commit de015e9

Browse files
committed
[core] Prevent RCE when using --exec with %q (CVE-2023-40581)
The shell escape function is now using `""` instead of `\"`. `utils.Popen` has been patched to properly quote commands. Prior to this fix using `--exec` together with `%q` when on Windows could cause remote code to execute. See GHSA-42h4-v29r-42qg for reference. Authored by: Grub4K
1 parent 61bdf15 commit de015e9

File tree

6 files changed

+46
-13
lines changed

6 files changed

+46
-13
lines changed

Diff for: devscripts/changelog_override.json

+5
Original file line numberDiff line numberDiff line change
@@ -93,5 +93,10 @@
9393
"action": "add",
9494
"when": "c1d71d0d9f41db5e4306c86af232f5f6220a130b",
9595
"short": "[priority] **The minimum *recommended* Python version has been raised to 3.8**\nSince Python 3.7 has reached end-of-life, support for it will be dropped soon. [Read more](https://github.com/yt-dlp/yt-dlp/issues/7803)"
96+
},
97+
{
98+
"action": "add",
99+
"when": "61bdf15fc7400601c3da1aa7a43917310a5bf391",
100+
"short": "[priority] Security: [[CVE-2023-40581](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-40581)] [Prevent RCE when using `--exec` with `%q` on Windows](https://github.com/yt-dlp/yt-dlp/security/advisories/GHSA-42h4-v29r-42qg)\n - The shell escape function is now using `\"\"` instead of `\\\"`.\n - `utils.Popen` has been patched to properly quote commands."
96101
}
97102
]

Diff for: test/test_YoutubeDL.py

+3-3
Original file line numberDiff line numberDiff line change
@@ -784,9 +784,9 @@ def expect_same_infodict(out):
784784
test('%(title4)#S', 'foo_bar_test')
785785
test('%(title4).10S', ('foo "bar" ', 'foo "bar"' + ('#' if compat_os_name == 'nt' else ' ')))
786786
if compat_os_name == 'nt':
787-
test('%(title4)q', ('"foo \\"bar\\" test"', ""foo ⧹"bar⧹" test""))
788-
test('%(formats.:.id)#q', ('"id 1" "id 2" "id 3"', '"id 1" "id 2" "id 3"'))
789-
test('%(formats.0.id)#q', ('"id 1"', '"id 1"'))
787+
test('%(title4)q', ('"foo ""bar"" test"', None))
788+
test('%(formats.:.id)#q', ('"id 1" "id 2" "id 3"', None))
789+
test('%(formats.0.id)#q', ('"id 1"', None))
790790
else:
791791
test('%(title4)q', ('\'foo "bar" test\'', '\'foo "bar" test\''))
792792
test('%(formats.:.id)#q', "'id 1' 'id 2' 'id 3'")

Diff for: test/test_utils.py

+16
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import io
1515
import itertools
1616
import json
17+
import subprocess
1718
import xml.etree.ElementTree
1819

1920
from yt_dlp.compat import (
@@ -28,6 +29,7 @@
2829
InAdvancePagedList,
2930
LazyList,
3031
OnDemandPagedList,
32+
Popen,
3133
age_restricted,
3234
args_to_str,
3335
base_url,
@@ -2388,6 +2390,20 @@ def test_extract_basic_auth(self):
23882390
assert extract_basic_auth('http://user:@foo.bar') == ('http://foo.bar', 'Basic dXNlcjo=')
23892391
assert extract_basic_auth('http://user:pass@foo.bar') == ('http://foo.bar', 'Basic dXNlcjpwYXNz')
23902392

2393+
@unittest.skipUnless(compat_os_name == 'nt', 'Only relevant on Windows')
2394+
def test_Popen_windows_escaping(self):
2395+
def run_shell(args):
2396+
stdout, stderr, error = Popen.run(
2397+
args, text=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
2398+
assert not stderr
2399+
assert not error
2400+
return stdout
2401+
2402+
# Test escaping
2403+
assert run_shell(['echo', 'test"&']) == '"test""&"\n'
2404+
# Test if delayed expansion is disabled
2405+
assert run_shell(['echo', '^!']) == '"^!"\n'
2406+
assert run_shell('echo "^!"') == '"^!"\n'
23912407

23922408
if __name__ == '__main__':
23932409
unittest.main()

Diff for: yt_dlp/compat/__init__.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ def compat_etree_fromstring(text):
3030
if compat_os_name == 'nt':
3131
def compat_shlex_quote(s):
3232
import re
33-
return s if re.match(r'^[-_\w./]+$', s) else '"%s"' % s.replace('"', '\\"')
33+
return s if re.match(r'^[-_\w./]+$', s) else s.replace('"', '""').join('""')
3434
else:
3535
from shlex import quote as compat_shlex_quote # noqa: F401
3636

Diff for: yt_dlp/postprocessor/exec.py

+5-7
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
1-
import subprocess
2-
31
from .common import PostProcessor
42
from ..compat import compat_shlex_quote
5-
from ..utils import PostProcessingError, encodeArgument, variadic
3+
from ..utils import Popen, PostProcessingError, variadic
64

75

86
class ExecPP(PostProcessor):
@@ -27,10 +25,10 @@ def parse_cmd(self, cmd, info):
2725
def run(self, info):
2826
for tmpl in self.exec_cmd:
2927
cmd = self.parse_cmd(tmpl, info)
30-
self.to_screen('Executing command: %s' % cmd)
31-
retCode = subprocess.call(encodeArgument(cmd), shell=True)
32-
if retCode != 0:
33-
raise PostProcessingError('Command returned error code %d' % retCode)
28+
self.to_screen(f'Executing command: {cmd}')
29+
_, _, return_code = Popen.run(cmd, shell=True)
30+
if return_code != 0:
31+
raise PostProcessingError(f'Command returned error code {return_code}')
3432
return [], info
3533

3634

Diff for: yt_dlp/utils/_utils.py

+16-2
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ def _fix(key):
825825
_fix('LD_LIBRARY_PATH') # Linux
826826
_fix('DYLD_LIBRARY_PATH') # macOS
827827

828-
def __init__(self, *args, env=None, text=False, **kwargs):
828+
def __init__(self, args, *remaining, env=None, text=False, shell=False, **kwargs):
829829
if env is None:
830830
env = os.environ.copy()
831831
self._fix_pyinstaller_ld_path(env)
@@ -835,7 +835,21 @@ def __init__(self, *args, env=None, text=False, **kwargs):
835835
kwargs['universal_newlines'] = True # For 3.6 compatibility
836836
kwargs.setdefault('encoding', 'utf-8')
837837
kwargs.setdefault('errors', 'replace')
838-
super().__init__(*args, env=env, **kwargs, startupinfo=self._startupinfo)
838+
839+
if shell and compat_os_name == 'nt' and kwargs.get('executable') is None:
840+
if not isinstance(args, str):
841+
args = ' '.join(compat_shlex_quote(a) for a in args)
842+
shell = False
843+
args = f'{self.__comspec()} /Q /S /D /V:OFF /C "{args}"'
844+
845+
super().__init__(args, *remaining, env=env, shell=shell, **kwargs, startupinfo=self._startupinfo)
846+
847+
def __comspec(self):
848+
comspec = os.environ.get('ComSpec') or os.path.join(
849+
os.environ.get('SystemRoot', ''), 'System32', 'cmd.exe')
850+
if os.path.isabs(comspec):
851+
return comspec
852+
raise FileNotFoundError('shell not found: neither %ComSpec% nor %SystemRoot% is set')
839853

840854
def communicate_or_kill(self, *args, **kwargs):
841855
try:

0 commit comments

Comments
 (0)