From 982230c3f1bc1f757208fc9afbcd451a77948824 Mon Sep 17 00:00:00 2001 From: hnnasit <84355507+hnnasit@users.noreply.github.com> Date: Mon, 16 Jan 2023 13:16:25 -0800 Subject: [PATCH 1/3] Revert "Revert "fix: Add back the fixed CVE-2007-4559 Patch changes (#4539)" (#4560)" This reverts commit 6aab7de3311b16fccc35f6784329bcefab3d866a. --- .gitignore | 5 ++ samcli/lib/utils/tar.py | 25 +++++++++ samcli/local/docker/container.py | 8 +-- tests/functional/lib/utils/__init__.py | 0 tests/functional/lib/utils/test_tar.py | 29 ++++++++++ .../testdata/lib/utils/path_reversal_uxix.tgz | Bin 0 -> 800 bytes .../testdata/lib/utils/path_reversal_win.tgz | Bin 0 -> 799 bytes tests/functional/testdata/lib/utils/test.tgz | Bin 0 -> 769 bytes tests/unit/lib/utils/test_tar.py | 53 +++++++++++++++++- tests/unit/local/docker/test_container.py | 14 ++--- 10 files changed, 120 insertions(+), 14 deletions(-) create mode 100644 tests/functional/lib/utils/__init__.py create mode 100644 tests/functional/lib/utils/test_tar.py create mode 100644 tests/functional/testdata/lib/utils/path_reversal_uxix.tgz create mode 100644 tests/functional/testdata/lib/utils/path_reversal_win.tgz create mode 100644 tests/functional/testdata/lib/utils/test.tgz diff --git a/.gitignore b/.gitignore index 14fd530d9f6..d8fce677108 100644 --- a/.gitignore +++ b/.gitignore @@ -163,6 +163,11 @@ typings/ # Output of 'npm pack' *.tgz +# Except test file +!tests/functional/testdata/lib/utils/test.tgz +!tests/functional/testdata/lib/utils/path_reversal_uxix.tgz +!tests/functional/testdata/lib/utils/path_reversal_win.tgz + # Yarn Integrity file .yarn-integrity diff --git a/samcli/lib/utils/tar.py b/samcli/lib/utils/tar.py index d080e10294f..4963b3ebd0e 100644 --- a/samcli/lib/utils/tar.py +++ b/samcli/lib/utils/tar.py @@ -2,7 +2,9 @@ Tarball Archive utility """ +import os import tarfile +from typing import Union from tempfile import TemporaryFile from contextlib import contextmanager @@ -39,3 +41,26 @@ def create_tarball(tar_paths, tar_filter=None, mode="w"): yield tarballfile finally: tarballfile.close() + + +def _is_within_directory(directory: Union[str, os.PathLike], target: Union[str, os.PathLike]) -> bool: + """Checks if target is located under directory""" + abs_directory = os.path.abspath(directory) + abs_target = os.path.abspath(target) + + prefix = os.path.commonprefix([abs_directory, abs_target]) + + return bool(prefix == abs_directory) + + +def extract_tarfile(tarfile_path: Union[str, os.PathLike], unpack_dir: Union[str, os.PathLike]) -> None: + """Extracts a tarfile""" + with tarfile.open(tarfile_path, "r:*") as tar: + # Makes sure the tar file is sanitized and is free of directory traversal vulnerability + # See: https://github.com/advisories/GHSA-gw9q-c7gh-j9vm + for member in tar.getmembers(): + member_path = os.path.join(unpack_dir, member.name) + if not _is_within_directory(unpack_dir, member_path): + raise tarfile.ExtractError("Attempted Path Traversal in Tar File") + + tar.extractall(unpack_dir) diff --git a/samcli/local/docker/container.py b/samcli/local/docker/container.py index 46c8fe6467f..b73e21e7236 100644 --- a/samcli/local/docker/container.py +++ b/samcli/local/docker/container.py @@ -3,7 +3,6 @@ """ import os import logging -import tarfile import tempfile import threading import socket @@ -14,6 +13,7 @@ from docker.errors import NotFound as DockerNetworkNotFound from samcli.lib.utils.retry import retry +from samcli.lib.utils.tar import extract_tarfile from .exceptions import ContainerNotStartableException from .utils import to_posix_path, find_free_port, NoFreePortsError @@ -362,7 +362,8 @@ def _can_connect_to_socket(self) -> bool: a_socket.close() return connection_succeeded - def copy(self, from_container_path, to_host_path): + def copy(self, from_container_path, to_host_path) -> None: + """Copies a path from container into host path""" if not self.is_created(): raise RuntimeError("Container does not exist. Cannot get logs for this container") @@ -378,8 +379,7 @@ def copy(self, from_container_path, to_host_path): # Seek the handle back to start of file for tarfile to use fp.seek(0) - with tarfile.open(fileobj=fp, mode="r") as tar: - tar.extractall(path=to_host_path) + extract_tarfile(tarfile_path=fp.name, unpack_dir=to_host_path) @staticmethod def _write_container_output(output_itr, stdout=None, stderr=None): diff --git a/tests/functional/lib/utils/__init__.py b/tests/functional/lib/utils/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/functional/lib/utils/test_tar.py b/tests/functional/lib/utils/test_tar.py new file mode 100644 index 00000000000..59a4c5d481a --- /dev/null +++ b/tests/functional/lib/utils/test_tar.py @@ -0,0 +1,29 @@ +import os +import tempfile +import shutil +import platform +from pathlib import Path +from tarfile import ExtractError + +from unittest import TestCase + +from samcli.lib.utils.tar import extract_tarfile + + +class TestExtractTarFile(TestCase): + def test_extract_tarfile_unpacks_a_tar(self): + test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", "test.tgz") + test_dir = tempfile.mkdtemp() + extract_tarfile(test_tar, test_dir) + output_files = set(os.listdir(test_dir)) + shutil.rmtree(test_dir) + print(output_files) + self.assertEqual({"test_utils.py"}, output_files) + + def test_raise_exception_for_unsafe_tarfile(self): + tar_filename = "path_reversal_win.tgz" if platform.system().lower() == "windows" else "path_reversal_uxix.tgz" + test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", tar_filename) + test_dir = tempfile.mkdtemp() + self.assertRaisesRegex( + ExtractError, "Attempted Path Traversal in Tar File", extract_tarfile, test_tar, test_dir + ) diff --git a/tests/functional/testdata/lib/utils/path_reversal_uxix.tgz b/tests/functional/testdata/lib/utils/path_reversal_uxix.tgz new file mode 100644 index 0000000000000000000000000000000000000000..876b0b7b2105ed19c14fd05319e1d13d15679a85 GIT binary patch literal 800 zcmV+*1K<1~iwFp=9DQQ~|8QY+XkT(=c4cyNVQgP@cxiYpbZ2@1?NnWB+b|UMv!C`8 zO!i;}f|j&S8H2!H_q2y?Ph$whzHZ(6L+FDvjKO}%e(SCrTea&nT^U^&=sYBnbNkwYTf|9^s_5{=Z6*xN1$!)~hf%8i@5p}Mkd z;-(rZkryR|VJ@30;ycAkUTrhZwx%dEkc#dad6SpTPSLu_;99X-R8dE7TP_U4_lM1C zUfgOUAw$)sm;0 zq1RS1GqvF9jbhw>AqvWT(eH`WmTS>+ZGUWVl?tw}=yg-nq@QGkta$~5u{JB4FQQq? z$++gTZbC7^Y<@2(GBIgiL0a8pc1#9gog8{kI_WhnT%sY#Xkm1#Mqz#h+G;Her7?~; z!hrF*bCt?UO8{~tsgOmDc?PK`i@m7h3*&U?7@+;%XJ`1{%zi|h%}91QcCITiJG6kZ zbM&{7dzLqnSr;bu^&556*1SV4d%v*1v96FB&9@~?wK?XU?T}*5VVczGwq4x{w*%FxWod)=w1!%%CkXEo zk(kGeDBjwiH2xEH?ZrCAaw0_|pk~t8x-ub~6cmo?%vBrP2t~uga?@SL#Qq7lIi@sr zzgC33*sZ!5){RZ6~&du7pW|83zoQ{WDGij7ND2zVqMq#T8xh#o%_iwLPG zT2L%nW$Gg38pzZtK@as!@ddYrEvF4>G|j-Sli29|Z9Jz+%L1RX&frVXcy@B!(3IDV zrzr@{Y|zQa*p|P0JO9qYry1AV{mj;#3P%N-VFg~y%h6LZRup)6czAetczAetczAet eczAetczAetczAetczAfc6n+EbNc@lhC;$NK36ZM+ literal 0 HcmV?d00001 diff --git a/tests/functional/testdata/lib/utils/path_reversal_win.tgz b/tests/functional/testdata/lib/utils/path_reversal_win.tgz new file mode 100644 index 0000000000000000000000000000000000000000..428bb48d0aa2b926ad0d61aadc1ba7fe59638753 GIT binary patch literal 799 zcmV+)1K|80iwFqj9DQQ~|8QY+XkT(=c4cyNVQgP_X>KlbXL z_Fx5!NZO=~L13?Y+QYV|Z3xA_Zr%Dr=z}zj!G6hp>#h{rwd*up8C@CZJS38J&(+aA zSEBT*2Bn$NMX9LR2hV^7;$pQ5e&YD{*ZWs?zBpUC?}*iMxek`+tHpA4zP>nH2aDx; zdA}_ zq@QZ2L|&8-hPiC2i0>3Dd9}?r+nS=xKq|UxNSX&hWjN{fIW3k?e5nTw7vxXaQyC z=x-zUEN>*UE==s}HfpKPyhANJzp%ftu8kD?o9;3u_D{IYF{N?; zYem?L?bPiu?B3zQ#GndJDfu$)m89MNTheo;z#Z@un?6w?;E`~Uauj|ddi+o%5mHgK zpjfoZ)J4iQkf~LI9_pLo3vLZtOdHZ@nt@#>vC;XPcutj;1wLo3!Iz-%?BuwiDX$q% zQxKZjpp%cWEr0uV{+)$SGp@7ynXNk&jtVxz3cQ$?qo-u7DDd#`@bK{P@bK{P@bK{P d@bK{P@bK{P@bK{P@bGvk{07zAt^)ul006{`|rE*hicbZMj54y={(p{bkEh%J=Y=zv|)`YN=@bI>UEGL$?bYg?ECI^)4r4I zZf>z&tu|zJvrbm)o6YU@hTve8++C66mGn%|Xu}msxflC`xDbb7d46HNI}35|e*{G( zg)&5*3^qa#^izG(ZY-XL`oVUJn;NKOUX&07xe^uON6kuJ?J~}GO;Ki`G(8G+$V+k5 zv=$lcHLGPsdV1S+K@fcGHm7;r+e2!4o=s(QGk7$Lr-q@|RxxX8$o-2M`2DD!yO6R9oNqUB-)+2Sgd+&s{| zDC%gKWQD4E1rcLyRlWeyW*}>GgzQn9+0d42#uTUqJ3&pGpGy8^(dTMK4SIfaK>~E|qq>bUb66V?* z^U3#%RHv5jw2gLZaW2Znzd_kf!}Ejwb3sSJ|v1l9>yfx*++Wj40CnuhjotSL`or{W~#AuWm1V0G>+=b zRU6v~MZ@pqrpJuQ(+j@lkkZgStqA*Jx9W}=cJKILa#V#@Y4mN{D{;U5_t24)J4h-M02Yo?dqH24Q>rf<_*~xnt@%9!p7w9 z!zI;56?mQX2H%6hvq$F*O?l0Dnu0XU23@?2ZTb7V^Y1KtnQ?>NuWa3=a8$4zSK!UO zoLnVSMQ{CQ#>2zI!^6YF!^6YF!^6YF!^6YF!^6YF!^6YF<3GkP9 Date: Tue, 17 Jan 2023 22:14:01 -0800 Subject: [PATCH 2/3] Updated extract_tarfile to include fileobj as input parameter as well --- samcli/lib/utils/tar.py | 26 +++++++++++++++++++---- samcli/local/docker/container.py | 2 +- tests/functional/lib/utils/test_tar.py | 11 +++++++--- tests/unit/lib/utils/test_tar.py | 22 ++++++++++++++++++- tests/unit/local/docker/test_container.py | 3 +-- 5 files changed, 53 insertions(+), 11 deletions(-) diff --git a/samcli/lib/utils/tar.py b/samcli/lib/utils/tar.py index 4963b3ebd0e..10107bff75f 100644 --- a/samcli/lib/utils/tar.py +++ b/samcli/lib/utils/tar.py @@ -4,7 +4,7 @@ import os import tarfile -from typing import Union +from typing import Union, IO, Optional from tempfile import TemporaryFile from contextlib import contextmanager @@ -53,9 +53,27 @@ def _is_within_directory(directory: Union[str, os.PathLike], target: Union[str, return bool(prefix == abs_directory) -def extract_tarfile(tarfile_path: Union[str, os.PathLike], unpack_dir: Union[str, os.PathLike]) -> None: - """Extracts a tarfile""" - with tarfile.open(tarfile_path, "r:*") as tar: +def extract_tarfile( + tarfile_path: Union[str, os.PathLike] = "", + file_obj: Optional[IO[bytes]] = None, + unpack_dir: Union[str, os.PathLike] = "", +) -> None: + """ + Extracts a tarfile using the provided parameters. If file_obj is specified, + it is used instead of the file_obj opened for tarfile_path. + + Parameters + ---------- + tarfile_path Union[str, os.PathLike] + Key representing a full path to the file or directory and the Value representing the path within the tarball + + file_obj Optional[IO[bytes]] + Object for the tarfile that will be extracted + + unpack_dir Union[str, os.PathLike] + The directory where the tarfile members will be extracted. + """ + with tarfile.open(name=tarfile_path, fileobj=file_obj, mode="r") as tar: # Makes sure the tar file is sanitized and is free of directory traversal vulnerability # See: https://github.com/advisories/GHSA-gw9q-c7gh-j9vm for member in tar.getmembers(): diff --git a/samcli/local/docker/container.py b/samcli/local/docker/container.py index b73e21e7236..4e3a899f561 100644 --- a/samcli/local/docker/container.py +++ b/samcli/local/docker/container.py @@ -379,7 +379,7 @@ def copy(self, from_container_path, to_host_path) -> None: # Seek the handle back to start of file for tarfile to use fp.seek(0) - extract_tarfile(tarfile_path=fp.name, unpack_dir=to_host_path) + extract_tarfile(file_obj=fp, unpack_dir=to_host_path) @staticmethod def _write_container_output(output_itr, stdout=None, stderr=None): diff --git a/tests/functional/lib/utils/test_tar.py b/tests/functional/lib/utils/test_tar.py index 59a4c5d481a..b4c6fb3f538 100644 --- a/tests/functional/lib/utils/test_tar.py +++ b/tests/functional/lib/utils/test_tar.py @@ -3,6 +3,7 @@ import shutil import platform from pathlib import Path +import tarfile from tarfile import ExtractError from unittest import TestCase @@ -14,10 +15,9 @@ class TestExtractTarFile(TestCase): def test_extract_tarfile_unpacks_a_tar(self): test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", "test.tgz") test_dir = tempfile.mkdtemp() - extract_tarfile(test_tar, test_dir) + extract_tarfile(tarfile_path=test_tar, unpack_dir=test_dir) output_files = set(os.listdir(test_dir)) shutil.rmtree(test_dir) - print(output_files) self.assertEqual({"test_utils.py"}, output_files) def test_raise_exception_for_unsafe_tarfile(self): @@ -25,5 +25,10 @@ def test_raise_exception_for_unsafe_tarfile(self): test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", tar_filename) test_dir = tempfile.mkdtemp() self.assertRaisesRegex( - ExtractError, "Attempted Path Traversal in Tar File", extract_tarfile, test_tar, test_dir + ExtractError, + "Attempted Path Traversal in Tar File", + extract_tarfile, + tarfile_path=test_tar, + unpack_dir=test_dir, ) + shutil.rmtree(test_dir) diff --git a/tests/unit/lib/utils/test_tar.py b/tests/unit/lib/utils/test_tar.py index ff339df39af..ec3e2a305f6 100644 --- a/tests/unit/lib/utils/test_tar.py +++ b/tests/unit/lib/utils/test_tar.py @@ -1,3 +1,4 @@ +import io from unittest import TestCase import tarfile from unittest.mock import Mock, patch, call @@ -92,7 +93,7 @@ def tar_filter(tar_info): @patch("samcli.lib.utils.tar.tarfile.open") @patch("samcli.lib.utils.tar._is_within_directory") - def test_extract_tarfile(self, is_within_directory_patch, tarfile_open_patch): + def test_extract_tarfile_file_name(self, is_within_directory_patch, tarfile_open_patch): tarfile_path = "/test_tarfile_path/" unpack_dir = "/test_unpack_dir/" is_within_directory_patch.return_value = True @@ -109,6 +110,25 @@ def test_extract_tarfile(self, is_within_directory_patch, tarfile_open_patch): tarfile_file_mock.getmembers.assert_called_once() tarfile_file_mock.extractall.assert_called_once_with(unpack_dir) + @patch("samcli.lib.utils.tar.tarfile.open") + @patch("samcli.lib.utils.tar._is_within_directory") + def test_extract_tarfile_fileobj(self, is_within_directory_patch, tarfile_open_patch): + stream_str = io.BytesIO(b"Hello World!") + unpack_dir = "/test_unpack_dir/" + is_within_directory_patch.return_value = True + + tarfile_file_mock = Mock() # Mock tarfile + tar_file_obj_mock = Mock() # Mock member inside tarfile + tar_file_obj_mock.name = "obj_name" + tarfile_file_mock.getmembers.return_value = [tar_file_obj_mock] + tarfile_open_patch.return_value.__enter__.return_value = tarfile_file_mock + + extract_tarfile(file_obj=stream_str, unpack_dir=unpack_dir) + + is_within_directory_patch.assert_called_once() + tarfile_file_mock.getmembers.assert_called_once() + tarfile_file_mock.extractall.assert_called_once_with(unpack_dir) + @patch("samcli.lib.utils.tar.tarfile.open") @patch("samcli.lib.utils.tar._is_within_directory") def test_extract_tarfile_obj_not_within_dir(self, is_within_directory_patch, tarfile_open_patch): diff --git a/tests/unit/local/docker/test_container.py b/tests/unit/local/docker/test_container.py index 9e005da106d..f0131931336 100644 --- a/tests/unit/local/docker/test_container.py +++ b/tests/unit/local/docker/test_container.py @@ -850,13 +850,12 @@ def test_must_copy_files_from_container(self, extract_tarfile_mock, tempfile_moc tempfile_ctxmgr = tempfile_mock.NamedTemporaryFile.return_value = Mock() fp_mock = Mock() - fp_mock.name = "/test_tarfile_path/" tempfile_ctxmgr.__enter__ = Mock(return_value=fp_mock) tempfile_ctxmgr.__exit__ = Mock() self.container.copy(source, dest) - extract_tarfile_mock.assert_called_with(tarfile_path=fp_mock.name, unpack_dir=dest) + extract_tarfile_mock.assert_called_with(file_obj=fp_mock, unpack_dir=dest) # Make sure archive data is written to the file fp_mock.write.assert_has_calls([call(x) for x in tar_stream], any_order=False) From 04f431d9535fe8a625fc9091edd34379ec44033a Mon Sep 17 00:00:00 2001 From: Haresh Nasit Date: Wed, 18 Jan 2023 12:54:31 -0800 Subject: [PATCH 3/3] Added functional tests for file_obj kw arg --- tests/functional/lib/utils/test_tar.py | 30 +++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/tests/functional/lib/utils/test_tar.py b/tests/functional/lib/utils/test_tar.py index b4c6fb3f538..8d3fa345f91 100644 --- a/tests/functional/lib/utils/test_tar.py +++ b/tests/functional/lib/utils/test_tar.py @@ -3,7 +3,6 @@ import shutil import platform from pathlib import Path -import tarfile from tarfile import ExtractError from unittest import TestCase @@ -12,7 +11,7 @@ class TestExtractTarFile(TestCase): - def test_extract_tarfile_unpacks_a_tar(self): + def test_extract_tarfile_arg_path_unpacks_a_tar(self): test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", "test.tgz") test_dir = tempfile.mkdtemp() extract_tarfile(tarfile_path=test_tar, unpack_dir=test_dir) @@ -20,7 +19,7 @@ def test_extract_tarfile_unpacks_a_tar(self): shutil.rmtree(test_dir) self.assertEqual({"test_utils.py"}, output_files) - def test_raise_exception_for_unsafe_tarfile(self): + def test_raise_exception_for_unsafe_tarfile_with_path_arg(self): tar_filename = "path_reversal_win.tgz" if platform.system().lower() == "windows" else "path_reversal_uxix.tgz" test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", tar_filename) test_dir = tempfile.mkdtemp() @@ -32,3 +31,28 @@ def test_raise_exception_for_unsafe_tarfile(self): unpack_dir=test_dir, ) shutil.rmtree(test_dir) + + def test_extract_tarfile_arg_fileobj_unpacks_a_tar(self): + test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", "test.tgz") + test_dir = tempfile.mkdtemp() + with open(test_tar, mode="rb") as tar: + before_extract_output_files = set(os.listdir(test_dir)) + self.assertEqual(set(), before_extract_output_files) + extract_tarfile(file_obj=tar, unpack_dir=test_dir) + after_extract_output_files = set(os.listdir(test_dir)) + self.assertEqual({"test_utils.py"}, after_extract_output_files) + shutil.rmtree(test_dir) + + def test_raise_exception_for_unsafe_tarfile_with_flieobj_arg(self): + tar_filename = "path_reversal_win.tgz" if platform.system().lower() == "windows" else "path_reversal_uxix.tgz" + test_tar = Path(__file__).resolve().parents[3].joinpath("functional", "testdata", "lib", "utils", tar_filename) + test_dir = tempfile.mkdtemp() + with open(test_tar, mode="rb") as tar: + self.assertRaisesRegex( + ExtractError, + "Attempted Path Traversal in Tar File", + extract_tarfile, + file_obj=tar, + unpack_dir=test_dir, + ) + shutil.rmtree(test_dir)