Fix symlinks #42
Fix symlinks #42
Conversation
|
Is this project still maintained? Most PRs seem unattended, except for simple bound bumping. |
| check name | ||
| | FilePath.Native.isAbsolute name | ||
| | checkCommon name |
There was a problem hiding this comment.
Previously when not (FilePath.Native.isValid name) we returned InvalidFileName, but now it is AbsoluteFileName. Why?
| @@ -65,24 +65,46 @@ checkSecurity = checkEntries checkEntrySecurity | |||
| checkEntrySecurity :: Entry -> Maybe FileNameError | |||
There was a problem hiding this comment.
If filepaths now allow .., a comment for checkSecurity needs to be updated.
|
|
||
| where getDepth [] = 0 | ||
| getDepth ("..":fps) = 1 + getDepth fps | ||
| getDepth (_:fps) = getDepth fps - 1 |
There was a problem hiding this comment.
I feel uneasy merging such changes without any regression tests. Could we please add some?
There was a problem hiding this comment.
What happens if one of the elements is "."? It should not affect the depth, right? Imagine a symbolic link of form ./.././.. or similar.
@hasufell is my understanding correct here?
There was a problem hiding this comment.
Yeah, I think we need to normalise first. I followed the codepath and I don't see that normalisation happens anywhere else. The link target is potentially read directly from the tar archive data and can contain anything.
| = Just $ InvalidFileName name | ||
|
|
||
| | otherwise = Nothing | ||
|
|
||
| linkDepth :: FilePath -- ^ Name of link |
There was a problem hiding this comment.
It's somewhat confusing what the arguments exactly are.
First argument is:
- the path of the symlink file relative to the tar root
Second argument is:
- the link target, which must always be a relative path
| in getDepth allPaths | ||
|
|
||
| where getDepth [] = 0 | ||
| getDepth ("..":fps) = 1 + getDepth fps |
There was a problem hiding this comment.
There's a bug here I believe. link itself (which is the filepath of the symlink) can already contain ... Although this is uncommon and most tools avoid doing this, our implementation does not. Note that the existence of a .. doesn't necessarily mean it's a tarbomb.
Try something like:
htar -c 'doc/../..' > /tmp/foo.tar
Afais getDepth will report a problem here and cause failure, although the symlink target itself might not be to blame. This is likely a tarbomb, but for that we have https://hackage.haskell.org/package/tar-0.5.1.1/docs/Codec-Archive-Tar-Check.html#v:checkTarbomb
|
@vmchale would you be interested to proceed with this PR? It's understandable, if you lost interest after two years. |
|
Superseded by #82. |
This fixes #32
We no longer fail when a symlink points to
../../Makefile.gen(for instance), provided that it does not go outside of the unpacking directory.(This is dependent on #41)