Marcono1234 commented on code in PR #776: URL: https://github.com/apache/commons-compress/pull/776#discussion_r3537266983
########## src/test/java/org/apache/commons/compress/archivers/extractor/ExtractorRootBoundaryTest.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.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 + * 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 { Review Comment: For completeness maybe this needs also a test where `target = Path.of("..")`. If Extractor did not call `toRealPath`, then such a target path would render escape detection ineffective because `normalize` would not remove `..` entries from the path. This is similar to the `toAbsolutePath().normalize()` concerns which you had already mentioned before and which were mentioned in the Commons IO PR; though the use of `toRealPath` here solves this already. (Not sure if this is actually covered already by the tests and I just overlooked it.) ########## src/main/java/org/apache/commons/compress/archivers/extractor/Extractor.java: ########## @@ -217,11 +217,27 @@ final void setBeforeLeafWrite(final Runnable hook) { } /** - * Resolves {@code name} against the extraction root and rejects any result that escapes it (the lexical zip-slip guard). + * Tests whether {@code path} is contained within the canonical extraction root, comparing component by component so a + * sibling that merely shares a name prefix (for example {@code root-old} beside {@code root}) is not treated as contained. + */ + private boolean isWithinRoot(final Path path) { + return path.startsWith(rootDirectory); + } + + /** + * Resolves {@code name} against the extraction root and applies the lexical zip-slip guard. + * + * @return the resolved path within the root, or {@code null} if {@code name} resolves to the root itself (for example + * {@code a/..}), which carries nothing to materialize; the caller skips such entries rather than writing at or + * replacing the root. + * @throws ArchiveException if the resolved path escapes the extraction root. */ private Path resolveWithinRoot(final String name) throws ArchiveException { final Path resolved = rootDirectory.resolve(name).normalize(); - if (!resolved.startsWith(rootDirectory)) { + if (resolved.equals(rootDirectory)) { Review Comment: On Windows it seems this fails to detect to file names consisting only of spaces and dots, see https://github.com/apache/commons-compress/pull/776#discussion_r3507869476. The paths are not equal but actually refer to the same directory. Not sure if it is worth accounting for that corner case. It seems `Files.isSameFile(resolved, rootDirectory)` could be used as fallback, but it probably has overhead. (Side note: I am planning to submit this as OpenJDK bug as well; but it does not help users with older Java versions.) ########## 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. + * <p> + * 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. + * </p> + * <p> + * 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. + * </p> + * + * @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<Path> 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}. + * <p> + * 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. + * </p> + * + * @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<ZipArchiveEntry> 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; + } Review Comment: Did you resolve this comment about `Files#readAttributes` intentionally? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
