You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2022/11/27 05:23:04 UTC
[GitHub] [buildstream-plugins] gtristan opened a new pull request, #37: CVE-2007-4559 Patch
gtristan opened a new pull request, #37:
URL: https://github.com/apache/buildstream-plugins/pull/37
## Patching CVE-2007-4559
Hi, we are security researchers from the Advanced Research Center at [Trellix](https://www.trellix.com). We have began a campaign to patch a widespread bug named CVE-2007-4559. CVE-2007-4559 is a 15 year old bug in the Python tarfile package. By using extract() or extractall() on a tarfile object without sanitizing input, a maliciously crafted .tar file could perform a directory path traversal attack. We found at least one unsantized extractall() in your codebase and are providing a patch for you via pull request. The patch essentially checks to see if all tarfile members will be extracted safely and throws an exception otherwise. We encourage you to use this patch or your own solution to secure against CVE-2007-4559. Further technical information about the vulnerability can be found in this [blog](https://www.trellix.com/en-us/about/newsroom/stories/research/tarfile-exploiting-the-world.html).
If you have further questions you may contact us through this projects lead researcher [Kasimir Schulz](mailto:kasimir.schulz@trellix.com).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #37: CVE-2007-4559 Patch
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #37:
URL: https://github.com/apache/buildstream-plugins/pull/37#discussion_r1288995651
##########
src/buildstream_plugins/sources/cargo.py:
##########
@@ -148,7 +148,25 @@ def stage(self, directory):
try:
mirror_file = self._get_mirror_file()
with tarfile.open(mirror_file) as tar:
- tar.extractall(path=directory)
+
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+
+ return prefix == abs_directory
+
+ def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
+
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
+ raise Exception("Attempted Path Traversal in Tar File")
+
+ tar.extractall(path, members, numeric_owner=numeric_owner)
Review Comment:
The fundamental problem here IMHO is we are handling sources in host without any kind of sandboxing or isolation. This is just duck-taping around that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] BenjaminSchubert commented on a diff in pull request #37: CVE-2007-4559 Patch
Posted by GitBox <gi...@apache.org>.
BenjaminSchubert commented on code in PR #37:
URL: https://github.com/apache/buildstream-plugins/pull/37#discussion_r1032921498
##########
src/buildstream_plugins/sources/cargo.py:
##########
@@ -148,7 +148,25 @@ def stage(self, directory):
try:
mirror_file = self._get_mirror_file()
with tarfile.open(mirror_file) as tar:
- tar.extractall(path=directory)
+
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+
+ return prefix == abs_directory
+
+ def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
+
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
Review Comment:
I believe we could simplify the logic with `pathlib` here:
```suggestion
def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
path = Path(path).resolve()
for member in tar.getmembers():
member_path = path.joinpath(member).resolve()
if path not in member_path.parents:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on pull request #37: CVE-2007-4559 Patch
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #37:
URL: https://github.com/apache/buildstream-plugins/pull/37#issuecomment-1328174856
NOTE: This PR was originally proposed in a personal github repo here: https://github.com/gtristan/buildstream-plugins/pull/4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] BenjaminSchubert commented on a diff in pull request #37: CVE-2007-4559 Patch
Posted by GitBox <gi...@apache.org>.
BenjaminSchubert commented on code in PR #37:
URL: https://github.com/apache/buildstream-plugins/pull/37#discussion_r1032921635
##########
src/buildstream_plugins/sources/cargo.py:
##########
@@ -148,7 +148,25 @@ def stage(self, directory):
try:
mirror_file = self._get_mirror_file()
with tarfile.open(mirror_file) as tar:
- tar.extractall(path=directory)
+
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+
+ return prefix == abs_directory
+
+ def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
+
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
+ raise Exception("Attempted Path Traversal in Tar File")
Review Comment:
I think this should be a `SourceError` if I remember our contracts with sources? Additionally, mentioning which file is violating what would be nice, I don't think a user would be able to debug easily otherwise
##########
src/buildstream_plugins/sources/cargo.py:
##########
@@ -148,7 +148,25 @@ def stage(self, directory):
try:
mirror_file = self._get_mirror_file()
with tarfile.open(mirror_file) as tar:
- tar.extractall(path=directory)
+
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+
+ return prefix == abs_directory
+
+ def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
+
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
+ raise Exception("Attempted Path Traversal in Tar File")
+
+ tar.extractall(path, members, numeric_owner=numeric_owner)
Review Comment:
My understanding of the `tar` is that it contains no global header, and thus doing a `tar.getmembers()` then a `tar.extracall()` actually ends up reading the file twice, which is wasteful. If we want to sanitize I would rather extract each entry one by one after we've sanitized it's destination
##########
src/buildstream_plugins/sources/cargo.py:
##########
@@ -148,7 +148,25 @@ def stage(self, directory):
try:
mirror_file = self._get_mirror_file()
with tarfile.open(mirror_file) as tar:
- tar.extractall(path=directory)
+
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+
+ return prefix == abs_directory
+
+ def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
+
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
Review Comment:
I believe we could simplify the logic with `pathlib` here:
```suggestion
def safe_extract(tar, path=".", members=None, *, numeric_owner=False):
path = Path(path).resolve()
for member in tar.getmembers():
member_path = path.joinpath(member.name).resolve()
if path not in member_path.parents:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org