You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@heron.apache.org by GitBox <gi...@apache.org> on 2022/10/18 04:38:36 UTC
[GitHub] [incubator-heron] huijunwu opened a new pull request, #3857: validate path before untar
huijunwu opened a new pull request, #3857:
URL: https://github.com/apache/incubator-heron/pull/3857
This PR tries to validate untar path before actually untar the file.
reference https://security.snyk.io/research/zip-slip-vulnerability
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064271461
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths to protect against this: https://security.snyk.io/research/zip-slip-vulnerability
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
+ raise Exception(f"tar attempted to extract a file outside the destination dir: {untarpath_abs}:{name}")
Review Comment:
```suggestion
raise Exception("tar attempted to extract a file " \
f"outside the destination dir: {untarpath_abs}:{name}")
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064321501
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,17 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths to protect against this:
Review Comment:
```suggestion
# Validate file paths to protect against this:
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1001853040
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
Review Comment:
@nwangtw Is your question implying that this change might not be necessary?
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1011172117
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
Review Comment:
@nwangtw Following up on my prior question. Should we proceed with this PR? Are there any changes you would like to see?
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis merged pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis merged PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064254058
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
Review Comment:
```suggestion
# Validate file paths to protect against this: https://security.snyk.io/research/zip-slip-vulnerability
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nwangtw commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nwangtw commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1011383403
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
Review Comment:
@nicknezis It might be necessary. I wasn't sure about the exact cases that trigger the issue and it is not easy to reason the code. Maybe add a comment with the vulnerability link so that in the future it is easy for other people to understand the code.
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
+ raise Exception(f"tar attempted to extract a file outside the destination directory: {name}")
Review Comment:
maybe useful to log `untarpath_abs` as well.
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1001855155
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
+ raise Exception(f"tar extract outside directory file: {name}")
Review Comment:
```suggestion
raise Exception(f"tar attempted to extract a file outside the destination directory: {name}")
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nwangtw commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nwangtw commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r997728737
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
Review Comment:
hmm. In which case that `os.path.abspath(os.path.join(tmpdir, name))` result does not start with `tmpdir_abs`?
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064270249
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths to protect against this: https://security.snyk.io/research/zip-slip-vulnerability
Review Comment:
```suggestion
# Validate file paths to protect against this:
# https://security.snyk.io/research/zip-slip-vulnerability
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064254813
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
+ raise Exception(f"tar attempted to extract a file outside the destination directory: {name}")
Review Comment:
```suggestion
raise Exception(f"tar attempted to extract a file outside the destination dir: {untarpath_abs}:{name}")
```
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-heron] nicknezis commented on a diff in pull request #3857: validate path before untar
Posted by GitBox <gi...@apache.org>.
nicknezis commented on code in PR #3857:
URL: https://github.com/apache/incubator-heron/pull/3857#discussion_r1064254208
##########
heron/tools/cli/src/python/execute.py:
##########
@@ -101,8 +101,15 @@ def heron_tar(class_name, topology_tar, arguments, tmpdir_root, java_defines):
'''
# Extract tar to a tmp folder.
tmpdir = tempfile.mkdtemp(dir=tmpdir_root, prefix='tmp')
+ tmpdir_abs = os.path.abspath(tmpdir)
with contextlib.closing(tarfile.open(topology_tar)) as tar:
+ # Validate file paths
+ for name in tar.getnames():
+ untarpath_abs = os.path.abspath(os.path.join(tmpdir, name))
+ if not untarpath_abs.startswith(tmpdir_abs):
Review Comment:
I updated the comment to include a the vulnerability link. I'll merge this pull request.
--
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@heron.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org