Marcono1234 commented on code in PR #776: URL: https://github.com/apache/commons-compress/pull/776#discussion_r3504668028
########## 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: `Files#readAttributes` does not document that it can throw `NoSuchFileException`, and in general `NoSuchFileException` is an "optional specific exception" and might not be reported by all file system implementations (?). So this seems to rely on implementation details. Maybe this should do `catch (IOException e)` and then when `if (e instanceof NoSuchFileException || Files.notExists(current, LinkOption.NOFOLLOW_LINKS)) { ... create directory }` and otherwise rethrow `e`? -- 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]
