Skip to content

Commit 07c6b3f

Browse files
authored
Merge pull request #8516 from bluetech/mktmp
Fix minor temporary directory security issue
2 parents 9c151a6 + c49100c commit 07c6b3f

File tree

5 files changed

+97
-24
lines changed

5 files changed

+97
-24
lines changed

changelog/8414.bugfix.rst

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
pytest used to create directories under ``/tmp`` with world-readable
2+
permissions. This means that any user in the system was able to read
3+
information written by tests in temporary directories (such as those created by
4+
the ``tmp_path``/``tmpdir`` fixture). Now the directories are created with
5+
private permissions.
6+
7+
pytest used silenty use a pre-existing ``/tmp/pytest-of-<username>`` directory,
8+
even if owned by another user. This means another user could pre-create such a
9+
directory and gain control of another user's temporary directory. Now such a
10+
condition results in an error.

src/_pytest/pathlib.py

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,13 +62,6 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath:
6262
return path.joinpath(".lock")
6363

6464

65-
def ensure_reset_dir(path: Path) -> None:
66-
"""Ensure the given path is an empty directory."""
67-
if path.exists():
68-
rm_rf(path)
69-
path.mkdir()
70-
71-
7265
def on_rm_rf_error(func, path: str, exc, *, start_path: Path) -> bool:
7366
"""Handle known read-only errors during rmtree.
7467
@@ -212,15 +205,15 @@ def _force_symlink(
212205
pass
213206

214207

215-
def make_numbered_dir(root: Path, prefix: str) -> Path:
208+
def make_numbered_dir(root: Path, prefix: str, mode: int = 0o700) -> Path:
216209
"""Create a directory with an increased number as suffix for the given prefix."""
217210
for i in range(10):
218211
# try up to 10 times to create the folder
219212
max_existing = max(map(parse_num, find_suffixes(root, prefix)), default=-1)
220213
new_number = max_existing + 1
221214
new_path = root.joinpath(f"{prefix}{new_number}")
222215
try:
223-
new_path.mkdir()
216+
new_path.mkdir(mode=mode)
224217
except Exception:
225218
pass
226219
else:
@@ -352,13 +345,17 @@ def cleanup_numbered_dir(
352345

353346

354347
def make_numbered_dir_with_cleanup(
355-
root: Path, prefix: str, keep: int, lock_timeout: float
348+
root: Path,
349+
prefix: str,
350+
keep: int,
351+
lock_timeout: float,
352+
mode: int,
356353
) -> Path:
357354
"""Create a numbered dir with a cleanup lock and remove old ones."""
358355
e = None
359356
for i in range(10):
360357
try:
361-
p = make_numbered_dir(root, prefix)
358+
p = make_numbered_dir(root, prefix, mode)
362359
lock_path = create_cleanup_lock(p)
363360
register_cleanup_lock_removal(lock_path)
364361
except Exception as exc:

src/_pytest/pytester.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,7 +1456,7 @@ def runpytest_subprocess(
14561456
:py:class:`Pytester.TimeoutExpired`.
14571457
"""
14581458
__tracebackhide__ = True
1459-
p = make_numbered_dir(root=self.path, prefix="runpytest-")
1459+
p = make_numbered_dir(root=self.path, prefix="runpytest-", mode=0o700)
14601460
args = ("--basetemp=%s" % p,) + args
14611461
plugins = [x for x in self.plugins if isinstance(x, str)]
14621462
if plugins:
@@ -1475,7 +1475,7 @@ def spawn_pytest(
14751475
The pexpect child is returned.
14761476
"""
14771477
basetemp = self.path / "temp-pexpect"
1478-
basetemp.mkdir()
1478+
basetemp.mkdir(mode=0o700)
14791479
invoke = " ".join(map(str, self._getpytestargs()))
14801480
cmd = f"{invoke} --basetemp={basetemp} {string}"
14811481
return self.spawn(cmd, expect_timeout=expect_timeout)

src/_pytest/tmpdir.py

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77

88
import attr
99

10-
from .pathlib import ensure_reset_dir
1110
from .pathlib import LOCK_TIMEOUT
1211
from .pathlib import make_numbered_dir
1312
from .pathlib import make_numbered_dir_with_cleanup
13+
from .pathlib import rm_rf
1414
from _pytest.compat import final
1515
from _pytest.compat import LEGACY_PATH
1616
from _pytest.compat import legacy_path
@@ -94,20 +94,22 @@ def mktemp(self, basename: str, numbered: bool = True) -> Path:
9494
basename = self._ensure_relative_to_basetemp(basename)
9595
if not numbered:
9696
p = self.getbasetemp().joinpath(basename)
97-
p.mkdir()
97+
p.mkdir(mode=0o700)
9898
else:
99-
p = make_numbered_dir(root=self.getbasetemp(), prefix=basename)
99+
p = make_numbered_dir(root=self.getbasetemp(), prefix=basename, mode=0o700)
100100
self._trace("mktemp", p)
101101
return p
102102

103103
def getbasetemp(self) -> Path:
104-
"""Return base temporary directory."""
104+
"""Return the base temporary directory, creating it if needed."""
105105
if self._basetemp is not None:
106106
return self._basetemp
107107

108108
if self._given_basetemp is not None:
109109
basetemp = self._given_basetemp
110-
ensure_reset_dir(basetemp)
110+
if basetemp.exists():
111+
rm_rf(basetemp)
112+
basetemp.mkdir(mode=0o700)
111113
basetemp = basetemp.resolve()
112114
else:
113115
from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT")
@@ -117,18 +119,41 @@ def getbasetemp(self) -> Path:
117119
# make_numbered_dir() call
118120
rootdir = temproot.joinpath(f"pytest-of-{user}")
119121
try:
120-
rootdir.mkdir(exist_ok=True)
122+
rootdir.mkdir(mode=0o700, exist_ok=True)
121123
except OSError:
122124
# getuser() likely returned illegal characters for the platform, use unknown back off mechanism
123125
rootdir = temproot.joinpath("pytest-of-unknown")
124-
rootdir.mkdir(exist_ok=True)
126+
rootdir.mkdir(mode=0o700, exist_ok=True)
127+
# Because we use exist_ok=True with a predictable name, make sure
128+
# we are the owners, to prevent any funny business (on unix, where
129+
# temproot is usually shared).
130+
# Also, to keep things private, fixup any world-readable temp
131+
# rootdir's permissions. Historically 0o755 was used, so we can't
132+
# just error out on this, at least for a while.
133+
if hasattr(os, "getuid"):
134+
rootdir_stat = rootdir.stat()
135+
uid = os.getuid()
136+
# getuid shouldn't fail, but cpython defines such a case.
137+
# Let's hope for the best.
138+
if uid != -1:
139+
if rootdir_stat.st_uid != uid:
140+
raise OSError(
141+
f"The temporary directory {rootdir} is not owned by the current user. "
142+
"Fix this and try again."
143+
)
144+
if (rootdir_stat.st_mode & 0o077) != 0:
145+
os.chmod(rootdir, rootdir_stat.st_mode & ~0o077)
125146
basetemp = make_numbered_dir_with_cleanup(
126-
prefix="pytest-", root=rootdir, keep=3, lock_timeout=LOCK_TIMEOUT
147+
prefix="pytest-",
148+
root=rootdir,
149+
keep=3,
150+
lock_timeout=LOCK_TIMEOUT,
151+
mode=0o700,
127152
)
128153
assert basetemp is not None, basetemp
129-
self._basetemp = t = basetemp
130-
self._trace("new basetemp", t)
131-
return t
154+
self._basetemp = basetemp
155+
self._trace("new basetemp", basetemp)
156+
return basetemp
132157

133158

134159
@final

testing/test_tmpdir.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,3 +454,44 @@ def test_tmp_path_factory_handles_invalid_dir_characters(
454454
monkeypatch.setattr(tmp_path_factory, "_given_basetemp", None)
455455
p = tmp_path_factory.getbasetemp()
456456
assert "pytest-of-unknown" in str(p)
457+
458+
459+
@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions")
460+
def test_tmp_path_factory_create_directory_with_safe_permissions(
461+
tmp_path: Path, monkeypatch: MonkeyPatch
462+
) -> None:
463+
"""Verify that pytest creates directories under /tmp with private permissions."""
464+
# Use the test's tmp_path as the system temproot (/tmp).
465+
monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path))
466+
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
467+
basetemp = tmp_factory.getbasetemp()
468+
469+
# No world-readable permissions.
470+
assert (basetemp.stat().st_mode & 0o077) == 0
471+
# Parent too (pytest-of-foo).
472+
assert (basetemp.parent.stat().st_mode & 0o077) == 0
473+
474+
475+
@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions")
476+
def test_tmp_path_factory_fixes_up_world_readable_permissions(
477+
tmp_path: Path, monkeypatch: MonkeyPatch
478+
) -> None:
479+
"""Verify that if a /tmp/pytest-of-foo directory already exists with
480+
world-readable permissions, it is fixed.
481+
482+
pytest used to mkdir with such permissions, that's why we fix it up.
483+
"""
484+
# Use the test's tmp_path as the system temproot (/tmp).
485+
monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path))
486+
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
487+
basetemp = tmp_factory.getbasetemp()
488+
489+
# Before - simulate bad perms.
490+
os.chmod(basetemp.parent, 0o777)
491+
assert (basetemp.parent.stat().st_mode & 0o077) != 0
492+
493+
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
494+
basetemp = tmp_factory.getbasetemp()
495+
496+
# After - fixed.
497+
assert (basetemp.parent.stat().st_mode & 0o077) == 0

0 commit comments

Comments
 (0)