Skip to content

Commit 51ddb4e

Browse files
authored
don't follow symlinks in the debugger tmp directory (#1575)
Debugger.start only creates its scratch directory when Path(tmp_dir).exists() is false, and get_tmp_directory returns the completely predictable <tempdir>/ipykernel_<pid>. Path.exists follows symlinks, so on a host with a shared temp directory another local user can reach that name first and point it at a directory they own; the kernel then skips the mkdir, the 0o700 mode from #1530 never applies, and dumpCell writes the cell source straight through the link. I went back to start after that permissions change and noticed the mkdir is reachable only when the path does not already exist, which is the one case someone else gets to control. Planting the predictable name as a symlink locally confirmed it: start carried on, dumpCell dropped the cell text into the other directory, and a symlink planted under the Murmur2 cell name turned the same write into an overwrite of a file outside the temp tree. Creating the directory first and only accepting an existing one when lstat says it is a real directory owned by this user closes that, and opening the dumped cell with O_NOFOLLOW keeps the file write itself from being redirected; the cell file now lands at 0o600 instead of whatever the umask gives, matching the directory around it. Added two regressions beside the existing debugger tests, both skipped when debugpy is absent and on Windows where symlinks need privileges and O_NOFOLLOW does not exist.
2 parents 3f179a8 + ad17040 commit 51ddb4e

2 files changed

Lines changed: 61 additions & 2 deletions

File tree

‎ipykernel/debugger.py‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import ast
44
import os
55
import re
6+
import stat
67
import sys
78
import typing as t
89
from pathlib import Path
@@ -305,6 +306,18 @@ async def send_dap_request(self, msg):
305306
return rep
306307

307308

309+
def _is_own_directory(path):
310+
"""Whether path is a directory, and not a symlink, belonging to this user."""
311+
try:
312+
st = os.lstat(path)
313+
except OSError:
314+
return False
315+
if not stat.S_ISDIR(st.st_mode):
316+
return False
317+
getuid = getattr(os, "getuid", None)
318+
return getuid is None or st.st_uid == getuid()
319+
320+
308321
class Debugger:
309322
"""The debugger class."""
310323

@@ -428,8 +441,16 @@ def start(self):
428441
"""Start the debugger."""
429442
if not self.debugpy_initialized:
430443
tmp_dir = get_tmp_directory()
431-
if not Path(tmp_dir).exists():
444+
try:
432445
Path(tmp_dir).mkdir(mode=0o700, parents=True)
446+
except FileExistsError:
447+
if not _is_own_directory(tmp_dir):
448+
self.log.error(
449+
"Not starting the debugger: %s already exists and is not a"
450+
" directory belonging to this user",
451+
tmp_dir,
452+
)
453+
return False
433454
host, port = self.debugpy_client.get_host_port()
434455
code = "import debugpy;"
435456
code += 'debugpy.listen(("' + host + '",' + port + "))"
@@ -471,7 +492,9 @@ async def dumpCell(self, message):
471492
code = message["arguments"]["code"]
472493
file_name = get_file_name(code)
473494

474-
with open(file_name, "w", encoding="utf-8") as f: # noqa: ASYNC230
495+
flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0)
496+
fd = os.open(file_name, flags, 0o600)
497+
with open(fd, "w", encoding="utf-8") as f: # noqa: ASYNC230
475498
f.write(code)
476499

477500
return {

‎tests/test_debugger.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import logging
12
import sys
23

34
import pytest
@@ -431,6 +432,41 @@ def test_convert_to_long_pathname():
431432
_convert_to_long_pathname(__file__)
432433

433434

435+
@pytest.mark.skipif(debugpy is None, reason="requires debugpy")
436+
@pytest.mark.skipif(sys.platform == "win32", reason="symlinks need privileges on windows")
437+
def test_start_rejects_foreign_tmp_directory(tmp_path, monkeypatch):
438+
from ipykernel.debugger import Debugger
439+
440+
# <tempdir>/ipykernel_<pid> is predictable, so on a shared temp directory
441+
# another user can get there first and point it at a path they own.
442+
elsewhere = tmp_path / "elsewhere"
443+
elsewhere.mkdir()
444+
planted = tmp_path / "ipykernel_1"
445+
planted.symlink_to(elsewhere, target_is_directory=True)
446+
monkeypatch.setattr("ipykernel.debugger.get_tmp_directory", lambda: str(planted))
447+
448+
debugger = Debugger(logging.getLogger(__name__), None, lambda event: None, None, None, [])
449+
assert debugger.start() is False
450+
451+
452+
@pytest.mark.skipif(debugpy is None, reason="requires debugpy")
453+
@pytest.mark.skipif(sys.platform == "win32", reason="O_NOFOLLOW is posix only")
454+
async def test_dump_cell_does_not_follow_symlink(tmp_path, monkeypatch):
455+
from ipykernel.debugger import Debugger
456+
457+
outside = tmp_path / "outside.py"
458+
outside.write_text("# untouched\n")
459+
cell_file = tmp_path / "cell.py"
460+
cell_file.symlink_to(outside)
461+
monkeypatch.setenv("IPYKERNEL_CELL_NAME", str(cell_file))
462+
463+
debugger = Debugger(logging.getLogger(__name__), None, lambda event: None, None, None, [])
464+
message = {"seq": 1, "command": "dumpCell", "arguments": {"code": "x = 1\n"}}
465+
with pytest.raises(OSError, match="symbolic link"):
466+
await debugger.dumpCell(message)
467+
assert outside.read_text() == "# untouched\n"
468+
469+
434470
def test_copy_to_globals(kernel_with_debug):
435471
local_var_name = "var"
436472
global_var_name = "var_copy"

0 commit comments

Comments
 (0)