Skip to content

Commit

Permalink
tmpdir: fix temporary directories created with world-readable permiss…
Browse files Browse the repository at this point in the history
…ions

(Written for a Unix system, but might be applicable to Windows as well).

pytest creates a root temporary directory under /tmp, named
`pytest-of-<username>`, and creates tmp_path's and other under it.
/tmp is shared between all users of the system.

This root temporary directory was created with 0o777&~umask permissions,
which usually becomes 0o755, meaning any user in the system could list
and read the files, which is undesirable.

Use 0o700 permissions instead. Also for subdirectories, because the root
dir is adjustable.
  • Loading branch information
bluetech committed Apr 3, 2021
1 parent 93dbae2 commit 9dc54f7
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 12 deletions.
5 changes: 5 additions & 0 deletions changelog/8414.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pytest used to create directories under ``/tmp`` with world-readable
permissions. This means that any user in the system was able to read
information written by tests in temporary directories (such as those created by
the ``tmp_path``/``tmpdir`` fixture). Now the directories are created with
private permissions.
8 changes: 4 additions & 4 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,15 @@ def _force_symlink(
pass


def make_numbered_dir(root: Path, prefix: str) -> Path:
def make_numbered_dir(root: Path, prefix: str, mode: int = 0o700) -> Path:
"""Create a directory with an increased number as suffix for the given prefix."""
for i in range(10):
# try up to 10 times to create the folder
max_existing = max(map(parse_num, find_suffixes(root, prefix)), default=-1)
new_number = max_existing + 1
new_path = root.joinpath(f"{prefix}{new_number}")
try:
new_path.mkdir()
new_path.mkdir(mode=mode)
except Exception:
pass
else:
Expand Down Expand Up @@ -347,13 +347,13 @@ def cleanup_numbered_dir(


def make_numbered_dir_with_cleanup(
root: Path, prefix: str, keep: int, lock_timeout: float
root: Path, prefix: str, keep: int, lock_timeout: float, mode: int,
) -> Path:
"""Create a numbered dir with a cleanup lock and remove old ones."""
e = None
for i in range(10):
try:
p = make_numbered_dir(root, prefix)
p = make_numbered_dir(root, prefix, mode)
lock_path = create_cleanup_lock(p)
register_cleanup_lock_removal(lock_path)
except Exception as exc:
Expand Down
4 changes: 2 additions & 2 deletions src/_pytest/pytester.py
Original file line number Diff line number Diff line change
Expand Up @@ -1426,7 +1426,7 @@ def runpytest_subprocess(self, *args, timeout: Optional[float] = None) -> RunRes
:rtype: RunResult
"""
__tracebackhide__ = True
p = make_numbered_dir(root=self.path, prefix="runpytest-")
p = make_numbered_dir(root=self.path, prefix="runpytest-", mode=0o700)
args = ("--basetemp=%s" % p,) + args
plugins = [x for x in self.plugins if isinstance(x, str)]
if plugins:
Expand All @@ -1445,7 +1445,7 @@ def spawn_pytest(
The pexpect child is returned.
"""
basetemp = self.path / "temp-pexpect"
basetemp.mkdir()
basetemp.mkdir(mode=0o700)
invoke = " ".join(map(str, self._getpytestargs()))
cmd = f"{invoke} --basetemp={basetemp} {string}"
return self.spawn(cmd, expect_timeout=expect_timeout)
Expand Down
16 changes: 10 additions & 6 deletions src/_pytest/tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,22 +90,22 @@ def mktemp(self, basename: str, numbered: bool = True) -> Path:
basename = self._ensure_relative_to_basetemp(basename)
if not numbered:
p = self.getbasetemp().joinpath(basename)
p.mkdir()
p.mkdir(mode=0o700)
else:
p = make_numbered_dir(root=self.getbasetemp(), prefix=basename)
p = make_numbered_dir(root=self.getbasetemp(), prefix=basename, mode=0o700)
self._trace("mktemp", p)
return p

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

if self._given_basetemp is not None:
basetemp = self._given_basetemp
if basetemp.exists():
rm_rf(basetemp)
basetemp.mkdir()
basetemp.mkdir(mode=0o700)
basetemp = basetemp.resolve()
else:
from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT")
Expand All @@ -114,9 +114,13 @@ def getbasetemp(self) -> Path:
# use a sub-directory in the temproot to speed-up
# make_numbered_dir() call
rootdir = temproot.joinpath(f"pytest-of-{user}")
rootdir.mkdir(exist_ok=True)
rootdir.mkdir(mode=0o700, exist_ok=True)
basetemp = make_numbered_dir_with_cleanup(
prefix="pytest-", root=rootdir, keep=3, lock_timeout=LOCK_TIMEOUT
prefix="pytest-",
root=rootdir,
keep=3,
lock_timeout=LOCK_TIMEOUT,
mode=0o700,
)
assert basetemp is not None, basetemp
self._basetemp = basetemp
Expand Down
16 changes: 16 additions & 0 deletions testing/test_tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,19 @@ def test(tmp_path):
# running a second time and ensure we don't crash
result = pytester.runpytest("--basetemp=tmp")
assert result.ret == 0


@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions")
def test_tmp_path_factory_create_directory_with_safe_permissions(
tmp_path: Path, monkeypatch,
) -> None:
"""Verify that pytest creates directories under /tmp with private permissions."""
# Use the test's tmp_path as the system temproot (/tmp).
monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path))
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
basetemp = tmp_factory.getbasetemp()

# No world-readable permissions.
assert (basetemp.stat().st_mode & 0o077) == 0
# Parent too (pytest-of-foo).
assert (basetemp.parent.stat().st_mode & 0o077) == 0

0 comments on commit 9dc54f7

Please sign in to comment.