Marcono1234 commented on code in PR #776:
URL: https://github.com/apache/commons-compress/pull/776#discussion_r3979140604
##########
src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java:
##########
@@ -217,11 +217,27 @@ final void setBeforeLeafWrite(final Runnable hook) {
}
/**
- * Resolves {@code name} against the extraction root and rejects any
result that escapes it (the lexical zip-slip guard).
+ * Tests whether {@code path} is contained within the canonical extraction
root, comparing component by component so a
+ * sibling that merely shares a name prefix (for example {@code root-old}
beside {@code root}) is not treated as contained.
+ */
+ private boolean isWithinRoot(final Path path) {
+ return path.startsWith(rootDirectory);
+ }
+
+ /**
+ * Resolves {@code name} against the extraction root and applies the
lexical zip-slip guard.
+ *
+ * @return the resolved path within the root, or {@code null} if {@code
name} resolves to the root itself (for example
+ * {@code a/..}), which carries nothing to materialize; the caller
skips such entries rather than writing at or
+ * replacing the root.
+ * @throws ArchiveException if the resolved path escapes the extraction
root.
*/
private Path resolveWithinRoot(final String name) throws ArchiveException {
final Path resolved = rootDirectory.resolve(name).normalize();
- if (!resolved.startsWith(rootDirectory)) {
+ if (resolved.equals(rootDirectory)) {
Review Comment:
It seems my suggestion to use `isSameFile` is flawed / risky because it
accesses the file system. So it should only occur as additional check, after a
`Path#startsWith` check passed (which does not access the file system).
Otherwise I am worried that merely by calling `isSameFile` a Windows UNC
path could leak information to a remote server.
Though as you mention, it might be desirable to avoid `isSameFile`
completely (and I guess that is fine?).
However, either way, maybe it would be good to add a Windows-only test where
a ZIP entry has a Windows UNC path as name (e.g. `\\localhost\invalid`) and
ensure that the reported error is from the ZIP Slip protection and not an error
like `FileSystemException: \\localhost\invalid\: The network name cannot be
found`, which would indicate that some API was used which actually tried to
perform a file system access _before_ they ZIP Slip check.
What do you think?
(though it seems the PR here is unfortunately a bit stale, maybe due to lack
of time by the maintainers)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
