From 10683c0c121e38d8ef60d494b72579b016372592 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 6 Jun 2023 16:06:57 +0200 Subject: [PATCH 1/4] python:tests: Adopt safe_tarfile for extraction_filter raises BUG: https://bugzilla.samba.org/show_bug.cgi?id=15390 Signed-off-by: Andreas Schneider Reviewed-by: Douglas Bagnall (cherry picked from commit ebaa00816259cbae5c45ebf0ba5fb260b09e4695) --- python/samba/tests/safe_tarfile.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/python/samba/tests/safe_tarfile.py b/python/samba/tests/safe_tarfile.py index 40aa9e17d4a..cf9b725afcb 100644 --- a/python/samba/tests/safe_tarfile.py +++ b/python/samba/tests/safe_tarfile.py @@ -43,9 +43,16 @@ class SafeTarFileTestCase(TestCaseInTempDir): stf = safe_tarfile.open(tarname) - self.assertRaises(tarfile.ExtractError, - stf.extractall, - tarname) + # We we have data_filter, we have a patched python to address + # CVE-2007-4559. + if hasattr(tarfile, "data_filter"): + self.assertRaises(tarfile.OutsideDestinationError, + stf.extractall, + tarname) + else: + self.assertRaises(tarfile.ExtractError, + stf.extractall, + tarname) self.rm_files('x', 'tar.tar') def test_slash(self): @@ -60,8 +67,16 @@ class SafeTarFileTestCase(TestCaseInTempDir): tf.close() stf = safe_tarfile.open(tarname) - self.assertRaises(tarfile.ExtractError, - stf.extractall, - tarname) + + # We we have data_filter, we have a patched python to address + # CVE-2007-4559. + if hasattr(tarfile, "data_filter"): + self.assertRaises(NotADirectoryError, + stf.extractall, + tarname) + else: + self.assertRaises(tarfile.ExtractError, + stf.extractall, + tarname) self.rm_files('x', 'tar.tar') -- 2.41.0 From 8dd8ea221b040173e55dac57cdb58a5e792bbd16 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 6 Jun 2023 15:29:06 +0200 Subject: [PATCH 2/4] python:safe_tarfile: Set extraction_filter for pythons providing it It should be available for Python >= 3.11.4 but also has been backported. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15390 Signed-off-by: Andreas Schneider Reviewed-by: Douglas Bagnall (cherry picked from commit 8c90c66a9a409d807dad56822540509c9813425b) --- python/samba/safe_tarfile.py | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/python/samba/safe_tarfile.py b/python/samba/safe_tarfile.py index cc19770d73f..164bb0b31fe 100644 --- a/python/samba/safe_tarfile.py +++ b/python/samba/safe_tarfile.py @@ -15,6 +15,7 @@ # along with this program. If not, see . +import tarfile from tarfile import ExtractError, TarInfo, TarFile as UnsafeTarFile @@ -24,20 +25,27 @@ class TarFile(UnsafeTarFile): using '../../'. """ - def extract(self, member, path="", set_attrs=True, *, numeric_owner=False): - if isinstance(member, TarInfo): - name = member.name - else: - name = member - - if '../' in name: - raise ExtractError(f"'../' is not allowed in path '{name}'") - - if name.startswith('/'): - raise ExtractError(f"path '{name}' should not start with '/'") - - super().extract(member, path, set_attrs=set_attrs, - numeric_owner=numeric_owner) + try: + # New in version 3.11.4 (also has been backported) + # https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extraction_filter + # https://peps.python.org/pep-0706/ + extraction_filter = staticmethod(tarfile.data_filter) + except AttributeError: + def extract(self, member, path="", set_attrs=True, *, + numeric_owner=False): + if isinstance(member, TarInfo): + name = member.name + else: + name = member + + if '../' in name: + raise ExtractError(f"'../' is not allowed in path '{name}'") + + if name.startswith('/'): + raise ExtractError(f"path '{name}' should not start with '/'") + + super().extract(member, path, set_attrs=set_attrs, + numeric_owner=numeric_owner) open = TarFile.open -- 2.41.0 From cac6b5ad93e1b97be09faa5cb69f1f5b9072d30e Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 6 Jun 2023 15:30:20 +0200 Subject: [PATCH 3/4] python:safe_tarfile: Implement safer extractall() This also checks for symlinks and hardlinks. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15390 Signed-off-by: Andreas Schneider Reviewed-by: Douglas Bagnall (cherry picked from commit 431f7698e48387413aac586c7a939a1682464681) --- python/samba/safe_tarfile.py | 53 ++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/python/samba/safe_tarfile.py b/python/samba/safe_tarfile.py index 164bb0b31fe..8535f54f5b3 100644 --- a/python/samba/safe_tarfile.py +++ b/python/samba/safe_tarfile.py @@ -15,7 +15,9 @@ # along with this program. If not, see . +import os import tarfile +from pathlib import Path from tarfile import ExtractError, TarInfo, TarFile as UnsafeTarFile @@ -47,5 +49,56 @@ class TarFile(UnsafeTarFile): super().extract(member, path, set_attrs=set_attrs, numeric_owner=numeric_owner) + def extractall(self, path, members=None, *, numeric_owner=False): + self._safetarfile_check() + super().extractall(path, members, + numeric_owner=numeric_owner) + + def _safetarfile_check(self): + for tarinfo in self.__iter__(): + if self._is_traversal_attempt(tarinfo=tarinfo): + raise ExtractError( + "Attempted directory traversal for " + f"member: {tarinfo.name}") + if self._is_unsafe_symlink(tarinfo=tarinfo): + raise ExtractError( + "Attempted directory traversal via symlink for " + f"member: {tarinfo.linkname}") + if self._is_unsafe_link(tarinfo=tarinfo): + raise ExtractError( + "Attempted directory traversal via link for " + f"member: {tarinfo.linkname}") + + def _resolve_path(self, path): + return os.path.realpath(os.path.abspath(path)) + + def _is_path_in_dir(self, path, basedir): + return self._resolve_path(os.path.join(basedir, + path)).startswith(basedir) + + def _is_traversal_attempt(self, tarinfo): + if (tarinfo.name.startswith(os.sep) + or ".." + os.sep in tarinfo.name): + return True + return False + + def _is_unsafe_symlink(self, tarinfo): + if tarinfo.issym(): + symlink_file = Path( + os.path.normpath(os.path.join(os.getcwd(), + tarinfo.linkname))) + if not self._is_path_in_dir(symlink_file, os.getcwd()): + return True + return False + + def _is_unsafe_link(self, tarinfo): + if tarinfo.islnk(): + link_file = Path( + os.path.normpath(os.path.join(os.getcwd(), + tarinfo.linkname))) + if not self._is_path_in_dir(link_file, os.getcwd()): + return True + return False + open = TarFile.open -- 2.41.0 From fb45966ea33df8905fc9555ee66ebcb1c3ae278e Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 6 Jun 2023 15:38:12 +0200 Subject: [PATCH 4/4] python:safe_tarfile: Improve safe extract() This also checks for symlinks and hardlinks. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15390 Signed-off-by: Andreas Schneider Reviewed-by: Douglas Bagnall (cherry picked from commit 1f74f9f366d7f107a89220a4a5951bc4daf18025) --- python/samba/safe_tarfile.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/python/samba/safe_tarfile.py b/python/samba/safe_tarfile.py index 8535f54f5b3..7a2b0382a79 100644 --- a/python/samba/safe_tarfile.py +++ b/python/samba/safe_tarfile.py @@ -35,17 +35,7 @@ class TarFile(UnsafeTarFile): except AttributeError: def extract(self, member, path="", set_attrs=True, *, numeric_owner=False): - if isinstance(member, TarInfo): - name = member.name - else: - name = member - - if '../' in name: - raise ExtractError(f"'../' is not allowed in path '{name}'") - - if name.startswith('/'): - raise ExtractError(f"path '{name}' should not start with '/'") - + self._safetarfile_check() super().extract(member, path, set_attrs=set_attrs, numeric_owner=numeric_owner) -- 2.41.0