Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

Commit

Permalink
[VCDA-707] Fixed security hole in tarfile.extractall() related to dir…
Browse files Browse the repository at this point in the history
…ectory walking. (#268)

Issue : https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extractall
Fix is outlined at https://stackoverflow.com/questions/10060069/safely-extract-zip-or-tar-using-python/10077309#10077309

tarfile.extractall() method doesn't check for the presence of directory meta info hidden in file names of the archive members. As a result, malicious tar files can be used to extract payload into directories other than the target directory.

As a fix we filter out non safe members of archives before extracting them.

Testing Done:

Created an archive containing a file named "\..\1.txt". If this archive is extracted on windows, it will extract the text file in the root directory.
Wrote a a sample script to extract this tarfile with and without the filter we wrote.
Without filter : File gets extracted in the root folder.
With filter : We get expected error message.

````C:\Users\sena\Desktop\test>python extract.py attack_test.tar
\..\1.txt is blocked: illegal path.````

Also tried out the extraction script on a regular ova file and the extraction went on without any hiccups
  • Loading branch information
rocknes authored Jul 24, 2018
1 parent 3cbe892 commit bea8c39
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 1 deletion.
4 changes: 3 additions & 1 deletion pyvcloud/vcd/org.py
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from pyvcloud.vcd.exceptions import UploadException from pyvcloud.vcd.exceptions import UploadException
from pyvcloud.vcd.system import System from pyvcloud.vcd.system import System
from pyvcloud.vcd.utils import get_admin_href from pyvcloud.vcd.utils import get_admin_href
from pyvcloud.vcd.utils import get_safe_members_in_tar_file
from pyvcloud.vcd.utils import to_dict from pyvcloud.vcd.utils import to_dict


DEFAULT_CHUNK_SIZE = 1024 * 1024 DEFAULT_CHUNK_SIZE = 1024 * 1024
Expand Down Expand Up @@ -575,7 +576,8 @@ def upload_ovf(self,
try: try:
tempdir = tempfile.mkdtemp(dir='.') tempdir = tempfile.mkdtemp(dir='.')
ova = tarfile.open(file_name) ova = tarfile.open(file_name)
ova.extractall(path=tempdir) ova.extractall(path=tempdir,
members=get_safe_members_in_tar_file(ova))
ova.close() ova.close()
ovf_file = None ovf_file = None
extracted_files = os.listdir(tempdir) extracted_files = os.listdir(tempdir)
Expand Down
58 changes: 58 additions & 0 deletions pyvcloud/vcd/utils.py
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.


from os.path import abspath
from os.path import dirname
from os.path import join as joinpath
from os.path import realpath

import humanfriendly import humanfriendly
from lxml import etree from lxml import etree
from lxml.objectify import NoneElement from lxml.objectify import NoneElement
Expand Down Expand Up @@ -641,3 +646,56 @@ def get_admin_extension_href(href):
return href.replace('/api/admin/', '/api/admin/extension/') return href.replace('/api/admin/', '/api/admin/extension/')
else: else:
return href.replace('/api/', '/api/admin/extension/') return href.replace('/api/', '/api/admin/extension/')


def _badpath(path, base):
"""Determines if a given file path is under a given base path or not.
:param str path: file path where the file will be extracted to.
:param str base: path to the current working directory.
:return: False, if the path is under the given base, else True.
:rtype: bool
"""
# joinpath will ignore base if path is absolute
return not realpath(abspath(joinpath(base, path))).startswith(base)


def _badlink(info, base):
"""Determine if a given link is under a given base path or not.
:param TarInfo info: file that is going to be extracted.
:param str base: path to the current working directory.
:return: False, if the path is under the given base, else True.
:rtype: bool
"""
# Links are interpreted relative to the directory containing the link
tip = realpath(abspath(joinpath(base, dirname(info.name))))
return _badpath(info.linkname, base=tip)


def get_safe_members_in_tar_file(tarfile):
"""Retrieve members of a tar file that are safe to extract.
:param Tarfile tarfile: the archive that has been opened as a TarFile
object.
:return: list of members in the archive that are safe to extract.
:rtype: list
"""
base = realpath(abspath(('.')))
result = []
for finfo in tarfile.getmembers():
if _badpath(finfo.name, base):
print(finfo.name + ' is blocked: illegal path.')
elif finfo.issym() and _badlink(finfo, base):
print(finfo.name + ' is blocked: Symlink to ' + finfo.linkname)
elif finfo.islnk() and _badlink(finfo, base):
print(finfo.name + ' is blocked: Hard link to ' + finfo.linkname)
else:
result.append(finfo)
return result

0 comments on commit bea8c39

Please sign in to comment.