fix: #65#issuecomment-395988244 - zip slip by symlink #70
Conversation
aviadatsnyk
force-pushed
the
fix/zip-slip-symlink
branch
from
b5f0a73 to
09aa582
Compare
| @@ -8,6 +8,8 @@ import ( | |||
| "path/filepath" | |||
| "runtime" | |||
| "strings" | |||
|
|
|||
| "github.com/cyphar/filepath-securejoin" | |||
There was a problem hiding this comment.
I'm not a huge fan of introducing a whole dependency just for one function... is the logic something we can extract from it pretty easily?
There was a problem hiding this comment.
shouldn't be too hard, I guess. It's 135 loc including comments and whitespaces - https://github.com/cyphar/filepath-securejoin/blob/master/join.go
I would, however, consider using the lib since it's not as simple a task as it seems. (it's the only thing it's doing, btw, so we get almost no unnecessary code).
|
@aviadatsnyk If you can resolve the conflict I'll merge this in. Thanks! |
|
Hmm. The issue that linked above makes some good points about how SecureJoin still isn't a secure solution, if an attacker wins a race against you. Maybe we should just change the scope of this lib's threat model to assume that you already trust the archive file. Perhaps a better approach would be to try to detect a likely attack before extracting an archive? (At the moment, I'm being held up because of the "zip slip" fix even though it doesn't actually solve the problem, and is hindering me from working with a trustworthy archive that is not attacking my system.) |
proposed fix for #65 (comment)