Skip to content

bpo-35278: Sanitize tempfile prefix to prevent directory treversal #10627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bpo-35278: Sanitize tempfile prefix to prevent directory treversal #10627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Lib/tempfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ def _stat(fn):
fd = _os.open(fn, _os.O_RDONLY)
_os.close(fd)

if _os.altsep is None:
_path_separators = (_os.sep,)
else:
_path_separators = (_os.sep, _os.altsep)


def _exists(fn):
try:
_stat(fn)
Expand Down Expand Up @@ -121,6 +127,12 @@ def _sanitize_params(prefix, suffix, dir):
prefix = template
else:
prefix = _os.fsencode(template)
if output_type is str:
if any(sep in prefix for sep in _path_separators):
Comment on lines +130 to +131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply test that os.path.dirname(prefix+suffix) is empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer to reuse dirname() function.

raise ValueError("Prefix contains system separator character")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is called a pathname components separator.

else:
if any(_os.fsencode(sep) in prefix for sep in _path_separators):
raise ValueError("Prefix contains system separator character")
if dir is None:
if output_type is str:
dir = gettempdir()
Expand Down
17 changes: 17 additions & 0 deletions Lib/test/test_tempfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,23 @@ def test_usable_template(self):
os.rmdir(d)


class TestSanitizeParamsInner(BaseTestCase):
"""Test the internal function _sanitize_params."""

def test_throw_exception_on_path_separator_detection(self):
with self.assertRaises(ValueError):
tempfile.mkstemp(prefix=f"{os.sep}home")

def test_throw_exception_on_encoded_path_separator_detection(self):
with self.assertRaises(ValueError):
tempfile.mkstemp(prefix=f"{os.fsencode(os.sep)}home")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work as you expected. On Posix it generates prefix "b'/'home".

Use os.fsencode(f"{os.sep}home").


@unittest.skipIf(os.altsep is None, "os.altsep is not present on this platform")
def test_throw_exception_on_alternative_path_separator_detection(self):
with self.assertRaises(ValueError):
tempfile.mkstemp(prefix=f"{os.altsep}home")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need also a test for bytes prefix with altsep.

Add also tests for suffix containing a pathname components separator.

Add also tests for other functions that use _sanitize_params().

It will be too wasteful to add 24 separate test methods, so you should test several cases per method method.



class TestGetTempDir(BaseTestCase):
"""Test gettempdir()."""

Expand Down