From a796b86e90359210b856b428877e5a0a63774e69 Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Sun, 28 Jun 2026 15:30:09 -0300 Subject: [PATCH 1/6] [COMPRESS-727] Provide a symlink-resistant and concurrency-safe archive Extractor Adds a security-first Extractor under archivers. extractor with a SecureDirectoryStream-based race-safe path on Linux and a best-effort base extractor elsewhere. Symlink and special-file policies, safe hard-link resolution, tests and a changes.xml entry are included. --- src/changes/changes.xml | 3 +- .../archivers/extractor/Extractor.java | 388 ++++++++++++++++++ .../archivers/extractor/SecureExtractor.java | 101 +++++ .../extractor/SpecialFilePolicy.java | 38 ++ .../archivers/extractor/SymlinkPolicy.java | 50 +++ .../archivers/extractor/package-info.java | 31 ++ .../extractor/ExtractorBasePathTest.java | 117 ++++++ .../extractor/ExtractorFactoryTest.java | 76 ++++ .../extractor/ExtractorFuzzTest.java | 135 ++++++ .../extractor/ExtractorHappyPathTest.java | 76 ++++ .../extractor/ExtractorHardLinkTest.java | 62 +++ .../extractor/ExtractorMalformedNameTest.java | 77 ++++ .../extractor/ExtractorOverwriteTest.java | 82 ++++ .../extractor/ExtractorPentestTest.java | 141 +++++++ .../extractor/ExtractorResilienceTest.java | 229 +++++++++++ .../extractor/ExtractorRootBoundaryTest.java | 72 ++++ .../extractor/ExtractorSecurePathTest.java | 88 ++++ .../extractor/ExtractorSpecialFileTest.java | 77 ++++ .../extractor/ExtractorSymlinkPolicyTest.java | 119 ++++++ .../extractor/ExtractorSymlinkSlipTest.java | 95 +++++ .../extractor/ExtractorZipSlipTest.java | 64 +++ .../archivers/extractor/Fixtures.java | 208 ++++++++++ .../extractor/SecureExtractorRaceTest.java | 87 ++++ 23 files changed, 2415 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java create mode 100644 src/main/java/org/apache/commons/compress/archivers/extractor/SecureExtractor.java create mode 100644 src/main/java/org/apache/commons/compress/archivers/extractor/SpecialFilePolicy.java create mode 100644 src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java create mode 100644 src/main/java/org/apache/commons/compress/archivers/extractor/package-info.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorBasePathTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFactoryTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFuzzTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHappyPathTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHardLinkTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorOverwriteTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorPentestTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSecurePathTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSpecialFileTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkSlipTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/Fixtures.java create mode 100644 src/test/java/org/apache/commons/compress/archivers/extractor/SecureExtractorRaceTest.java diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a47a677e004..32c90249a5b 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -43,7 +43,8 @@ The type attribute can be add,update,fix,remove. - + Add a security-first Extractor (org.apache.commons.compress.archivers.extractor) that safely extracts archives into a directory, including symbolic links, resisting symlink-slip and, on platforms with java.nio.file.SecureDirectoryStream, concurrent symlink races. + Performance issue in SevenZFile #681. Fix kilobyte to kibibyte conversion in SevenZFile.ArchiveStatistics.assertValidity(int). Fix kilobyte to kibibyte conversion in SevenZFile.ArchiveStatistics.toString(). diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java new file mode 100644 index 00000000000..2c88863599e --- /dev/null +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java @@ -0,0 +1,388 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.channels.Channels; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.LinkOption; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.SecureDirectoryStream; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.Enumeration; +import java.util.Objects; + +import org.apache.commons.compress.archivers.ArchiveEntry; +import org.apache.commons.compress.archivers.ArchiveException; +import org.apache.commons.compress.archivers.ArchiveInputStream; +import org.apache.commons.compress.archivers.tar.TarArchiveEntry; +import org.apache.commons.compress.archivers.tar.TarFile; +import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; +import org.apache.commons.compress.archivers.zip.ZipFile; +import org.apache.commons.io.IOUtils; + +/** + * Safely materializes an archive into a target directory, including symbolic links, while resisting both symlink-slip from + * the archive itself and concurrent symlink races from third parties. + *

