Eliminate Zip-Slip vulnerability #908
Conversation
|
Seems like Kokoro won't run when not merging into |
chanseokoh
force-pushed
the
i431-zip-slip
branch
from
5742f9f
to
5d3d2bc
Compare
| Path entryPath = destination.resolve(entry.getName()); | ||
|
|
||
| String canonicalTarget = entryPath.toFile().getCanonicalPath(); | ||
| if (!canonicalTarget.startsWith(canonicalDestination + File.separator)) { |
There was a problem hiding this comment.
Could this be done with NIO? Like maybe entryPath.toAbsolutePath().startsWith(destination.toAbsolutePath())?
There was a problem hiding this comment.
A canonical path is an absolute path, but the opposite is not true. For example,
System.out.println(Paths.get("/temp1/../temp/test.txt").toAbsolutePath());
prints /temp1/../temp/test.txt.
The code here is what https://snyk.io/research/zip-slip-vulnerability actually suggests, so I think it is safe to stick with the canonical path.
There was a problem hiding this comment.
Okay, though it feels off having to use the deprecated java.io.File API to account for a security vulnerability. Perhaps we could use Path#normalize?
There was a problem hiding this comment.
I still don't think Path.normalize() would work. A canonical path is a unique path for any given file; there can ever be only one canonical path. Computing a canonical path may involve "resolving symbolic links (on UNIX platforms), and converting drive letters to a standard case (on Microsoft Windows platforms)" according to its Javadoc. So I think it's safe to stick with the canonical path approach.
* Add unzip util method * Eliminate Zip-Slip vulnernability (#908)
On top of #906.
https://snyk.io/research/zip-slip-vulnerability