[VCDA-707] Fixed security hole in tarfile.extractall() related to directory walking.

[rocknes]

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:

1. 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.
2. 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

---

This change is 

[sahithi]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @rocknes, @sahithi, @pacogomez, and @hodgesrm)

---

pyvcloud/vcd/org.py, line 580 at r1 (raw file):

            ova = tarfile.open(file_name)
            ova.extractall(path=tempdir,
                           members=get_safe_members_in_tar_file(ova))

So, with this change, will malicious-ova still get extracted successfully and be used by vCD (barring files which do not comply path-rules you set below)? Shouldn't we simply throw an exception and abort the operation here? Talk to Yassine (or) please add him to the review.

---

pyvcloud/vcd/utils.py, line 696 at r1 (raw file):

Quoted 6 lines of code…

 if _badpath(finfo.name, base):
            print(finfo.name + ' is blocked: illegal path.')
        elif finfo.issym() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Hard link to ' + finfo.linkname)
        elif finfo.islnk() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Symlink to ' + finfo.linkname)

Use Logger statements instead of Print

[sahithi]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @rocknes, @sahithi, @pacogomez, and @hodgesrm)

---

pyvcloud/vcd/org.py, line 580 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

So, with this change, will malicious-ova still get extracted successfully and be used by vCD (barring files which do not comply path-rules you set below)? Shouldn't we simply throw an exception and abort the operation here? Talk to Yassine (or) please add him to the review.

As I will be on PTO next few weeks, please get it reviewed by Yassine.

[sahithi]

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rocknes, @pacogomez, and @hodgesrm)

[rocknes]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rocknes, @ckolovson, @pacogomez, and @hodgesrm)

---

pyvcloud/vcd/org.py, line 580 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

As I will be on PTO next few weeks, please get it reviewed by Yassine.

Identifying malicious file is not an easy task, and ideally users should scan untrustworthy ova files using a 3rd party anti-virus scanner. In this PR we are shutting down the directory walking loopwhole and that's it.

I don't think we should throw an exception here, because the method is a pure filter method. In our case the caller of this method is the one which is extracting the archive. This method shouldn't be worrying about what it's caller is doing, and just stick to filtering safe members of an archive.

---

pyvcloud/vcd/utils.py, line 696 at r1 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

 if _badpath(finfo.name, base):
            print(finfo.name + ' is blocked: illegal path.')
        elif finfo.issym() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Hard link to ' + finfo.linkname)
        elif finfo.islnk() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Symlink to ' + finfo.linkname)

Use Logger statements instead of Print

Unfortunately logger class is not exposed beyond client.py. I had to add support for logging in system tests separately. It's not impossible to expose the logger class from client.py.

I will like to propose we do it in a different pull request and update these statements in that pull request.

[ckolovson]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sahithi, @rocknes, @pacogomez, and @hodgesrm)

---

pyvcloud/vcd/utils.py, line 696 at r1 (raw file):

            print(finfo.name + ' is blocked: illegal path.')
        elif finfo.issym() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Hard link to ' + finfo.linkname)

Error message should say: "Symlink to..."

---

pyvcloud/vcd/utils.py, line 698 at r1 (raw file):

            print(finfo.name + ' is blocked: Hard link to ' + finfo.linkname)
        elif finfo.islnk() and _badlink(finfo, base):
            print(finfo.name + ' is blocked: Symlink to ' + finfo.linkname)

Error message should say "Hard link to..."

[ckolovson]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @sahithi, @rocknes, @pacogomez, and @hodgesrm)

[rocknes]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @sahithi, @pacogomez, and @hodgesrm)

---

pyvcloud/vcd/utils.py, line 696 at r1 (raw file):

Previously, ckolovson (Curt Kolovson) wrote…

Error message should say: "Symlink to..."

Thanks, will fix it.

---

pyvcloud/vcd/utils.py, line 698 at r1 (raw file):

Previously, ckolovson (Curt Kolovson) wrote…

Error message should say "Hard link to..."

Thanks, will fix it.