tomasilluminati commented on code in PR #776:
URL: https://github.com/apache/commons-compress/pull/776#discussion_r4059039834
##########
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:
@Marcono1234 Agreed. The guard never used isSameFile and stays lexical:
resolve, normalize, equals and startsWith, with nothing touching the file
system before containment. The Javadoc now says so.
Tests added: a host/share name like //localhost/invalid/evil.txt on every
platform through ZipFile, the zip stream and tar, plus Windows-only cases for
\\localhost\invalid, an absolute drive path and a drive-relative name. Each
accepts only the guard's own ArchiveException and an empty target, so a
FileSystemException fails it. With isSameFile put in front of the check, the
cross-platform cases fail with NoSuchFileException. The malformed-name test now
covers the Windows illegal characters and a share-less UNC name.
They pass with a Windows JDK 17 under Wine. The windows-latest leg has not
run since the first push, so a maintainer approving the workflow would help.
The trailing-dots alias stays documented and fail-closed; the JDK issue is
JDK-8248430.
--
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]