+ * Obtain an instance with {@link #newExtractor(Path)}, which returns the strongest implementation the platform supports: a + * race-safe extractor backed by {@link SecureDirectoryStream} where the file system provides one (Linux), otherwise this + * best-effort base implementation. The base implementation resolves every path component with + * {@link LinkOption#NOFOLLOW_LINKS} and writes regular files {@code CREATE_NEW}, which stops symlink-slip from the archive and + * silent overwrites, but it cannot fully close a concurrent time-of-check-to-time-of-use race; that residual is documented + * and only the secure implementation removes it. + *

+ *

+ * Defaults are conservative: symbolic-link entries are rejected ({@link SymlinkPolicy#REJECT}), special entries are skipped + * ({@link SpecialFilePolicy#SKIP}), and existing files are not overwritten. Instances are not thread-safe: configure an + * instance before extracting and do not share it across concurrent extractions. + *

+ * + * @since 1.29.0 + */ +public class Extractor { + + enum EntryType { + FILE, DIRECTORY, SYMLINK, HARD_LINK, SPECIAL + } + + /** + * Creates an extractor for {@code targetDirectory}, returning a race-safe implementation when the platform's file system + * provider exposes a {@link SecureDirectoryStream} (Linux), otherwise a best-effort implementation. + * + * @param targetDirectory an existing directory into which archives are extracted; resolved to its real path. + * @return an extractor bound to {@code targetDirectory}. + * @throws IOException if {@code targetDirectory} does not exist or is not a directory. + * @since 1.29.0 + */ + public static Extractor newExtractor(final Path targetDirectory) throws IOException { + final Path root = Objects.requireNonNull(targetDirectory, "targetDirectory").toRealPath(); + if (!Files.isDirectory(root)) { + throw new IOException("Target is not a directory: " + targetDirectory); + } + try (DirectoryStream stream = Files.newDirectoryStream(root)) { + if (stream instanceof SecureDirectoryStream) { + return new SecureExtractor(root); + } + } + return new Extractor(root); + } + + /** The validated, canonical extraction root. */ + final Path rootDirectory; + + private SymlinkPolicy symlinkPolicy = SymlinkPolicy.REJECT; + + private SpecialFilePolicy specialFilePolicy = SpecialFilePolicy.SKIP; + + private boolean overwrite; + + private Runnable beforeLeafWrite; + + Extractor(final Path rootDirectory) { + this.rootDirectory = rootDirectory; + } + + private static EntryType classify(final ArchiveEntry entry) { + if (entry instanceof TarArchiveEntry) { + final TarArchiveEntry tar = (TarArchiveEntry) entry; + if (tar.isSymbolicLink()) { + return EntryType.SYMLINK; + } + if (tar.isLink()) { + return EntryType.HARD_LINK; + } + if (tar.isBlockDevice() || tar.isCharacterDevice() || tar.isFIFO()) { + return EntryType.SPECIAL; + } + return tar.isDirectory() ? EntryType.DIRECTORY : EntryType.FILE; + } + if (entry instanceof ZipArchiveEntry && ((ZipArchiveEntry) entry).isUnixSymlink()) { + return EntryType.SYMLINK; + } + return entry.isDirectory() ? EntryType.DIRECTORY : EntryType.FILE; + } + + /** + * Extracts the entries of an {@link ArchiveInputStream}. + *

+ * Zip unix symbolic links are recognized only through {@link #extract(ZipFile)}: their unix mode lives in the central + * directory, which a stream does not expose, so a zip symbolic link presented through a stream is materialized as a + * regular file (no-follow, create-new) rather than a link. + *

+ * + * @param archive the archive to extract; not closed by this method. + * @throws IOException if extraction fails or a policy rejects an entry. + * @since 1.29.0 + */ + public void extract(final ArchiveInputStream archive) throws IOException { + ArchiveEntry entry; + while ((entry = archive.getNextEntry()) != null) { + if (!archive.canReadEntryData(entry)) { + continue; + } + final EntryType type = classify(entry); + String linkTarget = null; + if (type == EntryType.SYMLINK || type == EntryType.HARD_LINK) { + // Only tar carries link metadata through a stream: a zip unix symbolic link is recognized solely from the + // central directory (see extract(ZipFile)), which a stream does not expose, so classify never reports a + // streamed entry as a link unless it is a TarArchiveEntry. + linkTarget = ((TarArchiveEntry) entry).getLinkName(); + } + process(entry.getName(), type, linkTarget, type == EntryType.FILE ? archive : null); + } + } + + /** + * Extracts the entries of a {@link TarFile}. + * + * @param archive the archive to extract; not closed by this method. + * @throws IOException if extraction fails or a policy rejects an entry. + * @since 1.29.0 + */ + public void extract(final TarFile archive) throws IOException { + for (final TarArchiveEntry entry : archive.getEntries()) { + final EntryType type = classify(entry); + if (type == EntryType.FILE) { + try (InputStream content = archive.getInputStream(entry)) { + process(entry.getName(), type, null, content); + } + } else { + final String linkTarget = type == EntryType.SYMLINK || type == EntryType.HARD_LINK ? entry.getLinkName() : null; + process(entry.getName(), type, linkTarget, null); + } + } + } + + /** + * Extracts the entries of a {@link ZipFile}. + * + * @param archive the archive to extract; not closed by this method. + * @throws IOException if extraction fails or a policy rejects an entry. + * @since 1.29.0 + */ + public void extract(final ZipFile archive) throws IOException { + final Enumeration entries = archive.getEntries(); + while (entries.hasMoreElements()) { + final ZipArchiveEntry entry = entries.nextElement(); + if (!archive.canReadEntryData(entry)) { + continue; + } + final EntryType type = classify(entry); + if (type == EntryType.FILE) { + try (InputStream content = archive.getInputStream(entry)) { + process(entry.getName(), type, null, content); + } + } else { + final String linkTarget = type == EntryType.SYMLINK ? archive.getUnixSymlink(entry) : null; + process(entry.getName(), type, linkTarget, null); + } + } + } + + boolean isOverwrite() { + return overwrite; + } + + final void runBeforeLeafWrite() { + if (beforeLeafWrite != null) { + beforeLeafWrite.run(); + } + } + + final void setBeforeLeafWrite(final Runnable hook) { + this.beforeLeafWrite = hook; + } + + /** + * Resolves {@code name} against the extraction root and rejects any result that escapes it (the lexical zip-slip guard). + */ + private Path resolveWithinRoot(final String name) throws ArchiveException { + final Path resolved = rootDirectory.resolve(name).normalize(); + if (!resolved.startsWith(rootDirectory)) { + throw new ArchiveException("Entry '%s' would escape the extraction root", name); + } + return resolved; + } + + /** + * Walks the path from the root to {@code directory}, creating missing components and refusing to traverse any existing + * component that is a symbolic link or a non-directory. This is the best-effort, non-atomic parent resolution; the secure + * implementation overrides the leaf operations to be descriptor-relative. + */ + void ensureDirectory(final Path directory) throws IOException { + if (directory == null || directory.equals(rootDirectory)) { + return; + } + Path current = rootDirectory; + for (final Path component : rootDirectory.relativize(directory)) { + current = current.resolve(component); + final BasicFileAttributes attrs; + try { + attrs = Files.readAttributes(current, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS); + } catch (final NoSuchFileException e) { + Files.createDirectory(current); + continue; + } + if (attrs.isSymbolicLink()) { + throw new ArchiveException("Refusing to traverse symbolic link: %s", current); + } + if (!attrs.isDirectory()) { + throw new ArchiveException("Parent path component is not a directory: %s", current); + } + } + } + + private void process(final String name, final EntryType type, final String linkTarget, final InputStream content) throws IOException { + try { + processEntry(name, type, linkTarget, content); + } catch (final InvalidPathException e) { + // A name or link target that cannot become a Path on this platform (for example one containing a NUL byte) is + // rejected as a malformed entry rather than propagated as an unchecked exception that would abort extraction. + throw new ArchiveException("Invalid entry name: " + name, e); + } + } + + private void processEntry(final String name, final EntryType type, final String linkTarget, final InputStream content) throws IOException { + if (type == EntryType.SPECIAL) { + if (specialFilePolicy == SpecialFilePolicy.REJECT) { + throw new ArchiveException("Special entry not allowed: %s", name); + } + return; + } + if (type == EntryType.SYMLINK) { + if (symlinkPolicy == SymlinkPolicy.REJECT) { + throw new ArchiveException("Symbolic link not allowed: %s", name); + } + if (symlinkPolicy == SymlinkPolicy.SKIP) { + return; + } + } + final Path leaf = resolveWithinRoot(name); + ensureDirectory(leaf.getParent()); + switch (type) { + case DIRECTORY: + ensureDirectory(leaf); + break; + case FILE: + writeFile(leaf, content); + break; + case SYMLINK: + writeSymbolicLink(leaf, linkTarget); + break; + case HARD_LINK: + writeHardLink(leaf, linkTarget); + break; + default: + throw new ArchiveException("Unsupported entry type for '%s'", name); + } + } + + /** + * Sets whether existing files may be overwritten. When {@code false} (the default) an entry whose target already exists + * fails the extraction; the write never follows an existing symbolic link at the target path. + * + * @param overwrite whether to overwrite existing files. + * @return {@code this}. + * @since 1.29.0 + */ + public Extractor setOverwrite(final boolean overwrite) { + this.overwrite = overwrite; + return this; + } + + /** + * Sets the policy for special entries (devices, FIFOs). Defaults to {@link SpecialFilePolicy#SKIP}. + * + * @param specialFilePolicy the policy to apply; not null. + * @return {@code this}. + * @since 1.29.0 + */ + public Extractor setSpecialFilePolicy(final SpecialFilePolicy specialFilePolicy) { + this.specialFilePolicy = Objects.requireNonNull(specialFilePolicy, "specialFilePolicy"); + return this; + } + + /** + * Sets the policy for symbolic-link entries. Defaults to {@link SymlinkPolicy#REJECT}. + * + * @param symlinkPolicy the policy to apply; not null. + * @return {@code this}. + * @since 1.29.0 + */ + public Extractor setSymlinkPolicy(final SymlinkPolicy symlinkPolicy) { + this.symlinkPolicy = Objects.requireNonNull(symlinkPolicy, "symlinkPolicy"); + return this; + } + + /** + * Writes a regular file at {@code leaf} with no-follow semantics, refusing to follow an existing symbolic link at that + * path. With overwrite disabled the open is {@code CREATE_NEW}, so an existing entry fails the extraction. + */ + void writeFile(final Path leaf, final InputStream content) throws IOException { + runBeforeLeafWrite(); + final OutputStream out = overwrite + ? Channels.newOutputStream(Files.newByteChannel(leaf, StandardOpenOption.CREATE, StandardOpenOption.WRITE, + StandardOpenOption.TRUNCATE_EXISTING, LinkOption.NOFOLLOW_LINKS)) + : Channels.newOutputStream(Files.newByteChannel(leaf, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE, + LinkOption.NOFOLLOW_LINKS)); + try (OutputStream sink = out) { + IOUtils.copy(content, sink); + } + } + + /** + * Creates a hard link at {@code leaf} to {@code linkTarget}, which is resolved against the extraction root and rejected if + * it escapes. + */ + void writeHardLink(final Path leaf, final String linkTarget) throws IOException { + final Path resolved = rootDirectory.resolve(linkTarget).normalize(); + if (!resolved.startsWith(rootDirectory)) { + throw new ArchiveException("Hard link target escapes the extraction root: %s -> %s", leaf, linkTarget); + } + // The lexical check is not enough: a target routed through a symbolic link (planted or from an earlier entry) would + // be followed by createLink and escape the root. Re-resolve on disk and re-check containment, then link the real file. + final Path real = resolved.toRealPath(); + if (!real.startsWith(rootDirectory)) { + throw new ArchiveException("Hard link target resolves outside the extraction root: %s -> %s", leaf, linkTarget); + } + Files.createLink(leaf, real); + } + + /** + * Creates a symbolic link at {@code leaf} pointing at {@code linkTarget}. Under {@link SymlinkPolicy#ALLOW_WITHIN_ROOT} a + * target that resolves outside the root is rejected; under {@link SymlinkPolicy#ALLOW_ALL} the link is created verbatim. + */ + void writeSymbolicLink(final Path leaf, final String linkTarget) throws IOException { + if (symlinkPolicy == SymlinkPolicy.ALLOW_WITHIN_ROOT) { + final Path parent = leaf.getParent(); + final Path resolved = (parent != null ? parent : rootDirectory).resolve(linkTarget).normalize(); + if (!resolved.startsWith(rootDirectory)) { + throw new ArchiveException("Symbolic link target escapes the extraction root: %s -> %s", leaf, linkTarget); + } + } + Files.createSymbolicLink(leaf, leaf.getFileSystem().getPath(linkTarget)); + } +} diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/SecureExtractor.java b/src/main/java/org/apache/commons/compress/archivers/extractor/SecureExtractor.java new file mode 100644 index 00000000000..4d8e0ae82b3 --- /dev/null +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/SecureExtractor.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.SecureDirectoryStream; +import java.nio.file.StandardOpenOption; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.HashSet; +import java.util.Set; + +import org.apache.commons.io.IOUtils; + +/** + * Race-safe {@link Extractor} for platforms whose file system provider exposes a {@link SecureDirectoryStream}. The parent of + * each regular file is re-walked component by component from the root directory handle at write time, every component opened + * with {@link LinkOption#NOFOLLOW_LINKS}, and the file is created relative to the innermost directory handle (file + * descriptor). A third party therefore cannot win a time-of-check-to-time-of-use race by swapping a parent component for a + * symbolic link between resolution and write: the write targets the pinned directory inode, not a re-resolved path. + *

+ * Directory, symbolic-link, and hard-link creation remain path-based (the {@code java.nio.file} API exposes no + * descriptor-relative {@code mkdirat}/{@code symlinkat}/{@code linkat}); the component verification still uses no-follow + * semantics, but creation of those node types carries the documented residual TOCTOU window. Regular-file writes, the common + * and highest-volume case, are fully race-safe. + *

+ *

+ * Instances are created by {@link Extractor#newExtractor(Path)}; this type is an implementation detail and is not part of the + * public API surface. + *

+ */ +final class SecureExtractor extends Extractor { + + @SuppressWarnings("unchecked") + private static SecureDirectoryStream asSecure(final DirectoryStream stream) { + return (SecureDirectoryStream) stream; + } + + SecureExtractor(final Path rootDirectory) { + super(rootDirectory); + } + + @Override + void writeFile(final Path leaf, final InputStream content) throws IOException { + final Path relative = rootDirectory.relativize(leaf); + final int count = relative.getNameCount(); + final Path fileName = relative.getFileName(); + final Deque> handles = new ArrayDeque<>(); + try { + SecureDirectoryStream dir = asSecure(Files.newDirectoryStream(rootDirectory)); + handles.push(dir); + for (int i = 0; i < count - 1; i++) { + dir = asSecure(dir.newDirectoryStream(relative.getName(i), LinkOption.NOFOLLOW_LINKS)); + handles.push(dir); + } + runBeforeLeafWrite(); + final Set options = new HashSet<>(); + options.add(StandardOpenOption.WRITE); + options.add(LinkOption.NOFOLLOW_LINKS); + if (isOverwrite()) { + options.add(StandardOpenOption.CREATE); + options.add(StandardOpenOption.TRUNCATE_EXISTING); + } else { + options.add(StandardOpenOption.CREATE_NEW); + } + try (SeekableByteChannel channel = dir.newByteChannel(fileName, options); + OutputStream out = Channels.newOutputStream(channel)) { + IOUtils.copy(content, out); + } + } finally { + while (!handles.isEmpty()) { + handles.pop().close(); + } + } + } +} diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/SpecialFilePolicy.java b/src/main/java/org/apache/commons/compress/archivers/extractor/SpecialFilePolicy.java new file mode 100644 index 00000000000..a255632bddd --- /dev/null +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/SpecialFilePolicy.java @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +/** + * Controls how special entries (block devices, character devices, FIFOs) in an archive are handled during extraction. Such + * entries are never materialized by the secure path; this policy only decides whether their presence is tolerated. + * + * @since 1.29.0 + */ +public enum SpecialFilePolicy { + + /** + * Silently ignore special entries. This is the default. + */ + SKIP, + + /** + * Fail extraction if the archive contains a special entry. + */ + REJECT +} diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java b/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java new file mode 100644 index 00000000000..76ec4904b45 --- /dev/null +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +/** + * Controls how symbolic-link entries in an archive are handled during extraction. + * + * @since 1.29.0 + */ +public enum SymlinkPolicy { + + /** + * Fail extraction if the archive contains any symbolic-link entry. This is the default and the safest option for + * untrusted archives. + */ + REJECT, + + /** + * Silently ignore symbolic-link entries; nothing is created for them. + */ + SKIP, + + /** + * Create a symbolic link only if its target resolves to a location inside the extraction root; reject a link whose + * target escapes the root. A link created this way is never followed when later entries are written. + */ + ALLOW_WITHIN_ROOT, + + /** + * Create symbolic links verbatim, including links whose target escapes the extraction root. Only appropriate for fully + * trusted archives. + */ + ALLOW_ALL +} diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/package-info.java b/src/main/java/org/apache/commons/compress/archivers/extractor/package-info.java new file mode 100644 index 00000000000..3bc527757b7 --- /dev/null +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/package-info.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/** + * Safe, symlink-resistant extraction of archives into a directory. + *

+ * {@link org.apache.commons.compress.archivers.extractor.Extractor#newExtractor(java.nio.file.Path)} returns the strongest + * implementation the platform supports: a race-safe extractor backed by {@link java.nio.file.SecureDirectoryStream} where the + * file system provides one (Linux), otherwise a best-effort extractor that still resists symlink-slip from the archive but + * cannot fully close a concurrent TOCTOU race. Every path component is resolved with {@link java.nio.file.LinkOption#NOFOLLOW_LINKS} + * and regular files are written {@code CREATE_NEW} so a planted or archived symlink is never followed. + *

+ * + * @since 1.29.0 + */ +package org.apache.commons.compress.archivers.extractor; diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorBasePathTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorBasePathTest.java new file mode 100644 index 00000000000..b7ed4575b46 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorBasePathTest.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.commons.compress.archivers.tar.TarFile; +import org.apache.commons.compress.archivers.zip.ZipFile; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Exercises the best-effort base {@link Extractor} directly (not through the factory, which on Linux returns the secure + * subclass that overrides the file write) so the base path and the {@link TarFile} / {@link ZipFile} entry points are covered + * on every platform, including the gate platform. + */ +class ExtractorBasePathTest { + + @TempDir + Path target; + + private Extractor base() throws Exception { + return new Extractor(target.toRealPath()); + } + + @Test + void baseCreatesWithinRootSymlink() throws Exception { + final byte[] data = Fixtures.tar(dir("a"), dir("b"), symlink("a/link", "../b")); + Fixtures.extractTar(base().setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT), data); + assertTrue(Files.isSymbolicLink(target.resolve("a/link"))); + } + + @Test + void baseOverwriteTrueReplaces() throws Exception { + Files.write(target.resolve("a.txt"), "old".getBytes(StandardCharsets.UTF_8)); + final byte[] data = Fixtures.tar(file("a.txt", "new")); + Fixtures.extractTar(base().setOverwrite(true), data); + assertArrayEquals("new".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + } + + @Test + void extractsViaTarFile() throws Exception { + final byte[] data = Fixtures.tar(dir("a"), file("a/b.txt", "tarfile")); + try (TarFile archive = Fixtures.openTar(data)) { + base().extract(archive); + } + assertArrayEquals("tarfile".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b.txt"))); + } + + @Test + void extractsViaZipFile() throws Exception { + final byte[] data = Fixtures.zip(dir("a"), file("a/b.txt", "zipfile")); + try (ZipFile archive = Fixtures.openZip(data)) { + base().extract(archive); + } + assertArrayEquals("zipfile".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b.txt"))); + } + + @Test + void extractsHardLinkViaTarFile() throws Exception { + final byte[] data = Fixtures.tar(file("real.txt", "data"), hardlink("link", "real.txt")); + try (TarFile archive = Fixtures.openTar(data)) { + base().extract(archive); + } + assertTrue(Files.exists(target.resolve("link"))); + } + + @Test + void extractsSymlinkViaTarFile() throws Exception { + final byte[] data = Fixtures.tar(dir("b"), symlink("link", "b")); + try (TarFile archive = Fixtures.openTar(data)) { + base().setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT).extract(archive); + } + assertTrue(Files.isSymbolicLink(target.resolve("link"))); + } + + @Test + void extractsSymlinkViaZipFile() throws Exception { + final byte[] data = Fixtures.zip(dir("b"), symlink("link", "b")); + try (ZipFile archive = Fixtures.openZip(data)) { + base().setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT).extract(archive); + } + assertTrue(Files.isSymbolicLink(target.resolve("link"))); + } + + @Test + void createsStandaloneDirectory() throws Exception { + final byte[] data = Fixtures.tar(dir("emptydir")); + Fixtures.extractTar(base(), data); + assertTrue(Files.isDirectory(target.resolve("emptydir"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFactoryTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFactoryTest.java new file mode 100644 index 00000000000..239a741f628 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFactoryTest.java @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.SecureDirectoryStream; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Factory and platform detection: {@code newExtractor} returns the secure implementation exactly when the platform's file + * system provides a {@link SecureDirectoryStream}, and validates its target. + */ +class ExtractorFactoryTest { + + @TempDir + Path target; + + @Test + void factoryMatchesPlatformCapability() throws Exception { + final boolean secureFileSystem; + try (DirectoryStream stream = Files.newDirectoryStream(target)) { + secureFileSystem = stream instanceof SecureDirectoryStream; + } + assertEquals(secureFileSystem, Extractor.newExtractor(target) instanceof SecureExtractor); + } + + @Test + void rejectsMissingTarget() { + final Path missing = target.resolve("does-not-exist"); + assertThrows(IOException.class, () -> Extractor.newExtractor(missing)); + } + + @Test + void rejectsNonDirectoryTarget() throws Exception { + final Path file = Files.createFile(target.resolve("a-file")); + assertThrows(IOException.class, () -> Extractor.newExtractor(file)); + } + + @Test + void rejectsNullTarget() { + assertThrows(NullPointerException.class, () -> Extractor.newExtractor(null)); + } + + @Test + void settersReturnSameInstanceForChaining() throws Exception { + final Extractor extractor = Extractor.newExtractor(target); + assertSame(extractor, extractor.setSymlinkPolicy(SymlinkPolicy.SKIP)); + assertSame(extractor, extractor.setSpecialFilePolicy(SpecialFilePolicy.REJECT)); + assertSame(extractor, extractor.setOverwrite(true)); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFuzzTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFuzzTest.java new file mode 100644 index 00000000000..06ba50a4793 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorFuzzTest.java @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.util.Random; +import java.util.stream.Stream; + +import org.apache.commons.compress.archivers.tar.TarArchiveEntry; +import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; +import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream; +import org.apache.commons.compress.archivers.tar.TarConstants; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Property-based adversarial fuzz: hundreds of randomized malicious tars (traversal names, escaping symlink/hardlink targets, + * special files, mixed Unicode, deep nesting) extracted under the safe policies must never escape the extraction root and must + * fail only with {@link IOException} (never an unchecked crash). A canary file at the parent level proves no escape occurred. + * Seeds are fixed so any failure is reproducible. Jazzer-style coverage-guided fuzzing with traversal sanitizers is a CI + * addition; this gives a deterministic, dependency-free adversarial sweep. + */ +class ExtractorFuzzTest { + + private static final String[] NAMES = {"f", "d/f", "d/e/f", "../escape", "../../escape", "/abs/escape", "café", + "a/../b", "x", "d"}; + + private static final String[] TARGETS = {"../escape", "../../escape", "/etc/passwd", "d", "d/f", "x", "./y"}; + + private static final SymlinkPolicy[] SAFE_POLICIES = {SymlinkPolicy.REJECT, SymlinkPolicy.SKIP, + SymlinkPolicy.ALLOW_WITHIN_ROOT}; + + private static byte[] randomTar(final Random r) { + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(bos, "UTF-8")) { + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX); + tos.setAddPaxHeadersForNonAsciiNames(true); + final int count = 1 + r.nextInt(6); + for (int i = 0; i < count; i++) { + final String name = NAMES[r.nextInt(NAMES.length)] + (r.nextBoolean() ? "" : "/" + i); + final TarArchiveEntry entry; + byte[] content = null; + switch (r.nextInt(6)) { + case 0: + entry = new TarArchiveEntry(name); + content = ("d" + i).getBytes(StandardCharsets.UTF_8); + entry.setSize(content.length); + break; + case 1: + entry = new TarArchiveEntry(name.endsWith("/") ? name : name + "/"); + break; + case 2: + entry = new TarArchiveEntry(name, TarConstants.LF_SYMLINK); + entry.setLinkName(TARGETS[r.nextInt(TARGETS.length)]); + break; + case 3: + entry = new TarArchiveEntry(name, TarConstants.LF_LINK); + entry.setLinkName(TARGETS[r.nextInt(TARGETS.length)]); + break; + case 4: + entry = new TarArchiveEntry(name, TarConstants.LF_CHR); + break; + default: + entry = new TarArchiveEntry(name, TarConstants.LF_FIFO); + break; + } + tos.putArchiveEntry(entry); + if (content != null) { + tos.write(content); + } + tos.closeArchiveEntry(); + } + } catch (final IOException writerRejectedName) { + return null; + } + return bos.toByteArray(); + } + + @TempDir + Path target; + + @Test + void randomAdversarialArchivesNeverEscapeRoot() throws Exception { + final byte[] canaryBytes = "canary".getBytes(StandardCharsets.UTF_8); + final Path canary = Files.write(target.resolve("canary"), canaryBytes); + for (long seed = 0; seed < 384; seed++) { + final Random r = new Random(seed); + final Path root = Files.createDirectory(target.resolve("r" + seed)); + final byte[] tar = randomTar(r); + if (tar == null) { + continue; + } + final Extractor extractor = Extractor.newExtractor(root) + .setSymlinkPolicy(SAFE_POLICIES[r.nextInt(SAFE_POLICIES.length)]) + .setSpecialFilePolicy(r.nextBoolean() ? SpecialFilePolicy.SKIP : SpecialFilePolicy.REJECT) + .setOverwrite(r.nextBoolean()); + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(tar))) { + extractor.extract(in); + } catch (final IOException refusedOrMalformed) { + // A refusal (policy/escape/parse) is an acceptable outcome; only an escape or an unchecked crash is a bug. + } + } + assertArrayEquals(canaryBytes, Files.readAllBytes(canary), "the parent-level canary must be untouched"); + final long strayLeaves; + try (Stream entries = Files.list(target)) { + strayLeaves = entries.filter(p -> !Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)).count(); + } + assertEquals(1L, strayLeaves, "no file or link escaped to the parent directory (only the canary may be present)"); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHappyPathTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHappyPathTest.java new file mode 100644 index 00000000000..6d19e49e0f2 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHappyPathTest.java @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; +import org.apache.commons.compress.archivers.zip.ZipArchiveInputStream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Happy-path and boundary cases: a legitimate archive extracts faithfully, and an empty archive is a no-op. Proves the + * extractor does not over-block (no false positives). + */ +class ExtractorHappyPathTest { + + @TempDir + Path target; + + @Test + void emptyTarIsNoOp() throws Exception { + final byte[] data = Fixtures.tar(); + final Extractor extractor = Extractor.newExtractor(target); + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(data))) { + extractor.extract(in); + } + try (java.util.stream.Stream entries = Files.list(target)) { + assertTrue(!entries.findAny().isPresent(), "target should remain empty"); + } + } + + @Test + void extractsNestedTarViaArchiveInputStream() throws Exception { + final byte[] data = Fixtures.tar(dir("a"), file("a/b.txt", "hello")); + final Extractor extractor = Extractor.newExtractor(target); + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(data))) { + extractor.extract(in); + } + assertArrayEquals("hello".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b.txt"))); + } + + @Test + void extractsNestedZipViaArchiveInputStream() throws Exception { + final byte[] data = Fixtures.zip(dir("a"), file("a/b.txt", "world")); + final Extractor extractor = Extractor.newExtractor(target); + try (ZipArchiveInputStream in = new ZipArchiveInputStream(new ByteArrayInputStream(data))) { + extractor.extract(in); + } + assertArrayEquals("world".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b.txt"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHardLinkTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHardLinkTest.java new file mode 100644 index 00000000000..56a4cbb9d06 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorHardLinkTest.java @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Hard-link (tar {@code isLink()}) handling: a link whose target stays within the root is materialized as a hard link to the + * already-extracted file; a link whose target escapes the root is rejected. + */ +class ExtractorHardLinkTest { + + @TempDir + Path target; + + @Test + void createsHardLinkWithinRoot() throws Exception { + final byte[] data = Fixtures.tar(file("real.txt", "data"), hardlink("link", "real.txt")); + final Extractor extractor = Extractor.newExtractor(target); + Fixtures.extractTar(extractor, data); + assertArrayEquals("data".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("link"))); + final Object realKey = Files.readAttributes(target.resolve("real.txt"), BasicFileAttributes.class).fileKey(); + final Object linkKey = Files.readAttributes(target.resolve("link"), BasicFileAttributes.class).fileKey(); + assertEquals(realKey, linkKey); + } + + @Test + void rejectsHardLinkEscapingRoot() throws Exception { + final byte[] data = Fixtures.tar(hardlink("link", "../../etc/passwd")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java new file mode 100644 index 00000000000..1ae8da9143b --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; +import java.util.stream.Stream; + +import org.apache.commons.compress.archivers.ArchiveException; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * A malformed entry name that cannot be turned into a {@link Path} on this platform (a zip name field preserves a NUL byte, + * unlike a NUL-terminated ustar name) must fail closed with an {@link IOException}, never escape as an unchecked + * {@link InvalidPathException}. This is the "malicious input always yields IOException, never an unchecked crash" property the + * fuzz sweep relies on. + */ +class ExtractorMalformedNameTest { + + /** A zip name field can carry an embedded NUL, which {@code Paths.get} rejects with an {@link InvalidPathException}. */ + private static final String NUL_NAME = "pre" + (char) 0 + "post"; + + @TempDir + Path target; + + @Test + void nulByteZipNameFailsClosedAsArchiveException() throws Exception { + final byte[] data = Fixtures.zip(file(NUL_NAME, "x")); + final Extractor extractor = Extractor.newExtractor(target); + final IOException ex = assertThrows(IOException.class, () -> Fixtures.extractZip(extractor, data)); + assertInstanceOf(ArchiveException.class, ex); + assertInstanceOf(InvalidPathException.class, ex.getCause()); + } + + @Test + void nulByteZipNameLeavesNothingBehind() throws Exception { + final byte[] data = Fixtures.zip(file(NUL_NAME, "x")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractZip(extractor, data)); + try (Stream entries = Files.list(target)) { + assertEquals(0L, entries.count(), "a rejected malformed entry must not leave anything behind"); + } + } + + @Test + void wellFormedNameStillExtracts() throws Exception { + // The malformed-name guard must not regress legitimate extraction. + Fixtures.extractZip(Extractor.newExtractor(target), Fixtures.zip(file("ok.txt", "fine"))); + assertArrayEquals("fine".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("ok.txt"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorOverwriteTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorOverwriteTest.java new file mode 100644 index 00000000000..d469ec79700 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorOverwriteTest.java @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Overwrite policy. The default is fail-closed ({@code CREATE_NEW}): an existing target is never silently clobbered and an + * existing symbolic link at the target path is never followed. + */ +class ExtractorOverwriteTest { + + @TempDir + Path target; + + @Test + void failsClosedWhenTargetExists() throws Exception { + Files.write(target.resolve("a.txt"), "old".getBytes(StandardCharsets.UTF_8)); + final byte[] data = Fixtures.tar(file("a.txt", "new")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertArrayEquals("old".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + } + + @Test + void overwriteFalseDoesNotFollowExistingSymlink() throws Exception { + final Path outside = Files.createTempFile("compress-extractor-outside", ".txt"); + Files.write(outside, "secret".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(target.resolve("a.txt"), outside); + final byte[] data = Fixtures.tar(file("a.txt", "attacker")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertArrayEquals("secret".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(outside)); + } + + @Test + void overwriteReplacesExisting() throws Exception { + Files.write(target.resolve("a.txt"), "old".getBytes(StandardCharsets.UTF_8)); + final byte[] data = Fixtures.tar(file("a.txt", "new")); + final Extractor extractor = Extractor.newExtractor(target).setOverwrite(true); + Fixtures.extractTar(extractor, data); + assertArrayEquals("new".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + } + + @Test + void overwriteTrueStillDoesNotFollowExistingSymlink() throws Exception { + // Even with overwrite enabled the write is no-follow, so an existing symbolic link at the target is never traversed. + final Path outside = Files.createTempFile("compress-extractor-outside", ".txt"); + Files.write(outside, "secret".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(target.resolve("a.txt"), outside); + final byte[] data = Fixtures.tar(file("a.txt", "attacker")); + final Extractor extractor = Extractor.newExtractor(target).setOverwrite(true); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertArrayEquals("secret".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(outside)); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorPentestTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorPentestTest.java new file mode 100644 index 00000000000..21d64df3e76 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorPentestTest.java @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; + +import org.apache.commons.compress.archivers.zip.ZipFile; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Adversarial scopes beyond the core suite: a sibling directory that has the root as a string prefix (the classic + * {@code String.startsWith} trap, defeated by component-wise {@code Path.startsWith}); a hard link routed through a multi-hop + * symlink chain; a circular symlink (must fail closed, not hang); a hard link to a directory; a zip symbolic link with an + * escaping target through the {@code ZipFile} entry point; a symbolic link whose own name escapes the root; a bare {@code ..} + * name; and a deeply nested path (no stack overflow). + */ +class ExtractorPentestTest { + + @TempDir + Path target; + + @TempDir + Path attacker; + + @Test + void circularSymlinkFailsClosed() throws Exception { + Files.createSymbolicLink(target.resolve("loop"), target.resolve("loop")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(hardlink("x", "loop/y")))); + } + + @Test + void hardLinkToDirectoryFailsClosed() throws Exception { + final byte[] data = Fixtures.tar(dir("d"), hardlink("x", "d")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + } + + @Test + void rejectsBareDotDotEntryName() throws Exception { + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("..", "p")))); + } + + @Test + void rejectsHardLinkThroughSymlinkChain() throws Exception { + Files.write(attacker.resolve("secret"), "s".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(target.resolve("b"), attacker); + Files.createSymbolicLink(target.resolve("a"), target.resolve("b")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(hardlink("x", "a/secret")))); + assertFalse(Files.exists(target.resolve("x"))); + } + + @Test + void rejectsSiblingDirectoryWithRootAsPrefix() throws Exception { + final Path root = Files.createDirectory(target.resolve("root")); + final Extractor extractor = Extractor.newExtractor(root); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../root-evil/x", "p")))); + assertFalse(Files.exists(target.resolve("root-evil"))); + } + + @Test + void rejectsSymlinkWhoseNameEscapes() throws Exception { + final byte[] data = Fixtures.tar(symlink("../evil", "x")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_ALL); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(target.getParent().resolve("evil"), LinkOption.NOFOLLOW_LINKS)); + } + + @Test + void rejectsZipSymlinkEscapeViaZipFile() throws Exception { + final byte[] data = Fixtures.zip(symlink("link", "/etc")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + try (ZipFile zip = Fixtures.openZip(data)) { + assertThrows(IOException.class, () -> extractor.extract(zip)); + } + assertFalse(Files.exists(target.resolve("link"), LinkOption.NOFOLLOW_LINKS)); + } + + @Test + void toleratesDeepNestingWithoutStackOverflow() throws Exception { + final StringBuilder name = new StringBuilder(); + for (int i = 0; i < 120; i++) { + name.append("a/"); + } + name.append("file.txt"); + final Extractor extractor = Extractor.newExtractor(target); + Fixtures.extractTar(extractor, Fixtures.tar(file(name.toString(), "deep"))); + assertTrue(Files.exists(target.resolve(name.toString()))); + } + + @Test + void manyLegitimateEntriesAllExtract() throws Exception { + // A large legitimate archive must extract every entry: the safe defaults must not over-block real data. + final Fixtures.Entry[] entries = new Fixtures.Entry[40]; + for (int i = 0; i < entries.length; i++) { + entries[i] = file("dir" + i % 5 + "/f" + i + ".txt", "v" + i); + } + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(entries)); + for (int i = 0; i < entries.length; i++) { + assertTrue(Files.exists(target.resolve("dir" + i % 5 + "/f" + i + ".txt")), "entry " + i + " should exist"); + } + } + + @Test + void fileNameCollidingWithExistingDirectoryFailsClosed() throws Exception { + Files.createDirectory(target.resolve("collide")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("collide", "x")))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java new file mode 100644 index 00000000000..d202354c491 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java @@ -0,0 +1,229 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.fifo; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.Arrays; +import java.util.stream.Stream; + +import org.apache.commons.compress.archivers.zip.ZipFile; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Resilience and robustness cases beyond the core threat suite: legitimate-but-awkward inputs (empty, Unicode, long, dotted, + * duplicate names), idempotency and reuse, mixed entry types, partial-state containment when a later entry is rejected, and the + * fail-closed behavior of forward hard-link references and null configuration. These prove the extractor neither over-blocks + * legitimate archives nor leaks state on the unhappy paths. + */ +class ExtractorResilienceTest { + + @TempDir + Path target; + + @TempDir + Path attacker; + + private static byte[] bytes(final String s) { + return s.getBytes(StandardCharsets.UTF_8); + } + + @Test + void emptyFileEntryExtractsAsEmptyFile() throws Exception { + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("empty", ""))); + assertTrue(Files.exists(target.resolve("empty"))); + assertEquals(0L, Files.size(target.resolve("empty"))); + } + + @Test + void unicodeEntryNameExtractsContained() throws Exception { + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("café/ünï.txt", "hi"))); + assertArrayEquals(bytes("hi"), Files.readAllBytes(target.resolve("café/ünï.txt"))); + } + + @Test + void deeplyNestedFileCreatesAllParents() throws Exception { + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("p/q/r/s.txt", "x"))); + assertTrue(Files.isDirectory(target.resolve("p/q/r"))); + assertTrue(Files.exists(target.resolve("p/q/r/s.txt"))); + } + + @Test + void dotSegmentNameNormalizedAndContained() throws Exception { + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("a/./b.txt", "x"))); + assertTrue(Files.exists(target.resolve("a/b.txt"))); + assertFalse(Files.exists(target.getParent().resolve("b.txt"))); + } + + @Test + void setSymlinkPolicyNullRejected() throws Exception { + final Extractor e = Extractor.newExtractor(target); + assertThrows(NullPointerException.class, () -> e.setSymlinkPolicy(null)); + } + + @Test + void setSpecialFilePolicyNullRejected() throws Exception { + final Extractor e = Extractor.newExtractor(target); + assertThrows(NullPointerException.class, () -> e.setSpecialFilePolicy(null)); + } + + @Test + void extractorReusedForTwoArchives() throws Exception { + final Extractor e = Extractor.newExtractor(target); + Fixtures.extractTar(e, Fixtures.tar(file("a.txt", "1"))); + Fixtures.extractTar(e, Fixtures.tar(file("b.txt", "2"))); + assertTrue(Files.exists(target.resolve("a.txt"))); + assertTrue(Files.exists(target.resolve("b.txt"))); + } + + @Test + void mixedEntryTypesAllExtract() throws Exception { + final byte[] data = Fixtures.tar(dir("d"), file("d/f.txt", "x"), file("real.txt", "r"), symlink("d/link", "../real.txt"), + hardlink("h.txt", "real.txt")); + final Extractor e = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + Fixtures.extractTar(e, data); + assertTrue(Files.exists(target.resolve("d/f.txt"))); + assertTrue(Files.isSymbolicLink(target.resolve("d/link"))); + assertTrue(Files.exists(target.resolve("h.txt"))); + } + + @Test + void hardLinkForwardReferenceFailsClosed() throws Exception { + final byte[] data = Fixtures.tar(hardlink("link", "later.txt")); + final Extractor e = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(e, data)); + } + + @Test + void createdSymlinkDoesNotBlockLaterLegitWrites() throws Exception { + final byte[] data = Fixtures.tar(dir("b"), symlink("b/link", "."), file("c.txt", "ok")); + final Extractor e = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + Fixtures.extractTar(e, data); + assertTrue(Files.isSymbolicLink(target.resolve("b/link"))); + assertTrue(Files.exists(target.resolve("c.txt"))); + } + + @Test + void midArchiveEscapeLeavesEarlierEntriesButNoEscape() throws Exception { + final byte[] data = Fixtures.tar(file("good.txt", "ok"), file("../evil.txt", "bad")); + final Extractor e = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(e, data)); + assertTrue(Files.exists(target.resolve("good.txt")), "earlier valid entry written"); + assertFalse(Files.exists(target.getParent().resolve("evil.txt")), "escape blocked"); + } + + @Test + void trailingSlashNameIsDirectory() throws Exception { + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(dir("onlydir"))); + assertTrue(Files.isDirectory(target.resolve("onlydir"))); + } + + @Test + void longSingleComponentNameExtracts() throws Exception { + final char[] c = new char[150]; + Arrays.fill(c, 'a'); + final String name = new String(c) + ".txt"; + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file(name, "x"))); + assertTrue(Files.exists(target.resolve(name))); + } + + @Test + void multipleHardLinksShareInode() throws Exception { + final byte[] data = Fixtures.tar(file("real.txt", "data"), hardlink("h1.txt", "real.txt"), hardlink("h2.txt", "real.txt")); + Fixtures.extractTar(Extractor.newExtractor(target), data); + final Object k0 = Files.readAttributes(target.resolve("real.txt"), BasicFileAttributes.class).fileKey(); + assertEquals(k0, Files.readAttributes(target.resolve("h1.txt"), BasicFileAttributes.class).fileKey()); + assertEquals(k0, Files.readAttributes(target.resolve("h2.txt"), BasicFileAttributes.class).fileKey()); + } + + @Test + void duplicateDirectoryEntryTolerated() throws Exception { + final byte[] data = Fixtures.tar(dir("d"), dir("d"), file("d/f.txt", "x")); + Fixtures.extractTar(Extractor.newExtractor(target), data); + assertTrue(Files.exists(target.resolve("d/f.txt"))); + } + + @Test + void zipFileSymlinkSlipBlocked() throws Exception { + final byte[] data = Fixtures.zip(dir("b"), symlink("link", "b"), file("link/evil", "x")); + final Extractor e = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + try (ZipFile zip = Fixtures.openZip(data)) { + assertThrows(IOException.class, () -> e.extract(zip)); + } + assertFalse(Files.exists(target.resolve("b/evil"))); + } + + @Test + void skipPolicyIgnoresZipSymlinkButExtractsRest() throws Exception { + final byte[] data = Fixtures.zip(symlink("link", "/etc"), file("real.txt", "ok")); + final Extractor e = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.SKIP); + try (ZipFile zip = Fixtures.openZip(data)) { + e.extract(zip); + } + assertFalse(Files.exists(target.resolve("link"), LinkOption.NOFOLLOW_LINKS)); + assertTrue(Files.exists(target.resolve("real.txt"))); + } + + @Test + void specialRejectStopsExtraction() throws Exception { + final byte[] data = Fixtures.tar(file("a.txt", "ok"), fifo("f"), file("b.txt", "later")); + final Extractor e = Extractor.newExtractor(target).setSpecialFilePolicy(SpecialFilePolicy.REJECT); + assertThrows(IOException.class, () -> Fixtures.extractTar(e, data)); + assertTrue(Files.exists(target.resolve("a.txt"))); + assertFalse(Files.exists(target.resolve("b.txt"))); + } + + @Test + void emptyZipViaZipFileIsNoOp() throws Exception { + final byte[] data = Fixtures.zip(); + final Extractor e = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(data)) { + e.extract(zip); + } + try (Stream s = Files.list(target)) { + assertFalse(s.findAny().isPresent(), "target should remain empty"); + } + } + + @Test + void allowAllCreatesRelativeEscapingTargetButLeafStaysInRoot() throws Exception { + final byte[] data = Fixtures.tar(symlink("escape-link", "../../../../etc/passwd")); + final Extractor e = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_ALL); + Fixtures.extractTar(e, data); + final Path link = target.resolve("escape-link"); + assertTrue(Files.isSymbolicLink(link)); + assertEquals("../../../../etc/passwd", Files.readSymbolicLink(link).toString()); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java new file mode 100644 index 00000000000..16ce7d45f5e --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Root-boundary invariant: {@code newExtractor} canonicalizes the target with {@code toRealPath}, so the containment guard is + * not sensitive to how the target is spelled (trailing separator, {@code .} component, or a symlinked path). This is why the + * extractor uses {@code resolveWithinRoot} against a canonical root rather than the lexical {@code ArchiveEntry.resolveIn}, + * which compares against the parent string as given and misbehaves when that parent is not normalized. + */ +class ExtractorRootBoundaryTest { + + @TempDir + Path target; + + @Test + void rejectsTraversalWhenTargetHasTrailingSeparator() throws Exception { + final Path slashy = Paths.get(target.toString() + "/"); + final Extractor extractor = Extractor.newExtractor(slashy); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); + assertFalse(Files.exists(target.getParent().resolve("escape"))); + } + + @Test + void rejectsTraversalWhenTargetHasDotComponent() throws Exception { + final Path dotted = Paths.get(target.toString(), "."); + final Extractor extractor = Extractor.newExtractor(dotted); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); + assertFalse(Files.exists(target.getParent().resolve("escape"))); + } + + @Test + void canonicalizesSymlinkedRootAndStaysContained() throws Exception { + final Path real = Files.createDirectory(target.resolve("real")); + final Path link = Files.createSymbolicLink(target.resolve("link"), real); + Fixtures.extractTar(Extractor.newExtractor(link), Fixtures.tar(file("a.txt", "hi"))); + assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(real.resolve("a.txt"))); + final Extractor extractor = Extractor.newExtractor(link); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); + assertFalse(Files.exists(target.resolve("escape"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSecurePathTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSecurePathTest.java new file mode 100644 index 00000000000..3d4f37fcdfd --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSecurePathTest.java @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Positive and adversarial coverage of the {@link SecureExtractor} regular-file write path, which only activates on a platform + * whose file system provides a {@link java.nio.file.SecureDirectoryStream} (Linux). Each test asserts the secure subclass is in + * use, so on non-secure platforms (macOS, Windows) the cases are skipped rather than silently exercising the base path. + */ +class ExtractorSecurePathTest { + + @TempDir + Path target; + + @TempDir + Path attacker; + + @BeforeEach + void requireSecurePlatform() throws Exception { + assumeTrue(Extractor.newExtractor(target) instanceof SecureExtractor, "requires a SecureDirectoryStream platform (Linux)"); + } + + @Test + void writesNestedFileRelativeToPinnedHandle() throws Exception { + final byte[] data = Fixtures.tar(dir("a"), dir("a/b"), file("a/b/c.txt", "deep-secure")); + Fixtures.extractTar(Extractor.newExtractor(target), data); + assertArrayEquals("deep-secure".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b/c.txt"))); + } + + @Test + void rejectsWriteThroughPreExistingParentSymlink() throws Exception { + Files.createSymbolicLink(target.resolve("sub"), attacker); + final byte[] data = Fixtures.tar(file("sub/file.txt", "x")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(attacker.resolve("file.txt"))); + } + + @Test + void overwriteReplacesThroughPinnedHandle() throws Exception { + Files.write(target.resolve("a.txt"), "old".getBytes(StandardCharsets.UTF_8)); + final byte[] data = Fixtures.tar(file("a.txt", "new")); + Fixtures.extractTar(Extractor.newExtractor(target).setOverwrite(true), data); + assertArrayEquals("new".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + } + + @Test + void overwriteDoesNotFollowExistingSymlinkThroughPinnedHandle() throws Exception { + final Path outside = Files.write(attacker.resolve("secret"), "secret".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(target.resolve("a.txt"), outside); + final byte[] data = Fixtures.tar(file("a.txt", "attacker")); + final Extractor extractor = Extractor.newExtractor(target).setOverwrite(true); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertArrayEquals("secret".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(outside)); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSpecialFileTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSpecialFileTest.java new file mode 100644 index 00000000000..efda753492c --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSpecialFileTest.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.blockDev; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.charDev; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.fifo; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Special-entry (device, FIFO) handling: skipped by default so the rest of the archive still extracts, or rejected when the + * policy demands it. The secure path never materializes such entries. + */ +class ExtractorSpecialFileTest { + + @TempDir + Path target; + + @Test + void rejectsSpecialWhenConfigured() throws Exception { + final byte[] data = Fixtures.tar(fifo("fifo0")); + final Extractor extractor = Extractor.newExtractor(target).setSpecialFilePolicy(SpecialFilePolicy.REJECT); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + } + + @Test + void skipsSpecialByDefault() throws Exception { + final byte[] data = Fixtures.tar(charDev("cdev0"), fifo("fifo0"), file("real.txt", "ok")); + final Extractor extractor = Extractor.newExtractor(target); + Fixtures.extractTar(extractor, data); + assertFalse(Files.exists(target.resolve("cdev0"), LinkOption.NOFOLLOW_LINKS)); + assertFalse(Files.exists(target.resolve("fifo0"), LinkOption.NOFOLLOW_LINKS)); + assertTrue(Files.exists(target.resolve("real.txt"))); + } + + @Test + void skipsBlockDeviceByDefault() throws Exception { + final byte[] data = Fixtures.tar(blockDev("bdev0"), file("real.txt", "ok")); + Fixtures.extractTar(Extractor.newExtractor(target), data); + assertFalse(Files.exists(target.resolve("bdev0"), LinkOption.NOFOLLOW_LINKS)); + assertTrue(Files.exists(target.resolve("real.txt"))); + } + + @Test + void rejectsBlockDeviceWhenConfigured() throws Exception { + final byte[] data = Fixtures.tar(blockDev("bdev0")); + final Extractor extractor = Extractor.newExtractor(target).setSpecialFilePolicy(SpecialFilePolicy.REJECT); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java new file mode 100644 index 00000000000..9c84a52cd26 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; + +import org.apache.commons.compress.archivers.zip.ZipFile; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Symbolic-link policy behavior. The archive itself is trusted; these prove that the extractor controls link creation and + * never follows a created link, regardless of platform (base or secure). + */ +class ExtractorSymlinkPolicyTest { + + @TempDir + Path target; + + @Test + void allowAllCreatesEscapingLink() throws Exception { + final byte[] data = Fixtures.tar(symlink("link", "/etc")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_ALL); + Fixtures.extractTar(extractor, data); + final Path link = target.resolve("link"); + assertTrue(Files.isSymbolicLink(link)); + assertEquals("/etc", Files.readSymbolicLink(link).toString()); + } + + @Test + void allowWithinRootCreatesInRootLink() throws Exception { + final byte[] data = Fixtures.tar(dir("a"), dir("b"), symlink("a/link", "../b")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + Fixtures.extractTar(extractor, data); + final Path link = target.resolve("a/link"); + assertTrue(Files.isSymbolicLink(link)); + assertEquals("../b", Files.readSymbolicLink(link).toString()); + } + + @Test + void allowWithinRootRejectsAbsoluteEscape() throws Exception { + final byte[] data = Fixtures.tar(symlink("link", "/etc")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(target.resolve("link"), LinkOption.NOFOLLOW_LINKS)); + } + + @Test + void allowWithinRootRejectsRelativeEscape() throws Exception { + final byte[] data = Fixtures.tar(symlink("link", "../../etc/passwd")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(target.resolve("link"), LinkOption.NOFOLLOW_LINKS)); + } + + @Test + void rejectsSymlinkByDefaultTar() throws Exception { + final byte[] data = Fixtures.tar(symlink("link", "/etc")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + } + + @Test + void rejectsSymlinkByDefaultZip() throws Exception { + final byte[] data = Fixtures.zip(symlink("link", "/etc")); + final Extractor extractor = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(data)) { + assertThrows(IOException.class, () -> extractor.extract(zip)); + } + } + + @Test + void skipSilentlyIgnoresSymlink() throws Exception { + final byte[] data = Fixtures.tar(symlink("link", "/etc"), file("real.txt", "ok")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.SKIP); + Fixtures.extractTar(extractor, data); + assertFalse(Files.exists(target.resolve("link"), LinkOption.NOFOLLOW_LINKS)); + assertTrue(Files.exists(target.resolve("real.txt"))); + } + + @Test + void allowWithinRootCreatesZipSymlinkViaZipFile() throws Exception { + final byte[] data = Fixtures.zip(dir("a"), dir("b"), symlink("a/link", "../b")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + try (ZipFile zip = Fixtures.openZip(data)) { + extractor.extract(zip); + } + final Path link = target.resolve("a/link"); + assertTrue(Files.isSymbolicLink(link)); + assertEquals("../b", Files.readSymbolicLink(link).toString()); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkSlipTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkSlipTest.java new file mode 100644 index 00000000000..0c18ba9de17 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkSlipTest.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Symlink-slip and duplicate-entry hardening: writes must never traverse a symbolic link in a parent component, whether the + * link was planted on disk beforehand or created by an earlier archive entry, and a duplicate entry must fail closed rather + * than overwrite. These are the generic defensive properties that also defeat normalization/case-collision variants, since + * the defense is a file-system-level no-follow check on the resolved inode, not a lexical name comparison. + */ +class ExtractorSymlinkSlipTest { + + @TempDir + Path target; + + @TempDir + Path attacker; + + @Test + void duplicateEntryFailsClosed() throws Exception { + final byte[] data = Fixtures.tar(file("a.txt", "first"), file("a.txt", "second")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertArrayEquals("first".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + } + + @Test + void neverFollowsCreatedSymlinkForLaterWrite() throws Exception { + final byte[] data = Fixtures.tar(dir("b"), symlink("link", "b"), file("link/evil", "x")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(target.resolve("b/evil"))); + } + + @Test + void rejectsWriteThroughPreExistingParentSymlink() throws Exception { + Files.createSymbolicLink(target.resolve("sub"), attacker); + final byte[] data = Fixtures.tar(file("sub/file.txt", "x")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(attacker.resolve("file.txt"))); + } + + @Test + void baseRejectsWriteThroughPreExistingParentSymlink() throws Exception { + Files.createSymbolicLink(target.resolve("sub"), attacker); + final byte[] data = Fixtures.tar(file("sub/file.txt", "x")); + final Extractor extractor = new Extractor(target.toRealPath()); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(attacker.resolve("file.txt"))); + } + + @Test + void rejectsHardLinkTargetThroughSymlink() throws Exception { + // node-tar CVE-2026-26960 class: a hard link whose target routes through a symlink must not escape the root. + Files.write(attacker.resolve("secret"), "secret".getBytes(StandardCharsets.UTF_8)); + Files.createSymbolicLink(target.resolve("s"), attacker); + final byte[] data = Fixtures.tar(hardlink("x", "s/secret")); + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, data)); + assertFalse(Files.exists(target.resolve("x"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java new file mode 100644 index 00000000000..546d96fe6c4 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.rawFile; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Zip-slip regression: entry names that escape the target via {@code ..} or an absolute path must be rejected, preserving the + * lexical guard that {@code ArchiveEntry.resolveIn} provides today. + */ +class ExtractorZipSlipTest { + + @TempDir + Path target; + + @Test + void rejectsAbsoluteName() throws Exception { + final byte[] data = Fixtures.tar(rawFile("/tmp/compress-extractor-evil", "x")); + final Extractor extractor = Extractor.newExtractor(target); + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(data))) { + assertThrows(IOException.class, () -> extractor.extract(in)); + } + assertFalse(Files.exists(Paths.get("/tmp/compress-extractor-evil"))); + } + + @Test + void rejectsParentTraversal() throws Exception { + final byte[] data = Fixtures.tar(file("../compress-extractor-evil.txt", "x")); + final Extractor extractor = Extractor.newExtractor(target); + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(data))) { + assertThrows(IOException.class, () -> extractor.extract(in)); + } + assertFalse(Files.exists(target.getParent().resolve("compress-extractor-evil.txt"))); + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/Fixtures.java b/src/test/java/org/apache/commons/compress/archivers/extractor/Fixtures.java new file mode 100644 index 00000000000..11e6e02d625 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/Fixtures.java @@ -0,0 +1,208 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.apache.commons.compress.archivers.tar.TarArchiveEntry; +import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; +import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream; +import org.apache.commons.compress.archivers.tar.TarConstants; +import org.apache.commons.compress.archivers.tar.TarFile; +import org.apache.commons.compress.archivers.zip.UnixStat; +import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; +import org.apache.commons.compress.archivers.zip.ZipArchiveInputStream; +import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream; +import org.apache.commons.compress.archivers.zip.ZipFile; + +/** + * Builds in-memory tar and zip archives with arbitrary entry types (regular, directory, symbolic link, hard link, special) + * for extractor tests. Entries are written with the public archive writers, which faithfully encode link flags and unix + * modes. {@link Entry#rawFile} preserves a leading {@code /} (the writer strips it by default), so an absolute-path escape can + * be exercised. Duplicate-name and collision fixtures (not used in this public suite) would need low-level construction. + */ +final class Fixtures { + + enum Kind { + FILE, DIR, SYMLINK, HARDLINK, CHARDEV, BLOCKDEV, FIFO + } + + static final class Entry { + + static Entry blockDev(final String name) { + return new Entry(Kind.BLOCKDEV, name, null, false); + } + + static Entry charDev(final String name) { + return new Entry(Kind.CHARDEV, name, null, false); + } + + static Entry dir(final String name) { + return new Entry(Kind.DIR, name, null, false); + } + + static Entry fifo(final String name) { + return new Entry(Kind.FIFO, name, null, false); + } + + static Entry file(final String name, final String content) { + return new Entry(Kind.FILE, name, content, false); + } + + static Entry hardlink(final String name, final String target) { + return new Entry(Kind.HARDLINK, name, target, false); + } + + static Entry rawFile(final String name, final String content) { + return new Entry(Kind.FILE, name, content, true); + } + + static Entry symlink(final String name, final String target) { + return new Entry(Kind.SYMLINK, name, target, false); + } + + final Kind kind; + final String name; + final String data; + final boolean preserveAbsolute; + + private Entry(final Kind kind, final String name, final String data, final boolean preserveAbsolute) { + this.kind = kind; + this.name = name; + this.data = data; + this.preserveAbsolute = preserveAbsolute; + } + } + + static void extractTar(final Extractor extractor, final byte[] tar) throws IOException { + try (TarArchiveInputStream in = new TarArchiveInputStream(new ByteArrayInputStream(tar))) { + extractor.extract(in); + } + } + + static void extractZip(final Extractor extractor, final byte[] zip) throws IOException { + try (ZipArchiveInputStream in = new ZipArchiveInputStream(new ByteArrayInputStream(zip))) { + extractor.extract(in); + } + } + + static byte[] tar(final Entry... entries) throws IOException { + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(bos)) { + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX); + for (final Entry e : entries) { + final TarArchiveEntry te; + byte[] content = null; + switch (e.kind) { + case FILE: + te = e.preserveAbsolute ? new TarArchiveEntry(e.name, TarConstants.LF_NORMAL, true) : new TarArchiveEntry(e.name); + content = e.data.getBytes(StandardCharsets.UTF_8); + te.setSize(content.length); + break; + case DIR: + te = new TarArchiveEntry(e.name.endsWith("/") ? e.name : e.name + "/"); + break; + case SYMLINK: + te = new TarArchiveEntry(e.name, TarConstants.LF_SYMLINK); + te.setLinkName(e.data); + break; + case HARDLINK: + te = new TarArchiveEntry(e.name, TarConstants.LF_LINK); + te.setLinkName(e.data); + break; + case CHARDEV: + te = new TarArchiveEntry(e.name, TarConstants.LF_CHR); + break; + case BLOCKDEV: + te = new TarArchiveEntry(e.name, TarConstants.LF_BLK); + break; + case FIFO: + te = new TarArchiveEntry(e.name, TarConstants.LF_FIFO); + break; + default: + throw new IllegalArgumentException(String.valueOf(e.kind)); + } + tos.putArchiveEntry(te); + if (content != null) { + tos.write(content); + } + tos.closeArchiveEntry(); + } + } + return bos.toByteArray(); + } + + static byte[] zip(final Entry... entries) throws IOException { + final ByteArrayOutputStream bos = new ByteArrayOutputStream(); + try (ZipArchiveOutputStream zos = new ZipArchiveOutputStream(bos)) { + for (final Entry e : entries) { + final ZipArchiveEntry ze; + byte[] content = null; + switch (e.kind) { + case FILE: + ze = new ZipArchiveEntry(e.name); + content = e.data.getBytes(StandardCharsets.UTF_8); + break; + case DIR: + ze = new ZipArchiveEntry(e.name.endsWith("/") ? e.name : e.name + "/"); + break; + case SYMLINK: + ze = new ZipArchiveEntry(e.name); + ze.setUnixMode(UnixStat.LINK_FLAG | 0777); + content = e.data.getBytes(StandardCharsets.UTF_8); + break; + default: + throw new IllegalArgumentException("zip fixture does not support " + e.kind); + } + zos.putArchiveEntry(ze); + if (content != null) { + zos.write(content); + } + zos.closeArchiveEntry(); + } + } + return bos.toByteArray(); + } + + static TarFile openTar(final byte[] tar) throws IOException { + final Path file = Files.createTempFile("compress-extractor-tar", ".tar"); + Files.write(file, tar); + return TarFile.builder().setPath(file).get(); + } + + static ZipFile openZip(final byte[] zip) throws IOException { + final Path file = Files.createTempFile("compress-extractor-zip", ".zip"); + Files.write(file, zip); + return ZipFile.builder().setPath(file).get(); + } + + static Path write(final Path dir, final String name, final byte[] data) throws IOException { + final Path p = dir.resolve(name); + Files.write(p, data); + return p; + } + + private Fixtures() { + } +} diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/SecureExtractorRaceTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/SecureExtractorRaceTest.java new file mode 100644 index 00000000000..e0b1d10d519 --- /dev/null +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/SecureExtractorRaceTest.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.commons.compress.archivers.extractor; + +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Concurrent TOCTOU race: a third party swaps a parent directory component for a symbolic link after the extractor has + * resolved the parent but before the leaf file is written (driven deterministically through the {@code beforeLeafWrite} test + * seam). The secure extractor, which writes relative to a pinned directory handle, must not let the write escape; the base + * extractor, which re-resolves the path, follows the swap (the documented residual). + */ +class SecureExtractorRaceTest { + + @TempDir + Path target; + + @TempDir + Path attacker; + + private static byte[] entryUnderSub() throws IOException { + return Fixtures.tar(dir("sub"), file("sub/file.txt", "payload")); + } + + private Runnable swapSubForSymlink(final Path root) { + return () -> { + try { + final Path sub = root.resolve("sub"); + Files.delete(sub); + Files.createSymbolicLink(sub, attacker); + } catch (final IOException e) { + throw new UncheckedIOException(e); + } + }; + } + + @Test + void baseExtractorDoesNotDefeatParentSwap() throws Exception { + final Path root = target.toRealPath(); + final Extractor extractor = new Extractor(root); + extractor.setBeforeLeafWrite(swapSubForSymlink(root)); + Fixtures.extractTar(extractor, entryUnderSub()); + assertTrue(Files.exists(attacker.resolve("file.txt")), "base extractor follows the swapped parent (documented residual)"); + } + + @Test + void secureExtractorDefeatsParentSwap() throws Exception { + final Path root = target.toRealPath(); + final Extractor extractor = Extractor.newExtractor(target); + assumeTrue(extractor instanceof SecureExtractor, "requires a SecureDirectoryStream platform (Linux)"); + extractor.setBeforeLeafWrite(swapSubForSymlink(root)); + try { + Fixtures.extractTar(extractor, entryUnderSub()); + } catch (final IOException failClosed) { + // Fail-closed (the pinned parent was unlinked) is an acceptable secure outcome; the escape must not happen. + } + assertFalse(Files.exists(attacker.resolve("file.txt")), "secure extractor must not write through the swapped parent"); + } +} From 9db201e13ee91783f60f219b7e55263bc2e057c4 Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Sun, 28 Jun 2026 18:54:41 -0300 Subject: [PATCH 2/6] [COMPRESS-727] Fix symlink target assertions on Windows Compare Path values instead of toString() so the platform separator does not break the test. --- .../archivers/extractor/ExtractorResilienceTest.java | 2 +- .../archivers/extractor/ExtractorSymlinkPolicyTest.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java index d202354c491..d433780f33c 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorResilienceTest.java @@ -224,6 +224,6 @@ void allowAllCreatesRelativeEscapingTargetButLeafStaysInRoot() throws Exception Fixtures.extractTar(e, data); final Path link = target.resolve("escape-link"); assertTrue(Files.isSymbolicLink(link)); - assertEquals("../../../../etc/passwd", Files.readSymbolicLink(link).toString()); + assertEquals(link.getFileSystem().getPath("../../../../etc/passwd"), Files.readSymbolicLink(link)); } } diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java index 9c84a52cd26..235f4704f20 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java @@ -51,7 +51,7 @@ void allowAllCreatesEscapingLink() throws Exception { Fixtures.extractTar(extractor, data); final Path link = target.resolve("link"); assertTrue(Files.isSymbolicLink(link)); - assertEquals("/etc", Files.readSymbolicLink(link).toString()); + assertEquals(link.getFileSystem().getPath("/etc"), Files.readSymbolicLink(link)); } @Test @@ -61,7 +61,7 @@ void allowWithinRootCreatesInRootLink() throws Exception { Fixtures.extractTar(extractor, data); final Path link = target.resolve("a/link"); assertTrue(Files.isSymbolicLink(link)); - assertEquals("../b", Files.readSymbolicLink(link).toString()); + assertEquals(link.getFileSystem().getPath("../b"), Files.readSymbolicLink(link)); } @Test @@ -114,6 +114,6 @@ void allowWithinRootCreatesZipSymlinkViaZipFile() throws Exception { } final Path link = target.resolve("a/link"); assertTrue(Files.isSymbolicLink(link)); - assertEquals("../b", Files.readSymbolicLink(link).toString()); + assertEquals(link.getFileSystem().getPath("../b"), Files.readSymbolicLink(link)); } } From 1b45e213690bac0e5ab7b6bf19f8211e2be53546 Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Thu, 2 Jul 2026 00:36:21 -0300 Subject: [PATCH 3/6] [COMPRESS-727] Document ALLOW_WITHIN_ROOT as best-effort and skip root-resolving entries ALLOW_WITHIN_ROOT can't fully guarantee containment, so the Javadoc now says it is lexical and best-effort, with REJECT and SKIP as the safe modes for untrusted input, plus a Safe Extraction section in the user guide. resolveWithinRoot returns null for names that resolve to the root itself like "." or "a/..", and the caller skips them rather than writing at the root; escapes still throw. Also folded the four containment checks into one isWithinRoot helper, and noted that hard-link creation carries the same residual TOCTOU as symlink creation. --- .../archivers/extractor/Extractor.java | 36 +++++- .../archivers/extractor/SymlinkPolicy.java | 13 ++- src/site/xdoc/examples.xml | 73 ++++++++++++ .../extractor/ExtractorRootBoundaryTest.java | 106 ++++++++++++++++++ .../extractor/ExtractorSymlinkPolicyTest.java | 82 ++++++++++++++ 5 files changed, 301 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java index 2c88863599e..e32c48ba33e 100644 --- a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java +++ b/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)) { + return null; + } + if (!isWithinRoot(resolved)) { throw new ArchiveException("Entry '%s' would escape the extraction root", name); } return resolved; @@ -281,6 +297,11 @@ private void processEntry(final String name, final EntryType type, final String } } final Path leaf = resolveWithinRoot(name); + if (leaf == null) { + // The entry resolves to the extraction root itself; there is nothing to materialize and writing here would + // target or replace the root, so skip it. + return; + } ensureDirectory(leaf.getParent()); switch (type) { case DIRECTORY: @@ -355,17 +376,20 @@ void writeFile(final Path leaf, final InputStream content) throws IOException { /** * Creates a hard link at {@code leaf} to {@code linkTarget}, which is resolved against the extraction root and rejected if - * it escapes. + * it escapes. The target is re-resolved with {@link Path#toRealPath} and re-checked before {@link Files#createLink}, which + * closes the hard-link-through-symlink escape. A residual time-of-check-to-time-of-use window remains between that re-check + * and the link creation, the same non-atomic residual as symbolic-link creation, as {@code java.nio} exposes no + * descriptor-relative {@code linkat} for the secure implementation to use. */ void writeHardLink(final Path leaf, final String linkTarget) throws IOException { final Path resolved = rootDirectory.resolve(linkTarget).normalize(); - if (!resolved.startsWith(rootDirectory)) { + if (!isWithinRoot(resolved)) { throw new ArchiveException("Hard link target escapes the extraction root: %s -> %s", leaf, linkTarget); } // The lexical check is not enough: a target routed through a symbolic link (planted or from an earlier entry) would // be followed by createLink and escape the root. Re-resolve on disk and re-check containment, then link the real file. final Path real = resolved.toRealPath(); - if (!real.startsWith(rootDirectory)) { + if (!isWithinRoot(real)) { throw new ArchiveException("Hard link target resolves outside the extraction root: %s -> %s", leaf, linkTarget); } Files.createLink(leaf, real); @@ -379,7 +403,7 @@ void writeSymbolicLink(final Path leaf, final String linkTarget) throws IOExcept if (symlinkPolicy == SymlinkPolicy.ALLOW_WITHIN_ROOT) { final Path parent = leaf.getParent(); final Path resolved = (parent != null ? parent : rootDirectory).resolve(linkTarget).normalize(); - if (!resolved.startsWith(rootDirectory)) { + if (!isWithinRoot(resolved)) { throw new ArchiveException("Symbolic link target escapes the extraction root: %s -> %s", leaf, linkTarget); } } diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java b/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java index 76ec4904b45..f1102a4e88a 100644 --- a/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/SymlinkPolicy.java @@ -19,7 +19,9 @@ package org.apache.commons.compress.archivers.extractor; /** - * Controls how symbolic-link entries in an archive are handled during extraction. + * Controls how symbolic-link entries in an archive are handled during extraction. For untrusted archives, {@link #REJECT} + * (the default) and {@link #SKIP} are the safe choices; {@link #ALLOW_WITHIN_ROOT} enforces only a best-effort lexical + * containment check, as described on that constant. * * @since 1.29.0 */ @@ -37,8 +39,13 @@ public enum SymlinkPolicy { SKIP, /** - * Create a symbolic link only if its target resolves to a location inside the extraction root; reject a link whose - * target escapes the root. A link created this way is never followed when later entries are written. + * Create a symbolic link only when its target lexically resolves inside the extraction root, and reject a link whose + * target escapes. This containment check is best-effort: it is evaluated lexically against the archive-declared target at + * creation time, so it does not prevent a target reached through another symbolic link, nor a chain of individually + * in-root links that escapes only once the operating system resolves them together. Extraction itself never follows these + * links (every path component is resolved with {@link java.nio.file.LinkOption#NOFOLLOW_LINKS}), so nothing is written + * outside the root while extracting; the residual risk is a created link that a later consumer follows. For untrusted + * archives prefer {@link #REJECT} (the default) or {@link #SKIP}. */ ALLOW_WITHIN_ROOT, diff --git a/src/site/xdoc/examples.xml b/src/site/xdoc/examples.xml index 5ac537fc222..878cb316370 100644 --- a/src/site/xdoc/examples.xml +++ b/src/site/xdoc/examples.xml @@ -224,6 +224,79 @@ try (InputStream fi = Files.newInputStream(Paths.get("my.tar.gz")); + +

The skeleton above trusts the archive: it follows symbolic + links and can be raced by a concurrent attacker. Starting with + Compress 1.29 the + org.apache.commons.compress.archivers.extractor.Extractor + class materializes a whole archive into a target directory + safely, resisting both symlink-slip from the archive and + concurrent symlink races.

+ +

Obtain one with Extractor.newExtractor(Path), + which returns the strongest implementation the platform + supports: a race-safe extractor backed by + java.nio.file.SecureDirectoryStream where the file + system provides one (Linux), otherwise a best-effort extractor + that still resists symlink-slip from the archive but cannot + fully close a concurrent time-of-check-to-time-of-use race.

+ + i = ... create the stream for your format, use buffering...) { + Extractor.newExtractor(targetDir).extract(i); +} +]]> + +

Defaults are conservative: symbolic-link entries are + rejected, special entries such as devices and FIFOs are + skipped, and existing files are not overwritten. The handling + of symbolic links is controlled by SymlinkPolicy:

+ + + + + + + + + + + + + + + + + + + + + + + + + + +
PolicyBehavior
REJECT (default)Fail if the archive contains any symbolic-link entry. Safest for untrusted archives.
SKIPSilently ignore symbolic-link entries.
ALLOW_WITHIN_ROOTCreate a symbolic link only when its target lexically resolves inside the root. Best-effort, see the caveat below.
ALLOW_ALLCreate symbolic links verbatim. Only for fully trusted archives.
+ + + +

ALLOW_WITHIN_ROOT is best-effort: the + containment test is lexical and evaluated at creation time, so + it cannot prevent a link reached through another link, nor a + chain of individually in-root links that escapes only once the + operating system resolves them together. Extraction itself + never follows these links, so nothing is written outside the + root while extracting; the residual risk is a created link that + a later consumer follows. For untrusted archives use + REJECT (the default) or SKIP.

+ +

Apart from 7z all formats that support writing provide a subclass of ArchiveOutputStream that can be used diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java index 16ce7d45f5e..e76222b4bb7 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java @@ -18,19 +18,29 @@ */ package org.apache.commons.compress.archivers.extractor; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.dir; import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.hardlink; +import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.symlink; import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.BasicFileAttributes; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; /** * Root-boundary invariant: {@code newExtractor} canonicalizes the target with {@code toRealPath}, so the containment guard is @@ -69,4 +79,100 @@ void canonicalizesSymlinkedRootAndStaysContained() throws Exception { assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); assertFalse(Files.exists(target.resolve("escape"))); } + + @ParameterizedTest + @ValueSource(strings = { "sub/..", "a/b/../..", "a/b/c/../../.." }) + void fileResolvingToRootIsSkipped(final String name) throws Exception { + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file(name, "x"))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void directoryResolvingToRootIsSkipped() throws Exception { + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(dir("sub/.."))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void symlinkResolvingToRootIsSkipped() throws Exception { + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + Fixtures.extractTar(extractor, Fixtures.tar(symlink("sub/..", "irrelevant"))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void siblingSharingRootNamePrefixIsRejected() throws Exception { + final Path sibling = target.resolveSibling(target.getFileName().toString() + "-sibling"); + final String escape = "../" + sibling.getFileName().toString() + "/pwned"; + final Extractor extractor = Extractor.newExtractor(target); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file(escape, "x")))); + assertFalse(Files.exists(sibling.resolve("pwned"))); + } + + @Test + void pathWithParentSegmentStayingInsideRootIsWritten() throws Exception { + // The skip fires only on an exact-root resolution; a name that merely contains ".." but normalizes to a location + // inside the root is still written there. + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("a/b/../c", "hi"))); + assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/c"))); + assertFalse(Files.exists(target.resolve("a/b"))); + } + + @Test + void rootResolvingEntryIsSkippedWithoutAbortingTheArchive() throws Exception { + // Skipping a root-resolving entry is per-entry, not fatal, so later entries still extract. + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("x/..", "ignored"), file("keep.txt", "hi"))); + assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("keep.txt"))); + } + + @Test + void hardlinkResolvingToRootIsSkipped() throws Exception { + // A hard link entry has no policy gate, so the root-resolution skip is what keeps its name from targeting the root. + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(hardlink("sub/..", "whatever"))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void fileResolvingToRootIsSkippedViaZip() throws Exception { + // Same skip, exercised through the zip path for format parity with the tar cases above. + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + Fixtures.extractZip(Extractor.newExtractor(target), Fixtures.zip(file("sub/..", "x"))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void currentDirectoryEntryIsSkipped() throws Exception { + // A "." entry is common in real tar archives and resolves to the root, so it is skipped rather than failing the + // extraction or targeting the root. + final Object rootKey = Files.readAttributes(target, BasicFileAttributes.class).fileKey(); + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(dir("."))); + assertTrue(Files.isDirectory(target)); + assertEquals(rootKey, Files.readAttributes(target, BasicFileAttributes.class).fileKey()); + } + + @Test + void dotSegmentMidPathIsWritten() throws Exception { + // A "." segment in the middle of a name normalizes away; the entry is still written at its real in-root location. + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("a/./b", "hi"))); + assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a/b"))); + } + + @Test + void backslashInNameIsLiteralNotSeparatorOnPosix() throws Exception { + // On POSIX a backslash is an ordinary filename character, not a separator, so a name that would traverse on Windows + // stays a single in-root component here. Guards against a future change that splits on backslash. + assumeTrue(File.separatorChar == '/'); + Fixtures.extractTar(Extractor.newExtractor(target), Fixtures.tar(file("a\\..\\..\\etc", "x"))); + assertTrue(Files.exists(target.resolve("a\\..\\..\\etc"))); + assertFalse(Files.exists(target.getParent().resolve("etc"))); + } } diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java index 235f4704f20..f2b1fa90925 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorSymlinkPolicyTest.java @@ -34,6 +34,8 @@ import org.apache.commons.compress.archivers.zip.ZipFile; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; /** * Symbolic-link policy behavior. The archive itself is trusted; these prove that the extractor controls link creation and @@ -116,4 +118,84 @@ void allowWithinRootCreatesZipSymlinkViaZipFile() throws Exception { assertTrue(Files.isSymbolicLink(link)); assertEquals(link.getFileSystem().getPath("../b"), Files.readSymbolicLink(link)); } + + private static final int POC_DEPTH = 20; + + private static String repeat(final String component, final int times) { + final StringBuilder sb = new StringBuilder(component); + for (int i = 1; i < times; i++) { + sb.append('/').append(component); + } + return sb.toString(); + } + + /** + * Builds the indirect (chained) symbolic-link escape from COMPRESS-727: {@code malicious-entry} is written first with a + * target that is lexically inside the root, while the deeper link it passes through only escapes once the operating system + * resolves the chain. Each link is individually within-root, which is exactly what {@link SymlinkPolicy#ALLOW_WITHIN_ROOT} + * cannot see through. + */ + private static byte[] indirectEscapeZip() throws IOException { + final String nested = repeat("a", POC_DEPTH); + final String maliciousTarget = nested + "/" + repeat("..", POC_DEPTH) + "/etc/passwd"; + final String nestedTarget = repeat("..", POC_DEPTH - 1); + return Fixtures.zip(symlink("malicious-entry", maliciousTarget), symlink(nested, nestedTarget)); + } + + @Test + void indirectSymlinkEscapeRejectedByDefault() throws Exception { + final Extractor extractor = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(indirectEscapeZip())) { + assertThrows(IOException.class, () -> extractor.extract(zip)); + } + assertFalse(Files.exists(target.resolve("malicious-entry"), LinkOption.NOFOLLOW_LINKS)); + } + + @Test + void indirectSymlinkEscapeSkipped() throws Exception { + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.SKIP); + try (ZipFile zip = Fixtures.openZip(indirectEscapeZip())) { + extractor.extract(zip); + } + assertFalse(Files.exists(target.resolve("malicious-entry"), LinkOption.NOFOLLOW_LINKS)); + } + + @ParameterizedTest + @EnumSource(names = { "ALLOW_WITHIN_ROOT", "ALLOW_ALL" }) + void indirectSymlinkEscapeCreatesInRootLinksButWritesNothingOutside(final SymlinkPolicy policy) throws Exception { + final String nested = repeat("a", POC_DEPTH); + final String maliciousTarget = nested + "/" + repeat("..", POC_DEPTH) + "/etc/passwd"; + final String nestedTarget = repeat("..", POC_DEPTH - 1); + final byte[] data = Fixtures.zip(symlink("malicious-entry", maliciousTarget), symlink(nested, nestedTarget)); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(policy); + try (ZipFile zip = Fixtures.openZip(data)) { + extractor.extract(zip); + } + final Path malicious = target.resolve("malicious-entry"); + final Path deep = target.resolve(nested); + // Each link passes the per-link lexical containment check, so both are created inside the root. This is the + // documented best-effort limitation: the escape only occurs if a later consumer follows the chain, never during + // extraction, which resolves no links. + assertTrue(Files.isSymbolicLink(malicious)); + assertTrue(Files.isSymbolicLink(deep)); + // The crafted targets are stored verbatim (the fixture is not silently neutered) and only links, no file content, were + // written, so extraction produced nothing outside the root. + assertEquals(malicious.getFileSystem().getPath(maliciousTarget), Files.readSymbolicLink(malicious)); + assertEquals(deep.getFileSystem().getPath(nestedTarget), Files.readSymbolicLink(deep)); + } + + @Test + void legitSymlinkChainWithinRootIsCreated() throws Exception { + // Positive control for the chain the escape test attacks: when every hop stays inside the root the whole chain is + // created, so ALLOW_WITHIN_ROOT is not over-rejecting legitimate links. + final byte[] data = Fixtures.zip(dir("b"), symlink("a", "b"), symlink("c", "a")); + final Extractor extractor = Extractor.newExtractor(target).setSymlinkPolicy(SymlinkPolicy.ALLOW_WITHIN_ROOT); + try (ZipFile zip = Fixtures.openZip(data)) { + extractor.extract(zip); + } + assertTrue(Files.isSymbolicLink(target.resolve("a"))); + assertTrue(Files.isSymbolicLink(target.resolve("c"))); + assertEquals(target.getFileSystem().getPath("b"), Files.readSymbolicLink(target.resolve("a"))); + assertEquals(target.getFileSystem().getPath("a"), Files.readSymbolicLink(target.resolve("c"))); + } } From d34af87c49a1ea280563b0a276ebadbe92c1107a Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Mon, 13 Jul 2026 12:44:33 -0300 Subject: [PATCH 4/6] [COMPRESS-727] Harden ensureDirectory and cover parent-component targets Address review feedback from Marcono1234. ensureDirectory now catches IOException and confirms non-existence with Files.notExists(NOFOLLOW_LINKS) before creating a component, instead of relying on the undocumented NoSuchFileException from readAttributes. Real errors are rethrown rather than masked as a missing directory. Add a test for a target spelled with a parent (..) component, anchored to an in-root subdirectory so it canonicalizes back to the temp target. Note the Windows dots-and-spaces name aliasing as a known corner case; the entry stays contained and the CREATE_NEW write fails closed. --- .../compress/archivers/extractor/Extractor.java | 14 +++++++++++--- .../extractor/ExtractorRootBoundaryTest.java | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java index e32c48ba33e..d7d0fe9a00c 100644 --- a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java @@ -237,6 +237,9 @@ private Path resolveWithinRoot(final String name) throws ArchiveException { if (resolved.equals(rootDirectory)) { return null; } + // A component of only dots or spaces (for example "...") is stripped by Windows and can alias the root, which this + // lexical equality does not catch. The entry still stays within the root and its no-follow CREATE_NEW write fails + // closed rather than replacing the root, so this is a known corner case rather than a boundary escape. if (!isWithinRoot(resolved)) { throw new ArchiveException("Entry '%s' would escape the extraction root", name); } @@ -258,9 +261,14 @@ void ensureDirectory(final Path directory) throws IOException { final BasicFileAttributes attrs; try { attrs = Files.readAttributes(current, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS); - } catch (final NoSuchFileException e) { - Files.createDirectory(current); - continue; + } catch (final IOException e) { + // readAttributes documents only IOException, and NoSuchFileException is an optional specific exception, so + // confirm the component is genuinely missing before creating it and rethrow any other I/O error. + if (e instanceof NoSuchFileException || Files.notExists(current, LinkOption.NOFOLLOW_LINKS)) { + Files.createDirectory(current); + continue; + } + throw e; } if (attrs.isSymbolicLink()) { throw new ArchiveException("Refusing to traverse symbolic link: %s", current); diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java index e76222b4bb7..49b0f6962fb 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java @@ -80,6 +80,20 @@ void canonicalizesSymlinkedRootAndStaysContained() throws Exception { assertFalse(Files.exists(target.resolve("escape"))); } + @Test + void canonicalizesTargetWithParentComponentAndStaysContained() throws Exception { + // A ".."-spelled target must behave like the "." and trailing-separator cases. A bare relative Path.of("..") is + // avoided on purpose: it would resolve to the parent of the process working directory and extract outside the + // @TempDir, so the ".." is anchored to a real in-root subdirectory that resolves back to the canonical target. + final Path sub = Files.createDirectory(target.resolve("sub")); + final Path viaParent = Paths.get(sub.toString(), ".."); + Fixtures.extractTar(Extractor.newExtractor(viaParent), Fixtures.tar(file("a.txt", "hi"))); + assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); + final Extractor extractor = Extractor.newExtractor(viaParent); + assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); + assertFalse(Files.exists(target.getParent().resolve("escape"))); + } + @ParameterizedTest @ValueSource(strings = { "sub/..", "a/b/../..", "a/b/c/../../.." }) void fileResolvingToRootIsSkipped(final String name) throws Exception { From 67e98269fbeaddef81115428d5f38d7490b580fd Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Sun, 9 Aug 2026 14:26:31 -0300 Subject: [PATCH 5/6] [COMPRESS-727] Cover a relative parent target in the root boundary test --- .../extractor/ExtractorRootBoundaryTest.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java index 49b0f6962fb..ca1e401c747 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java @@ -81,17 +81,20 @@ void canonicalizesSymlinkedRootAndStaysContained() throws Exception { } @Test - void canonicalizesTargetWithParentComponentAndStaysContained() throws Exception { - // A ".."-spelled target must behave like the "." and trailing-separator cases. A bare relative Path.of("..") is - // avoided on purpose: it would resolve to the parent of the process working directory and extract outside the - // @TempDir, so the ".." is anchored to a real in-root subdirectory that resolves back to the canonical target. - final Path sub = Files.createDirectory(target.resolve("sub")); - final Path viaParent = Paths.get(sub.toString(), ".."); - Fixtures.extractTar(Extractor.newExtractor(viaParent), Fixtures.tar(file("a.txt", "hi"))); - assertArrayEquals("hi".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("a.txt"))); - final Extractor extractor = Extractor.newExtractor(viaParent); - assertThrows(IOException.class, () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../escape", "x")))); - assertFalse(Files.exists(target.getParent().resolve("escape"))); + void rejectsTraversalWhenTargetIsRelativeParent() throws Exception { + // A bare ".." target is the one spelling a lexical normalize() cannot collapse, so the containment check would + // wrongly accept an escape: "..".resolve("../x").normalize() is "../../x", which still startsWith(".."). The + // toRealPath in newExtractor is what defeats that, and this test fails if it is ever dropped. Only the rejecting + // half is exercised, because a benign entry would land in the parent of the working directory; the entry name is + // distinctive so that an unexpected escape is recognizable instead of looking like a stray file. + final Path relativeParent = Paths.get(".."); + final Path root = relativeParent.toRealPath(); + assumeTrue(root.getParent() != null); + final Path escaped = root.getParent().resolve("commons-compress-escape.txt"); + final Extractor extractor = Extractor.newExtractor(relativeParent); + assertThrows(IOException.class, + () -> Fixtures.extractTar(extractor, Fixtures.tar(file("../commons-compress-escape.txt", "x")))); + assertFalse(Files.exists(escaped)); } @ParameterizedTest From 4d04bcb23416db394fd41b50913eaf117ad5694b Mon Sep 17 00:00:00 2001 From: tomasilluminati Date: Mon, 21 Sep 2026 00:14:47 -0300 Subject: [PATCH 6/6] [COMPRESS-727] Pin the lexical zip-slip guard, add UNC tests resolveWithinRoot decides containment from the name alone. Any file system call before Path#startsWith would touch an attacker-chosen location, and on Windows a UNC name would reach the network before the entry is rejected. The Javadoc now states that invariant. Tests pin it: a host/share name on every platform through ZipFile, ZipArchiveInputStream and tar, Windows-only UNC, drive and drive-relative spellings, and Windows illegal names that fail closed through the InvalidPathException wrapper. They accept only the guard's own ArchiveException, so a FileSystemException fails them. No logic change. --- .../archivers/extractor/Extractor.java | 7 ++ .../extractor/ExtractorMalformedNameTest.java | 20 +++++ .../extractor/ExtractorZipSlipTest.java | 86 ++++++++++++++++++- 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java index d7d0fe9a00c..49da58b0439 100644 --- a/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java +++ b/src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java @@ -219,6 +219,7 @@ final void setBeforeLeafWrite(final Runnable hook) { /** * 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. + * This is a string comparison; it never touches the file system. */ private boolean isWithinRoot(final Path path) { return path.startsWith(rootDirectory); @@ -226,6 +227,12 @@ private boolean isWithinRoot(final Path path) { /** * Resolves {@code name} against the extraction root and applies the lexical zip-slip guard. + *

+ * The guard is lexical by design and must stay free of file system access: {@code name} is untrusted until + * {@link #isWithinRoot(Path)} has accepted it, and on Windows a UNC name such as {@code \\host\share} would open a + * connection to {@code host} the moment a {@code Files} method or {@link Path#toRealPath(LinkOption...)} probed it. + * {@code resolve}, {@code normalize}, {@code equals} and {@code startsWith} inspect the path string only. + *

* * @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 diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java index 1ae8da9143b..5b6b50585d8 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorMalformedNameTest.java @@ -33,7 +33,11 @@ import org.apache.commons.compress.archivers.ArchiveException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; /** * A malformed entry name that cannot be turned into a {@link Path} on this platform (a zip name field preserves a NUL byte, @@ -74,4 +78,20 @@ void wellFormedNameStillExtracts() throws Exception { Fixtures.extractZip(Extractor.newExtractor(target), Fixtures.zip(file("ok.txt", "fine"))); assertArrayEquals("fine".getBytes(StandardCharsets.UTF_8), Files.readAllBytes(target.resolve("ok.txt"))); } + + @ParameterizedTest + @ValueSource(strings = { "evil.txt:ads", "evil*.txt", "evil?.txt", ".txt", "evil|.txt", "evil\".txt", "\\\\localhost" }) + @EnabledOnOs(OS.WINDOWS) + void illegalWindowsNameFailsClosedAsArchiveException(final String name) throws Exception { + // Windows rejects these spellings with an InvalidPathException (a reserved character, or a UNC name without a share); + // the same chokepoint turns it into the fail-closed ArchiveException the NUL case gets, with nothing left behind. + final byte[] data = Fixtures.zip(file(name, "x")); + final Extractor extractor = Extractor.newExtractor(target); + final IOException ex = assertThrows(IOException.class, () -> Fixtures.extractZip(extractor, data)); + assertInstanceOf(ArchiveException.class, ex); + assertInstanceOf(InvalidPathException.class, ex.getCause()); + try (Stream entries = Files.list(target)) { + assertEquals(0L, entries.count(), "a rejected malformed entry must not leave anything behind"); + } + } } diff --git a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java index 546d96fe6c4..a0d8671f642 100644 --- a/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorZipSlipTest.java @@ -20,28 +20,58 @@ import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.file; import static org.apache.commons.compress.archivers.extractor.Fixtures.Entry.rawFile; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.stream.Stream; +import org.apache.commons.compress.archivers.ArchiveException; import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; +import org.apache.commons.compress.archivers.zip.ZipFile; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.function.Executable; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; /** * Zip-slip regression: entry names that escape the target via {@code ..} or an absolute path must be rejected, preserving the - * lexical guard that {@code ArchiveEntry.resolveIn} provides today. + * lexical guard that {@code ArchiveEntry.resolveIn} provides today. The guard decides containment from the name alone, so a UNC + * or drive name on Windows has to fail with the guard's own exception and never with a {@code FileSystemException} from an API + * that touched the name first. */ class ExtractorZipSlipTest { + /** A UNC path on Windows and an absolute path everywhere else; the guard rejects it lexically on both. */ + private static final String HOST_SHARE_NAME = "//localhost/invalid/evil.txt"; + @TempDir Path target; + /** + * Asserts that the extraction fails with the zip-slip guard's own exception and leaves the target empty. A + * {@code FileSystemException} here ("The network name cannot be found" for a UNC name) would mean an API touched the name + * before containment was decided. + */ + private void assertRejectedByGuard(final Executable extraction) throws IOException { + final ArchiveException ex = assertThrows(ArchiveException.class, extraction); + assertTrue(ex.getMessage().contains("would escape the extraction root"), () -> "expected the zip-slip guard, got: " + ex); + assertNull(ex.getCause(), () -> "the zip-slip guard throws without a cause, got: " + ex.getCause()); + try (Stream entries = Files.list(target)) { + assertEquals(0L, entries.count(), "a rejected entry must not leave anything behind"); + } + } + @Test void rejectsAbsoluteName() throws Exception { final byte[] data = Fixtures.tar(rawFile("/tmp/compress-extractor-evil", "x")); @@ -61,4 +91,58 @@ void rejectsParentTraversal() throws Exception { } assertFalse(Files.exists(target.getParent().resolve("compress-extractor-evil.txt"))); } + + @Test + void rejectsHostShareNameViaZipFile() throws Exception { + final byte[] data = Fixtures.zip(file(HOST_SHARE_NAME, "x")); + final Extractor extractor = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(data)) { + assertRejectedByGuard(() -> extractor.extract(zip)); + } + } + + @Test + void rejectsHostShareNameViaZipStream() throws Exception { + final byte[] data = Fixtures.zip(file(HOST_SHARE_NAME, "x")); + final Extractor extractor = Extractor.newExtractor(target); + assertRejectedByGuard(() -> Fixtures.extractZip(extractor, data)); + } + + @Test + void rejectsHostShareNameViaTar() throws Exception { + final byte[] data = Fixtures.tar(rawFile(HOST_SHARE_NAME, "x")); + final Extractor extractor = Extractor.newExtractor(target); + assertRejectedByGuard(() -> Fixtures.extractTar(extractor, data)); + } + + @ParameterizedTest + @ValueSource(strings = { "\\\\localhost\\invalid", "\\\\localhost\\invalid\\evil.txt", "\\\\localhost\\invalid/evil.txt", + "//localhost/invalid/evil.txt", "C:\\Windows\\evil.txt", "C:/Windows/evil.txt" }) + @EnabledOnOs(OS.WINDOWS) + void rejectsUncOrDriveNameOnWindows(final String name) throws Exception { + // Each spelling is a UNC or absolute drive path to WindowsPathParser, so the guard rejects it from the name alone. A + // FileSystemException ("The network name cannot be found") would mean the name reached the file system, and possibly + // a remote host, before containment was decided. The zip layer folds a backslash-only name to forward slashes, which + // Windows reads as UNC just the same. + final byte[] data = Fixtures.zip(file(name, "x")); + final Extractor extractor = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(data)) { + assertRejectedByGuard(() -> extractor.extract(zip)); + } + assertRejectedByGuard(() -> Fixtures.extractZip(Extractor.newExtractor(target), data)); + } + + @Test + @EnabledOnOs(OS.WINDOWS) + void rejectsDriveRelativeNameOnAnotherDriveOnWindows() throws Exception { + // "Q:evil.txt" is relative to the current directory of drive Q:, which only the file system (GetFullPathName) can + // supply. The guard rejects it from the spelling alone, so that drive is never consulted. + final char rootDrive = Character.toUpperCase(target.toRealPath().getRoot().toString().charAt(0)); + final String name = (rootDrive == 'Q' ? "R" : "Q") + ":evil.txt"; + final byte[] data = Fixtures.zip(file(name, "x")); + final Extractor extractor = Extractor.newExtractor(target); + try (ZipFile zip = Fixtures.openZip(data)) { + assertRejectedByGuard(() -> extractor.extract(zip)); + } + } }