Projects
openEuler:22.03:LTS:SP1
apache-sshd
_service:tar_scm_kernel_repo:CVE-2023-35887.patch
Sign Up
Log In
Username
Password
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File _service:tar_scm_kernel_repo:CVE-2023-35887.patch of Package apache-sshd
From c20739b43aab0f7bf2ccad982a6cb37b9d5a8a0b Mon Sep 17 00:00:00 2001 From: Guillaume Nodet <gnodet@gmail.com> Date: Tue, 9 May 2023 15:17:26 +0200 Subject: [PATCH] [SSHD-1234] Rooted file system can leak informations --- .../file/root/RootedDirectoryStream.java | 63 +++ .../common/file/root/RootedFileSystem.java | 5 + .../file/root/RootedFileSystemProvider.java | 327 ++++++++---- .../file/root/RootedFileSystemUtils.java | 55 ++ .../root/RootedSecureDirectoryStream.java | 92 ++++ .../sshd/common/file/util/BaseFileSystem.java | 23 +- .../apache/sshd/common/util/io/IoUtils.java | 160 +++++- .../root/RootedFileSystemProviderTest.java | 469 ++++++++++++++---- .../sshd/common/util/io/IoUtilsTest.java | 48 ++ .../util/test/CommonTestSupportUtils.java | 4 +- .../client/fs/SftpFileSystemProvider.java | 4 +- .../server/AbstractSftpSubsystemHelper.java | 101 +++- .../sftp/server/SftpFileSystemAccessor.java | 94 +++- .../sshd/sftp/server/SftpSubsystem.java | 8 +- .../org/apache/sshd/sftp/client/SftpTest.java | 7 +- .../sshd/sftp/client/SftpVersionsTest.java | 12 +- 16 files changed, 1203 insertions(+), 269 deletions(-) create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedDirectoryStream.java create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemUtils.java create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedDirectoryStream.java b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedDirectoryStream.java new file mode 100644 index 000000000..dde9e748d --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedDirectoryStream.java @@ -0,0 +1,63 @@ +/* + * 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 + * + * http://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.sshd.common.file.root; + +import java.io.IOException; +import java.nio.file.DirectoryStream; +import java.nio.file.Path; +import java.util.Iterator; + +/** + * secure directory stream proxy for a {@link RootedFileSystem} + * + * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a> + */ +public class RootedDirectoryStream implements DirectoryStream<Path> { + protected final RootedFileSystem rfs; + protected final DirectoryStream<Path> delegate; + + public RootedDirectoryStream(RootedFileSystem rfs, DirectoryStream<Path> delegate) { + this.rfs = rfs; + this.delegate = delegate; + } + + @Override + public Iterator<Path> iterator() { + return root(rfs, delegate.iterator()); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + + protected Iterator<Path> root(RootedFileSystem rfs, Iterator<Path> iter) { + return new Iterator<Path>() { + @Override + public boolean hasNext() { + return iter.hasNext(); + } + + @Override + public Path next() { + return rfs.provider().root(rfs, iter.next()); + } + }; + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystem.java b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystem.java index 1b9a71ca1..efd0b134d 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystem.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystem.java @@ -94,6 +94,11 @@ public Iterable<FileStore> getFileStores() { return rootFs.getFileStores(); } + @Override + protected boolean hostFsHasWindowsSeparator() { + return "\\".equals(getRoot().getFileSystem().getSeparator()); + } + @Override public String toString() { return rootPath.toString(); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java index 0855c7751..9922582b1 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemProvider.java @@ -18,6 +18,7 @@ */ package org.apache.sshd.common.file.root; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -26,32 +27,40 @@ import java.nio.channels.AsynchronousFileChannel; import java.nio.channels.FileChannel; import java.nio.channels.SeekableByteChannel; +import java.nio.file.AccessDeniedException; import java.nio.file.AccessMode; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.CopyOption; +import java.nio.file.DirectoryNotEmptyException; import java.nio.file.DirectoryStream; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileStore; import java.nio.file.FileSystem; import java.nio.file.FileSystemAlreadyExistsException; +import java.nio.file.FileSystemException; +import java.nio.file.FileSystemLoopException; import java.nio.file.FileSystemNotFoundException; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.LinkOption; +import java.nio.file.NoSuchFileException; +import java.nio.file.NotDirectoryException; +import java.nio.file.NotLinkException; import java.nio.file.OpenOption; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.ProviderMismatchException; +import java.nio.file.SecureDirectoryStream; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.FileAttributeView; import java.nio.file.spi.FileSystemProvider; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.ExecutorService; -import org.apache.sshd.common.util.ValidateUtils; import org.apache.sshd.common.util.io.IoUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -157,14 +166,22 @@ public Path getPath(URI uri) { public InputStream newInputStream(Path path, OpenOption... options) throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.newInputStream(r, options); + try { + return p.newInputStream(r, options); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public OutputStream newOutputStream(Path path, OpenOption... options) throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.newOutputStream(r, options); + try { + return p.newOutputStream(r, options); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override @@ -172,7 +189,11 @@ public FileChannel newFileChannel(Path path, Set<? extends OpenOption> options, throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.newFileChannel(r, options, attrs); + try { + return p.newFileChannel(r, options, attrs); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override @@ -181,7 +202,11 @@ public AsynchronousFileChannel newAsynchronousFileChannel( throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.newAsynchronousFileChannel(r, options, executor, attrs); + try { + return p.newAsynchronousFileChannel(r, options, executor, attrs); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override @@ -189,82 +214,74 @@ public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> o throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.newByteChannel(r, options, attrs); + try { + return p.newByteChannel(r, options, attrs); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public DirectoryStream<Path> newDirectoryStream(Path dir, DirectoryStream.Filter<? super Path> filter) throws IOException { Path r = unroot(dir); FileSystemProvider p = provider(r); - return root(((RootedPath) dir).getFileSystem(), p.newDirectoryStream(r, filter)); + try { + return root(((RootedPath) dir).getFileSystem(), p.newDirectoryStream(r, filter)); + } catch (IOException ex) { + throw translateIoException(ex, dir); + } } protected DirectoryStream<Path> root(RootedFileSystem rfs, DirectoryStream<Path> ds) { - return new DirectoryStream<Path>() { - @Override - public Iterator<Path> iterator() { - return root(rfs, ds.iterator()); - } - - @Override - public void close() throws IOException { - ds.close(); - } - }; - } - - protected Iterator<Path> root(RootedFileSystem rfs, Iterator<Path> iter) { - return new Iterator<Path>() { - @Override - public boolean hasNext() { - return iter.hasNext(); - } - - @Override - public Path next() { - return root(rfs, iter.next()); - } - }; + if (ds instanceof SecureDirectoryStream) { + return new RootedSecureDirectoryStream(rfs, (SecureDirectoryStream<Path>) ds); + } + return new RootedDirectoryStream(rfs, ds); } @Override public void createDirectory(Path dir, FileAttribute<?>... attrs) throws IOException { Path r = unroot(dir); FileSystemProvider p = provider(r); - p.createDirectory(r, attrs); + try { + p.createDirectory(r, attrs); + } catch (IOException ex) { + throw translateIoException(ex, dir); + } } @Override public void createSymbolicLink(Path link, Path target, FileAttribute<?>... attrs) throws IOException { - createLink(link, target, true, attrs); - } - - @Override - public void createLink(Path link, Path existing) throws IOException { - createLink(link, existing, false); - } - - protected void createLink(Path link, Path target, boolean symLink, FileAttribute<?>... attrs) throws IOException { + // make sure symlink cannot break out of chroot jail. If it is unsafe, simply thrown an exception. This is + // to ensure that symlink semantics are maintained when it is safe, and creation fails when not. + RootedFileSystemUtils.validateSafeRelativeSymlink(target); Path l = unroot(link); - Path t = unroot(target); - /* - * For a symbolic link preserve the relative path - */ - if (symLink && (!target.isAbsolute())) { - RootedFileSystem rfs = ((RootedPath) target).getFileSystem(); - Path root = rfs.getRoot(); - t = root.relativize(t); - } + Path t = target.isAbsolute() ? unroot(target) : l.getFileSystem().getPath(target.toString()); FileSystemProvider p = provider(l); - if (symLink) { + try { p.createSymbolicLink(l, t, attrs); - } else { - p.createLink(l, t); + + if (log.isDebugEnabled()) { + log.debug("createSymbolicLink({} => {}", l, t); + } + } catch (IOException ex) { + throw translateIoException(ex, link); } + } - if (log.isDebugEnabled()) { - log.debug("createLink(symbolic={}) {} => {}", symLink, l, t); + @Override + public void createLink(Path link, Path existing) throws IOException { + Path l = unroot(link); + Path t = unroot(existing); + + try { + provider(l).createLink(l, t); + if (log.isDebugEnabled()) { + log.debug("createLink({} => {}", l, t); + } + } catch (IOException ex) { + throw translateIoException(ex, link); } } @@ -275,7 +292,11 @@ public void delete(Path path) throws IOException { log.trace("delete({}): {}", path, r); } FileSystemProvider p = provider(r); - p.delete(r); + try { + p.delete(r); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override @@ -285,19 +306,27 @@ public boolean deleteIfExists(Path path) throws IOException { log.trace("deleteIfExists({}): {}", path, r); } FileSystemProvider p = provider(r); - return p.deleteIfExists(r); + try { + return p.deleteIfExists(r); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public Path readSymbolicLink(Path link) throws IOException { Path r = unroot(link); FileSystemProvider p = provider(r); - Path t = p.readSymbolicLink(r); - Path target = root((RootedFileSystem) link.getFileSystem(), t); - if (log.isTraceEnabled()) { - log.trace("readSymbolicLink({})[{}]: {}[{}]", link, r, target, t); + try { + Path t = p.readSymbolicLink(r); + Path target = root((RootedFileSystem) link.getFileSystem(), t); + if (log.isTraceEnabled()) { + log.trace("readSymbolicLink({})[{}]: {}[{}]", link, r, target, t); + } + return target; + } catch (IOException ex) { + throw translateIoException(ex, link); } - return target; } @Override @@ -308,7 +337,11 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep log.trace("copy({})[{}]: {}[{}]", source, s, target, t); } FileSystemProvider p = provider(s); - p.copy(s, t, options); + try { + p.copy(s, t, options); + } catch (IOException ex) { + throw translateIoException(ex, source); + } } @Override @@ -319,7 +352,11 @@ public void move(Path source, Path target, CopyOption... options) throws IOExcep log.trace("move({})[{}]: {}[{}]", source, s, target, t); } FileSystemProvider p = provider(s); - p.move(s, t, options); + try { + p.move(s, t, options); + } catch (IOException ex) { + throw translateIoException(ex, source); + } } @Override @@ -327,21 +364,33 @@ public boolean isSameFile(Path path, Path path2) throws IOException { Path r = unroot(path); Path r2 = unroot(path2); FileSystemProvider p = provider(r); - return p.isSameFile(r, r2); + try { + return p.isSameFile(r, r2); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public boolean isHidden(Path path) throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - return p.isHidden(r); + try { + return p.isHidden(r); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public FileStore getFileStore(Path path) throws IOException { RootedFileSystem fileSystem = getFileSystem(path); Path root = fileSystem.getRoot(); - return Files.getFileStore(root); + try { + return Files.getFileStore(root); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } protected RootedFileSystem getFileSystem(Path path) throws FileSystemNotFoundException { @@ -384,7 +433,11 @@ protected RootedFileSystem getFileSystem(Path path) throws FileSystemNotFoundExc public void checkAccess(Path path, AccessMode... modes) throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - p.checkAccess(r, modes); + try { + p.checkAccess(r, modes); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override @@ -403,18 +456,26 @@ public <A extends BasicFileAttributes> A readAttributes(Path path, Class<A> type } FileSystemProvider p = provider(r); - return p.readAttributes(r, type, options); + try { + return p.readAttributes(r, type, options); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } @Override public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) throws IOException { Path r = unroot(path); FileSystemProvider p = provider(r); - Map<String, Object> attrs = p.readAttributes(r, attributes, options); - if (log.isTraceEnabled()) { - log.trace("readAttributes({})[{}] {}: {}", path, r, attributes, attrs); + try { + Map<String, Object> attrs = p.readAttributes(r, attributes, options); + if (log.isTraceEnabled()) { + log.trace("readAttributes({})[{}] {}: {}", path, r, attributes, attrs); + } + return attrs; + } catch (IOException ex) { + throw translateIoException(ex, path); } - return attrs; } @Override @@ -424,7 +485,11 @@ public void setAttribute(Path path, String attribute, Object value, LinkOption.. log.trace("setAttribute({})[{}] {}={}", path, r, attribute, value); } FileSystemProvider p = provider(r); - p.setAttribute(r, attribute, value, options); + try { + p.setAttribute(r, attribute, value, options); + } catch (IOException ex) { + throw translateIoException(ex, path); + } } protected FileSystemProvider provider(Path path) { @@ -434,10 +499,33 @@ protected FileSystemProvider provider(Path path) { protected Path root(RootedFileSystem rfs, Path nat) { if (nat.isAbsolute()) { + // preferred case - this isn't a symlink out of our jail + if (nat.startsWith(rfs.getRoot())) { + // If we have the same number of parts as the root, and start with the root, we must be the root. + if (nat.getNameCount() == rfs.getRoot().getNameCount()) { + return rfs.getPath("/"); + } + + // We are the root, and more. Get the first name past the root because of how getPath works + String firstName = "/" + nat.getName(rfs.getRoot().getNameCount()); + + // the rooted path should have the number of parts past the root + String[] varargs = new String[nat.getNameCount() - rfs.getRoot().getNameCount() - 1]; + int varargsCounter = 0; + for (int i = 1 + rfs.getRoot().getNameCount(); i < nat.getNameCount(); i++) { + varargs[varargsCounter++] = nat.getName(i).toString(); + } + return rfs.getPath(firstName, varargs); + } + + // This is the case where there's a symlink jailbreak, so we return a relative link as the directories above + // the chroot don't make sense to present + // The behavior with the fs class is that we follow the symlink. Note that this is dangerous. Path root = rfs.getRoot(); Path rel = root.relativize(nat); - return rfs.getPath("/" + rel.toString()); + return rfs.getPath("/" + rel); } else { + // For a relative symlink, simply return it as a RootedPath. Note that this may break out of the chroot. return rfs.getPath(nat.toString()); } } @@ -466,39 +554,74 @@ protected Path unroot(Path path) { * @throws InvalidPathException If the resolved path is not a proper sub-path of the rooted file system */ protected Path resolveLocalPath(RootedPath path) { - RootedPath absPath = Objects.requireNonNull(path, "No rooted path to resolve").toAbsolutePath(); - RootedFileSystem rfs = absPath.getFileSystem(); + Objects.requireNonNull(path, "No rooted path to resolve"); + RootedFileSystem rfs = path.getFileSystem(); Path root = rfs.getRoot(); - FileSystem lfs = root.getFileSystem(); + // initialize a list for the new file name parts + Path resolved = IoUtils.chroot(root, path); - String rSep = ValidateUtils.checkNotNullAndNotEmpty(rfs.getSeparator(), "No rooted file system separator"); - ValidateUtils.checkTrue(rSep.length() == 1, "Bad rooted file system separator: %s", rSep); - char rootedSeparator = rSep.charAt(0); + /* + * This can happen for Windows since we represent its paths as /C:/some/path, so substring(1) yields + * C:/some/path - which is resolved as an absolute path (which we don't want). + * + * This also is a security assertion to protect against unknown attempts to break out of the chroot jail + */ + if (!resolved.normalize().startsWith(root)) { + throw new InvalidPathException(root.toString(), "Not under root"); + } + return resolved; + } - String lSep = ValidateUtils.checkNotNullAndNotEmpty(lfs.getSeparator(), "No local file system separator"); - ValidateUtils.checkTrue(lSep.length() == 1, "Bad local file system separator: %s", lSep); - char localSeparator = lSep.charAt(0); + private IOException translateIoException(IOException ex, Path rootedPath) { + // cast is safe as path was unrooted earlier. + RootedPath rootedPathCasted = (RootedPath) rootedPath; + Path root = rootedPathCasted.getFileSystem().getRoot(); + + if (ex instanceof FileSystemException) { + String file = fixExceptionFileName(root, rootedPath, ((FileSystemException) ex).getFile()); + String otherFile = fixExceptionFileName(root, rootedPath, ((FileSystemException) ex).getOtherFile()); + String reason = ((FileSystemException) ex).getReason(); + if (NoSuchFileException.class.equals(ex.getClass())) { + return new NoSuchFileException(file, otherFile, reason); + } else if (FileSystemLoopException.class.equals(ex.getClass())) { + return new FileSystemLoopException(file); + } else if (NotDirectoryException.class.equals(ex.getClass())) { + return new NotDirectoryException(file); + } else if (DirectoryNotEmptyException.class.equals(ex.getClass())) { + return new DirectoryNotEmptyException(file); + } else if (NotLinkException.class.equals(ex.getClass())) { + return new NotLinkException(file); + } else if (AtomicMoveNotSupportedException.class.equals(ex.getClass())) { + return new AtomicMoveNotSupportedException(file, otherFile, reason); + } else if (FileAlreadyExistsException.class.equals(ex.getClass())) { + return new FileAlreadyExistsException(file, otherFile, reason); + } else if (AccessDeniedException.class.equals(ex.getClass())) { + return new AccessDeniedException(file, otherFile, reason); + } + return new FileSystemException(file, otherFile, reason); + } else if (ex.getClass().equals(FileNotFoundException.class)) { + return new FileNotFoundException(ex.getLocalizedMessage().replace(root.toString(), "")); + } + // not sure how to translate, so leave as is. Hopefully does not leak data + return ex; + } - String r = absPath.toString(); - String subPath = r.substring(1); - if (rootedSeparator != localSeparator) { - subPath = subPath.replace(rootedSeparator, localSeparator); + private String fixExceptionFileName(Path root, Path rootedPath, String fileName) { + if (fileName == null) { + return null; } - Path resolved = root.resolve(subPath); - resolved = resolved.normalize(); - resolved = resolved.toAbsolutePath(); - if (log.isTraceEnabled()) { - log.trace("resolveLocalPath({}): {}", absPath, resolved); + Path toFix = root.getFileSystem().getPath(fileName); + if (toFix.getNameCount() == root.getNameCount()) { + // return the root + return rootedPath.getFileSystem().getSeparator(); } - /* - * This can happen for Windows since we represent its paths as /C:/some/path, so substring(1) yields - * C:/some/path - which is resolved as an absolute path (which we don't want). - */ - if (!resolved.startsWith(root)) { - throw new InvalidPathException(r, "Not under root"); + StringBuilder ret = new StringBuilder(); + for (int partNum = root.getNameCount(); partNum < toFix.getNameCount(); partNum++) { + ret.append(rootedPath.getFileSystem().getSeparator()); + ret.append(toFix.getName(partNum++)); } - return resolved; + return ret.toString(); } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemUtils.java b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemUtils.java new file mode 100644 index 000000000..6b0b164e9 --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedFileSystemUtils.java @@ -0,0 +1,55 @@ +/* + * 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 + * + * http://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.sshd.common.file.root; + +import java.nio.file.InvalidPathException; +import java.nio.file.Path; + +/** + * Utility functions for rooted file utils + */ +public final class RootedFileSystemUtils { + + private RootedFileSystemUtils() { + // do not construct + } + + /** + * Validate that the relative path target is safe. This means that at no point in the path can there be more ".." + * than path parts. + * + * @param target the target directory to validate is safe. + */ + public static void validateSafeRelativeSymlink(Path target) { + int numNames = 0; + int numCdUps = 0; + for (int i = 0; i < target.getNameCount(); i++) { + if ("..".equals(target.getName(i).toString())) { + numCdUps++; + } else if (!".".equals(target.getName(i).toString())) { + numNames++; + } + + // need to check at each part to prevent data leakage outside of chroot + if (numCdUps > numNames) { + throw new InvalidPathException(target.toString(), "Symlink would exit chroot: " + target); + } + } + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java new file mode 100644 index 000000000..6fe811c5b --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/root/RootedSecureDirectoryStream.java @@ -0,0 +1,92 @@ +/* + * 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 + * + * http://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.sshd.common.file.root; + +import java.io.IOException; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.LinkOption; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.SecureDirectoryStream; +import java.nio.file.attribute.FileAttribute; +import java.nio.file.attribute.FileAttributeView; +import java.util.Set; + +/** + * A secure directory stream proxy for a {@link RootedFileSystem} + * + * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a> + */ +public class RootedSecureDirectoryStream extends RootedDirectoryStream implements SecureDirectoryStream<Path> { + + public RootedSecureDirectoryStream(RootedFileSystem rfs, SecureDirectoryStream<Path> delegate) { + super(rfs, delegate); + } + + @Override + public SecureDirectoryStream<Path> newDirectoryStream(Path path, LinkOption... options) throws IOException { + return new RootedSecureDirectoryStream(rfs, delegate().newDirectoryStream(fixPath(path), options)); + } + + protected Path fixPath(Path p) { + if (p.isAbsolute()) { + return rfs.provider().unroot(p); + } + + // convert to root fs path. + // Note: this IS able to go below the root directory by design - a way to break out of chroot. + // Be very cautious using this. + return rfs.getRootFileSystem().getPath(p.toString()); + } + + @Override + public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options, FileAttribute<?>... attrs) + throws IOException { + return delegate().newByteChannel(fixPath(path), options, attrs); + } + + @Override + public void deleteFile(Path path) throws IOException { + delegate().deleteFile(fixPath(path)); + } + + @Override + public void deleteDirectory(Path path) throws IOException { + delegate().deleteDirectory(fixPath(path)); + } + + @Override + public void move(Path srcpath, SecureDirectoryStream<Path> targetdir, Path targetpath) throws IOException { + delegate().move(fixPath(srcpath), targetdir, targetpath); + } + + @Override + public <V extends FileAttributeView> V getFileAttributeView(Class<V> type) { + return delegate().getFileAttributeView(type); + } + + @Override + public <V extends FileAttributeView> V getFileAttributeView(Path path, Class<V> type, LinkOption... options) { + return delegate().getFileAttributeView(path, type, options); + } + + private SecureDirectoryStream<Path> delegate() { + return (SecureDirectoryStream<Path>) delegate; + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/file/util/BaseFileSystem.java b/sshd-common/src/main/java/org/apache/sshd/common/file/util/BaseFileSystem.java index a5d099e8c..51b9a03bc 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/file/util/BaseFileSystem.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/file/util/BaseFileSystem.java @@ -34,6 +34,7 @@ import java.util.regex.Pattern; import org.apache.sshd.common.util.GenericUtils; +import org.apache.sshd.common.util.OsUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -79,7 +80,7 @@ public Iterable<FileStore> getFileStores() { public T getPath(String first, String... more) { StringBuilder sb = new StringBuilder(); if (!GenericUtils.isEmpty(first)) { - appendDedupSep(sb, first.replace('\\', '/')); // in case we are running on Windows + appendDedupSep(sb, handleWindowsSeparator(first)); } if (GenericUtils.length(more) > 0) { @@ -87,8 +88,7 @@ public T getPath(String first, String... more) { if ((sb.length() > 0) && (sb.charAt(sb.length() - 1) != '/')) { sb.append('/'); } - // in case we are running on Windows - appendDedupSep(sb, segment.replace('\\', '/')); + appendDedupSep(sb, handleWindowsSeparator(segment)); } } @@ -121,6 +121,23 @@ protected void appendDedupSep(StringBuilder sb, CharSequence s) { } } + /** + * In case we are running on Windows, accept "\\" as a file separator. Ignore in *nix as "\\" is a valid filename + * + * @param name the name to fix the separator for if running on Windows + * @return the fixed name + */ + protected String handleWindowsSeparator(String name) { + if (hostFsHasWindowsSeparator()) { + return name.replace('\\', '/'); + } + return name; + } + + protected boolean hostFsHasWindowsSeparator() { + return OsUtils.isWin32(); + } + @Override public PathMatcher getPathMatcher(String syntaxAndPattern) { int colonIndex = Objects.requireNonNull(syntaxAndPattern, "No argument").indexOf(':'); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java b/sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java index ccac66938..11dd1d92d 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java @@ -47,6 +47,7 @@ import java.util.Collection; import java.util.Collections; import java.util.EnumSet; +import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.Set; @@ -394,18 +395,13 @@ public static String getFileOwner(Path path, LinkOption... options) throws IOExc * explained above */ public static Boolean checkFileExists(Path path, LinkOption... options) { - boolean followLinks = true; - for (LinkOption opt : options) { - if (opt == LinkOption.NOFOLLOW_LINKS) { - followLinks = false; - break; - } - } + boolean followLinks = followLinks(options); + try { if (followLinks) { path.getFileSystem().provider().checkAccess(path); } else { - Files.readAttributes(path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS); + Files.readAttributes(path, BasicFileAttributes.class, options); } return Boolean.TRUE; } catch (NoSuchFileException e) { @@ -415,6 +411,63 @@ public static Boolean checkFileExists(Path path, LinkOption... options) { } } + /** + * Checks that a file exists with or without following any symlinks. + * + * @param path the path to check + * @param neverFollowSymlinks whether to follow symlinks + * @return true if the file exists with the symlink semantics, false if it doesn't exist, null + * if symlinks were found, or it is unknown if whether the file exists + */ + public static Boolean checkFileExistsAnySymlinks(Path path, boolean neverFollowSymlinks) { + try { + if (!neverFollowSymlinks) { + path.getFileSystem().provider().checkAccess(path); + } else { + // this is a bad fix because this leaves a nasty race condition - the directory may turn into a symlink + // between this check and the call to open() + for (int i = 1; i <= path.getNameCount(); i++) { + Path checkForSymLink = getFirstPartsOfPath(path, i); + BasicFileAttributes basicFileAttributes + = Files.readAttributes(checkForSymLink, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS); + if (basicFileAttributes.isSymbolicLink()) { + return false; + } + } + } + return true; + } catch (NoSuchFileException e) { + return false; + } catch (IOException e) { + return null; + } + } + + /** + * Extracts the first n parts of the path. For example <br> + * ("/home/test/test12", 1) returns "/home", <br> + * ("/home/test", 1) returns "/home/test" <br> + * etc. + * + * @param path the path to extract parts of + * @param partsToExtract the number of parts to extract + * @return the extracted path + */ + public static Path getFirstPartsOfPath(Path path, int partsToExtract) { + String firstName = path.getName(0).toString(); + String[] names = new String[partsToExtract - 1]; + for (int j = 1; j < partsToExtract; j++) { + names[j - 1] = path.getName(j).toString(); + } + Path checkForSymLink = path.getFileSystem().getPath(firstName, names); + // the root is not counted as a directory part so we must resolve the result relative to it. + Path root = path.getRoot(); + if (root != null) { + checkForSymLink = root.resolve(checkForSymLink); + } + return checkForSymLink; + } + /** * Read the requested number of bytes or fail if there are not enough left. * @@ -637,4 +690,95 @@ public static List<String> readAllLines(BufferedReader reader, int lineCountHint } return result; } + + /** + * Chroot a path under the new root + * + * @param newRoot the new root + * @param toSanitize the path to sanitize and chroot + * @return the chrooted path under the newRoot filesystem + */ + public static Path chroot(Path newRoot, Path toSanitize) { + Objects.requireNonNull(newRoot); + Objects.requireNonNull(toSanitize); + List<String> sanitized = removeExtraCdUps(toSanitize); + return buildPath(newRoot, newRoot.getFileSystem(), sanitized); + } + + /** + * Remove any extra directory ups from the Path + * + * @param toSanitize the path to sanitize + * @return the sanitized path + */ + public static Path removeCdUpAboveRoot(Path toSanitize) { + List<String> sanitized = removeExtraCdUps(toSanitize); + return buildPath(toSanitize.getRoot(), toSanitize.getFileSystem(), sanitized); + } + + private static List<String> removeExtraCdUps(Path toResolve) { + List<String> newNames = new ArrayList<>(toResolve.getNameCount()); + + int numCdUps = 0; + int numDirParts = 0; + for (int i = 0; i < toResolve.getNameCount(); i++) { + String name = toResolve.getName(i).toString(); + if ("..".equals(name)) { + // If we have more cdups than dir parts, so we ignore the ".." to avoid jail escapes + if (numDirParts > numCdUps) { + ++numCdUps; + newNames.add(name); + } + } else { + // if the current directory is a part of the name, don't increment number of dir parts, as it doesn't + // add to the number of ".."s that can be present before the root + if (!".".equals(name)) { + ++numDirParts; + } + newNames.add(name); + } + } + return newNames; + } + + /** + * Build a path from the list of path parts + * + * @param root the root path + * @param fs the filesystem + * @param namesList the parts of the path to build + * @return the built path + */ + public static Path buildPath(Path root, FileSystem fs, List<String> namesList) { + Objects.requireNonNull(fs); + if (namesList == null) { + return null; + } + + if (GenericUtils.isEmpty(namesList)) { + return root == null ? fs.getPath(".") : root; + } + + Path cleanedPathToResolve = buildRelativePath(fs, namesList); + return root == null ? cleanedPathToResolve : root.resolve(cleanedPathToResolve); + } + + /** + * Build a relative path on the filesystem fs from the path parts in the namesList + * + * @param fs the filesystem for the path + * @param namesList the names list + * @return the built path + */ + public static Path buildRelativePath(FileSystem fs, List<String> namesList) { + String[] names = new String[namesList.size() - 1]; + + Iterator<String> it = namesList.iterator(); + String rootName = it.next(); + for (int i = 0; it.hasNext(); i++) { + names[i] = it.next(); + } + Path cleanedPathToResolve = fs.getPath(rootName, names); + return cleanedPathToResolve; + } } diff --git a/sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java b/sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java index 9ec46b830..2520753ab 100644 --- a/sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java +++ b/sshd-common/src/test/java/org/apache/sshd/common/file/root/RootedFileSystemProviderTest.java @@ -19,28 +19,29 @@ package org.apache.sshd.common.file.root; +import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.nio.ByteBuffer; import java.nio.channels.Channel; import java.nio.channels.FileChannel; -import java.nio.file.DirectoryStream; -import java.nio.file.FileSystem; -import java.nio.file.Files; -import java.nio.file.InvalidPathException; -import java.nio.file.OpenOption; -import java.nio.file.Path; -import java.nio.file.StandardCopyOption; -import java.nio.file.StandardOpenOption; +import java.nio.charset.StandardCharsets; +import java.nio.file.*; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Objects; import java.util.Random; import java.util.TreeSet; import org.apache.sshd.common.util.GenericUtils; +import org.apache.sshd.common.util.OsUtils; import org.apache.sshd.util.test.CommonTestSupportUtils; import org.apache.sshd.util.test.NoIoTestCase; -import org.junit.BeforeClass; +import org.junit.Assert; +import org.junit.Assume; import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -49,7 +50,7 @@ /** * Tests the RootedFileSystemProvider implementation of {@link java.nio.file.spi.FileSystemProvider} checking that * permissions for generic FS commands are not permitted outside of the root directory. - * + * <p> * Individual tests are form pairs (e.g. testX, testXInvalid) where testXInvalid is expected to test a parent path of * {@link RootedFileSystem#getRoot()} * @@ -58,19 +59,21 @@ @FixMethodOrder(MethodSorters.NAME_ASCENDING) @Category({ NoIoTestCase.class }) public class RootedFileSystemProviderTest extends AssertableFile { - private static RootedFileSystem fileSystem; - private static Path rootSandbox; + private static final String SKIP_ON_WINDOWS = "Test fails due to windows normalizing paths before opening them, " + + "allowing one to open a file like \"C:\\directory_doesnt_exist\\..\\myfile.txt\" whereas this is blocked in unix"; + private static final String DOESNT_EXIST = "../doesnt_exist/../"; - public RootedFileSystemProviderTest() { - super(); - } + private final RootedFileSystem fileSystem; + private final Path rootSandbox; + private final FileHelper fileHelper; - @BeforeClass - public static void initializeFileSystem() throws IOException { + public RootedFileSystemProviderTest() throws Exception { + super(); + fileHelper = new FileHelper(); Path targetFolder = Objects.requireNonNull( CommonTestSupportUtils.detectTargetFolder(RootedFileSystemProviderTest.class), "Failed to detect target folder"); - rootSandbox = FileHelper.createTestSandbox(targetFolder.resolve(TEMP_SUBFOLDER_NAME)); + rootSandbox = fileHelper.createTestSandbox(targetFolder.resolve(TEMP_SUBFOLDER_NAME)); fileSystem = (RootedFileSystem) new RootedFileSystemProvider().newFileSystem(rootSandbox, Collections.emptyMap()); } @@ -86,144 +89,216 @@ public void testRoot() { /* mkdir */ @Test public void testMkdir() throws IOException { - Path created = FileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); - assertTrue(exists(created) && isDir(created) && isReadable(created)); + Path created = fileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); + try { + assertTrue(exists(created) && isDir(created) && isReadable(created)); + } finally { + Files.delete(created); + } } - @Test(expected = InvalidPathException.class) - public void testMkdirInvalid() throws IOException { - Path parent = FileHelper.createDirectory(fileSystem.getPath("../" + getCurrentTestName())); - fail(String.format("Unexpected success in creating directory %s", parent.toString())); + @Test + public void testMkdirInvalid() { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + + String parent = DOESNT_EXIST + getCurrentTestName(); + assertThrows(String.format("Unexpected success in creating directory %s", parent), NoSuchFileException.class, + () -> fileHelper.createDirectory(fileSystem.getPath(parent))); } /* rmdir */ @Test public void testRmdir() throws IOException { - Path created = FileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); - Path deleted = FileHelper.deleteDirectory(created); + Path created = fileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); + Path deleted = fileHelper.deleteDirectory(created); notExists(deleted); } - @Test(expected = InvalidPathException.class) + @Test(expected = NoSuchFileException.class) public void testRmdirInvalid() throws IOException { - Path deleted = FileHelper.deleteDirectory(fileSystem.getPath("../" + getCurrentTestName())); + Path deleted = fileHelper.deleteDirectory(fileSystem.getPath(DOESNT_EXIST + getCurrentTestName())); fail(String.format("Unexpected success in removing directory %s", deleted.toString())); } /* chdir */ @Test public void testChdir() throws IOException { - Path created = FileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); - Path createdFile = FileHelper.createFile(created.resolve(getCurrentTestName())); - boolean hasFile = false; - try (DirectoryStream<Path> ds = FileHelper.readDirectory(created)) { - for (Path p : ds) { - hasFile |= FileHelper.isSameFile(createdFile, - fileSystem.getPath(created.getFileName() + "/" + p.getFileName())); + Path created = fileHelper.createDirectory(fileSystem.getPath(getCurrentTestName())); + Path createdFile = fileHelper.createFile(created.resolve(getCurrentTestName())); + try { + boolean hasFile = false; + try (DirectoryStream<Path> ds = fileHelper.readDirectory(created)) { + for (Path p : ds) { + hasFile |= fileHelper.isSameFile(createdFile, + fileSystem.getPath(created.getFileName() + "/" + p.getFileName())); + } } + assertTrue(createdFile + " found in ch directory", hasFile); + } finally { + Files.delete(createdFile); + Files.delete(created); } - assertTrue(createdFile + " found in ch directory", hasFile); - } - - @Test(expected = InvalidPathException.class) - public void testChdirInvalid() throws IOException { - Path chdir = FileHelper.createDirectory(fileSystem.getPath("../" + getCurrentTestName())); - fail(String.format("Unexpected success in changing directory %s", chdir.toString())); } /* write */ @Test public void testWriteFile() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); assertTrue(exists(created) && isReadable(created)); } - @Test(expected = InvalidPathException.class) + @Test public void testWriteFileInvalid() throws IOException { - Path written = FileHelper.createFile(fileSystem.getPath("../" + getCurrentTestName())); - fail(String.format("Unexpected success in writing file %s", written.toString())); + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + + String written = DOESNT_EXIST + getCurrentTestName(); + assertThrows(String.format("Unexpected success in writing file %s", written), NoSuchFileException.class, + () -> fileHelper.createFile(fileSystem.getPath(written))); } /* read */ @Test public void testReadFile() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); - isNonEmpty(FileHelper.readFile(created)); + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + isNonEmpty(fileHelper.readFile(created)); } - @Test(expected = InvalidPathException.class) + @Test(expected = NoSuchFileException.class) public void testReadFileInvalid() throws IOException { - Path read = fileSystem.getPath("../" + getCurrentTestName()); - FileHelper.readFile(read); + Path read = fileSystem.getPath(DOESNT_EXIST + getCurrentTestName()); + fileHelper.readFile(read); fail(String.format("Unexpected success in reading file %s", read.toString())); } /* rm */ @Test public void testDeleteFile() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); - Path deleted = FileHelper.deleteFile(created); + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + Path deleted = fileHelper.deleteFile(created); notExists(deleted); } - @Test(expected = InvalidPathException.class) + @Test(expected = NoSuchFileException.class) public void testDeleteFileInvalid() throws IOException { - Path deleted = FileHelper.deleteFile(fileSystem.getPath("../" + getCurrentTestName())); + Path deleted = fileHelper.deleteFile(fileSystem.getPath(DOESNT_EXIST + getCurrentTestName())); fail(String.format("Unexpected success in deleting file %s", deleted.toString())); } /* cp */ @Test public void testCopyFile() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); Path destination = fileSystem.getPath(getCurrentTestName() + "dest"); - FileHelper.copyFile(created, destination); - assertTrue(exists(destination) && isReadable(destination)); + try { + fileHelper.copyFile(created, destination); + assertTrue(exists(destination) && isReadable(destination)); + } finally { + Files.delete(destination); + Files.delete(created); + } } - @Test(expected = InvalidPathException.class) + @Test public void testCopyFileInvalid() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); - Path copy = FileHelper.copyFile(created, fileSystem.getPath("../" + getCurrentTestName())); - fail(String.format("Unexpected success in copying file to %s", copy.toString())); + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + String copy = DOESNT_EXIST + getCurrentTestName(); + assertThrows(String.format("Unexpected success in copying file to %s", copy), + NoSuchFileException.class, + () -> fileHelper.copyFile(created, fileSystem.getPath(copy))); } /* mv */ @Test public void testMoveFile() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); Path destination = fileSystem.getPath(getCurrentTestName() + "dest"); - FileHelper.moveFile(created, destination); + fileHelper.moveFile(created, destination); assertTrue(notExists(created) && exists(destination) && isReadable(destination)); } - @Test(expected = InvalidPathException.class) + @Test public void testMoveFileInvalid() throws IOException { - Path created = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); - Path moved = FileHelper.moveFile(created, fileSystem.getPath("../" + getCurrentTestName())); - fail(String.format("Unexpected success in moving file to %s", moved.toString())); + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + + Path created = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + String moved = DOESNT_EXIST + getCurrentTestName(); + assertThrows(String.format("Unexpected success in moving file to %s", moved), NoSuchFileException.class, + () -> fileHelper.moveFile(created, fileSystem.getPath(moved))); } /* link */ @Test public void testCreateLink() throws IOException { - Path existing = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + Path existing = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); Path link = fileSystem.getPath(getCurrentTestName() + "link"); - FileHelper.createLink(link, existing); - assertTrue(exists(link) && isReadable(link)); + try { + fileHelper.createLink(link, existing); + assertTrue(exists(link) && isReadable(link)); + } finally { + Files.delete(link); + Files.delete(existing); + } + } + + @Test + public void testJailbreakLink() throws IOException { + testJailbreakLink("../"); + } + + @Test + public void testJailbreakLink2() throws IOException { + testJailbreakLink("../test/"); + } + + @Test + public void testJailbreakLink3() throws IOException { + testJailbreakLink("/.."); + } + + @Test + public void testJailbreakLink4() throws IOException { + testJailbreakLink("/./.."); + } + + @Test + public void testJailbreakLink5() throws IOException { + testJailbreakLink("/./../"); + } + + @Test + public void testJailbreakLink6() throws IOException { + testJailbreakLink("./../"); + } + + @Test + public void testJailbreakLink7() throws IOException { + String fileName = "/testdir/testdir2/../../.."; + testJailbreakLink(fileName); } - @Test(expected = InvalidPathException.class) + private void testJailbreakLink(String jailbrokenTarget) throws IOException { + Path target = fileSystem.getPath(jailbrokenTarget); + Path linkPath = fileSystem.getPath("/testLink"); + Assert.assertThrows(InvalidPathException.class, () -> fileSystem.provider().createSymbolicLink(linkPath, target)); + Assert.assertFalse(Files.exists(linkPath)); + } + + @Test public void testCreateLinkInvalid() throws IOException { - Path existing = FileHelper.createFile(fileSystem.getPath(getCurrentTestName())); - Path link = FileHelper.createLink(fileSystem.getPath("../" + getCurrentTestName() + "link"), existing); - fail(String.format("Unexpected success in linking file %s", link.toString())); + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + + Path existing = fileHelper.createFile(fileSystem.getPath(getCurrentTestName())); + String link = DOESNT_EXIST + getCurrentTestName() + "link"; + assertThrows(String.format("Unexpected success in linking file %s", link), NoSuchFileException.class, + () -> fileHelper.createLink(fileSystem.getPath(link), existing)); } @Test public void testNewByteChannelProviderMismatchException() throws IOException { RootedFileSystemProvider provider = new RootedFileSystemProvider(); - Path tempFolder = assertHierarchyTargetFolderExists(getTempTargetFolder()); + Path tempFolder = getTempTargetFolder(); Path file = Files.createTempFile(tempFolder, getCurrentTestName(), ".txt"); try (FileSystem fs = provider.newFileSystem(tempFolder, Collections.emptyMap()); Channel channel = provider.newByteChannel(fs.getPath(file.getFileName().toString()), Collections.emptySet())) { @@ -235,23 +310,212 @@ public void testNewByteChannelProviderMismatchException() throws IOException { public void testResolveRoot() throws IOException { Path root = GenericUtils.head(fileSystem.getRootDirectories()); Path dir = root.resolve("tsd"); - FileHelper.createDirectory(dir); - Path f1 = FileHelper.createFile(dir.resolve("test.txt")); - Path f2 = Files.newDirectoryStream(dir).iterator().next(); - assertTrue("Unrooted path found", f2 instanceof RootedPath); - assertEquals(f1, f2); - FileHelper.deleteFile(f1); - FileHelper.deleteDirectory(dir); + fileHelper.createDirectory(dir); + Path f1 = fileHelper.createFile(dir.resolve("test.txt")); + try { + Path f2; + try (DirectoryStream<Path> ds = Files.newDirectoryStream(dir)) { + f2 = ds.iterator().next(); + } + assertTrue("Unrooted path found", f2 instanceof RootedPath); + assertEquals(f1, f2); + } finally { + fileHelper.deleteFile(f1); + fileHelper.deleteDirectory(dir); + } + } + + @Test + public void testBreakOutOfChroot1() throws IOException { + String fileName = "../" + getCurrentTestName(); + testBreakOutOfChroot(fileName, fileName); + } + + @Test + public void testBreakOutOfChroot2() throws IOException { + String fileName = "./../" + getCurrentTestName(); + testBreakOutOfChroot(fileName, fileName); + } + + @Test + public void testBreakOutOfChroot3() throws IOException { + String fileName = "/../" + getCurrentTestName(); + testBreakOutOfChroot(fileName, fileName); + } + + @Test + public void testBreakOutOfChroot4() throws IOException { + String fileName = "/.././" + getCurrentTestName(); + testBreakOutOfChroot(fileName, fileName); + } + + @Test + public void testBreakOutOfChroot5() throws IOException { + String fileName = "/./../" + getCurrentTestName(); + testBreakOutOfChroot(fileName, fileName); + } + + @Test + public void testBreakOutOfChroot6() throws IOException { + String fileName = "//../" + getCurrentTestName(); + testBreakOutOfChroot(fileName, "/../" + getCurrentTestName()); + } + + /** + * Tests to make sure that the attempted break out of the chroot does not work with the specified filename + * + * @param fileName the filename to attempt to break out of the chroot with + * @throws IOException on test failure + */ + private void testBreakOutOfChroot(String fileName, String expected) throws IOException { + RootedPath breakoutAttempt = fileSystem.getPath(fileName); + + // make sure that our rooted fs behaves like a proper unix fs + Assert.assertEquals(expected, breakoutAttempt.toString()); + + Path expectedDir = fileSystem.getRoot().resolve(getCurrentTestName()); + Path newDir = fileHelper.createDirectory(breakoutAttempt); + try { + assertTrue(Files.isDirectory(expectedDir)); + + String baseName = breakoutAttempt.getName(breakoutAttempt.getNameCount() - 1).toString(); + assertTrue(fileHelper.isSameFile(newDir, fileSystem.getPath(baseName))); + + // make sure we didn't create it one directory out of the jail + assertFalse(Files.exists(fileSystem.getRoot().resolve("../" + breakoutAttempt.getFileName().toString()))); + + // make sure various methods of referencing the file work. + assertTrue(fileHelper.isSameFile(newDir, fileSystem.getPath("/" + baseName))); + assertTrue(fileHelper.isSameFile(newDir, fileSystem.getPath("/../../" + baseName))); + assertTrue(fileHelper.isSameFile(newDir, fileSystem.getPath("./../" + baseName))); + } finally { + // cleanup the directory. + fileHelper.deleteDirectory(newDir); + } + + assertFalse(Files.isDirectory(expectedDir)); + assertFalse(Files.isDirectory(newDir)); + } + + @Test + public void testValidSymlink1() throws IOException { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + String fileName = "/testdir/../"; + testValidSymlink(fileName, true); + } + + @Test + public void testValidSymlink2() throws IOException { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + String fileName = "/testdir/testdir2/../"; + testValidSymlink(fileName, true); + } + + @Test + public void testValidSymlink3() throws IOException { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + String fileName = "/testdir/../testdir3/"; + testValidSymlink(fileName, true); + } + + @Test + public void testValidSymlink4() throws IOException { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + String fileName = "testdir/../testdir3/../"; + testValidSymlink(fileName, true); + } + + @Test + public void testValidSymlink5() throws IOException { + Assume.assumeFalse(SKIP_ON_WINDOWS, OsUtils.isWin32()); + String fileName = "testdir/../testdir3/../testfile"; + testValidSymlink(fileName, false); + } + + public void testValidSymlink(String symlink, boolean targetIsDirectory) throws IOException { + Path target = fileSystem.getPath(symlink); + Path linkPath = fileSystem.getPath("/testLink"); + final List<Path> toDelete = new ArrayList<>(); + try { + fileSystem.provider().createSymbolicLink(linkPath, target); + toDelete.add(linkPath); + + // ensure that nothing processed the symlink. + Assert.assertEquals(Paths.get(symlink).toString(), + fileSystem.provider().readSymbolicLink(linkPath).toString()); + Assert.assertFalse(Files.exists(target)); + Assert.assertEquals(Files.exists(linkPath), Files.exists(target)); + + // If we don't follow the link, we simply check that the link exists, which it does as we created it. + Assert.assertTrue(Files.exists(linkPath, LinkOption.NOFOLLOW_LINKS)); + + createParentDirs(targetIsDirectory ? target : target.getParent(), toDelete); + + if (!targetIsDirectory) { + Files.createFile(target); + toDelete.add(target); + } + + Assert.assertTrue(Files.exists(linkPath)); + } finally { + for (int i = toDelete.size() - 1; i >= 0; i--) { + Path path = toDelete.get(i); + try { + Files.delete(path); + } catch (IOException ex) { + // ignore as we might try to delete "/dir/.." which will fail as it contains dir.. + } + } + } + } + + private static void createParentDirs(Path target, List<Path> toDelete) throws IOException { + if (target.getParent() != null) { + createParentDirs(target.getParent(), toDelete); + } + + if (!Files.isDirectory(target)) { + Files.createDirectories(target); + toDelete.add(target); + } + } + + @Test + public void testFileNamedSlashOnUnixBasedOS() throws IOException { + // skip ths test on Win32 + if (!"\\".equals(File.separator)) { + Path slashFile = fileSystem.getPath("\\"); + Path created = fileHelper.createFile(slashFile); + try { + assertTrue(Files.isRegularFile(created)); + } finally { + fileHelper.deleteFile(created); + } + } + } + + @Test + public void testStreams() throws IOException { + byte[] data = "This is test data".getBytes(StandardCharsets.UTF_8); + RootedPath testPath = fileSystem.getPath("testfile.txt"); + try (OutputStream is = Files.newOutputStream(testPath)) { + is.write(data); + } + byte[] read = new byte[data.length]; + try (InputStream is = Files.newInputStream(testPath)) { + is.read(read); + } + assertArrayEquals(data, read); } /* Private helper */ /** * Wrapper around the FileSystemProvider to test generic FS related commands. All created temp directories and files - * used for testing are deleted upon JVM exit. + * used for testing must be deleted in the test which creates them */ @SuppressWarnings("synthetic-access") - private static final class FileHelper { + private final class FileHelper { private FileHelper() { super(); } @@ -263,71 +527,66 @@ private FileHelper() { * @return the created sandbox Path * @throws IOException on failure to create */ - public static Path createTestSandbox(Path tempDir) throws IOException { - Path created = Files.createDirectories(tempDir.resolve(RootedFileSystemProviderTest.class.getSimpleName())); - created.toFile().deleteOnExit(); + public Path createTestSandbox(Path tempDir) throws IOException { + Path path = tempDir.resolve(RootedFileSystemProviderTest.class.getSimpleName()); + Path created = Files.createDirectories(path); return created; } - public static Path createFile(Path source) throws InvalidPathException, IOException { + public Path createFile(Path source) throws InvalidPathException, IOException { try (FileChannel fc = fileSystem.provider().newFileChannel(source, new TreeSet<OpenOption>(Arrays.asList(StandardOpenOption.CREATE, StandardOpenOption.WRITE)))) { byte[] randomBytes = new byte[1000]; new Random().nextBytes(randomBytes); fc.write(ByteBuffer.wrap(randomBytes)); - source.toFile().deleteOnExit(); return source; } } - public static Path createLink(Path link, Path existing) throws IOException { + public Path createLink(Path link, Path existing) throws IOException { fileSystem.provider().createLink(link, existing); - link.toFile().deleteOnExit(); return link; } - public static Path createDirectory(Path dir) throws InvalidPathException, IOException { + public Path createDirectory(Path dir) throws InvalidPathException, IOException { fileSystem.provider().createDirectory(dir); - dir.toFile().deleteOnExit(); return dir; } - public static Path deleteDirectory(Path dir) throws InvalidPathException, IOException { + public Path deleteDirectory(Path dir) throws InvalidPathException, IOException { return deleteFile(dir); } - public static Path deleteFile(Path source) throws InvalidPathException, IOException { + public Path deleteFile(Path source) throws InvalidPathException, IOException { fileSystem.provider().delete(source); return source; } - public static byte[] readFile(Path source) throws IOException { + public byte[] readFile(Path source) throws IOException { try (FileChannel fc = fileSystem.provider().newFileChannel(source, - new TreeSet<OpenOption>(Arrays.asList(StandardOpenOption.READ)))) { - byte[] readBytes = new byte[(int) source.toFile().length()]; + Collections.singleton(StandardOpenOption.READ))) { + byte[] readBytes = new byte[(int) Files.size(source)]; fc.read(ByteBuffer.wrap(readBytes)); return readBytes; } } - public static Path copyFile(Path source, Path destination) throws InvalidPathException, IOException { + public Path copyFile(Path source, Path destination) throws InvalidPathException, IOException { fileSystem.provider().copy(source, destination, StandardCopyOption.COPY_ATTRIBUTES); - destination.toFile().deleteOnExit(); return destination; } - public static Path moveFile(Path source, Path destination) throws InvalidPathException, IOException { + public Path moveFile(Path source, Path destination) throws InvalidPathException, IOException { fileSystem.provider().move(source, destination, StandardCopyOption.ATOMIC_MOVE); - destination.toFile().deleteOnExit(); return destination; } - public static DirectoryStream<Path> readDirectory(Path dir) throws InvalidPathException, IOException { + public DirectoryStream<Path> readDirectory(Path dir) throws InvalidPathException, IOException { DirectoryStream<Path> dirStream = fileSystem.provider().newDirectoryStream(dir, entry -> true); return dirStream; } - public static boolean isSameFile(Path source, Path destination) throws IOException { + public boolean isSameFile(Path source, Path destination) throws IOException { return fileSystem.provider().isSameFile(source, destination); } } diff --git a/sshd-common/src/test/java/org/apache/sshd/common/util/io/IoUtilsTest.java b/sshd-common/src/test/java/org/apache/sshd/common/util/io/IoUtilsTest.java index a94811dc5..247e3d655 100644 --- a/sshd-common/src/test/java/org/apache/sshd/common/util/io/IoUtilsTest.java +++ b/sshd-common/src/test/java/org/apache/sshd/common/util/io/IoUtilsTest.java @@ -19,11 +19,17 @@ package org.apache.sshd.common.util.io; +import java.io.IOException; +import java.nio.file.Files; import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; import org.apache.sshd.common.util.NumberUtils; +import org.apache.sshd.util.test.CommonTestSupportUtils; import org.apache.sshd.util.test.JUnitTestSupport; import org.apache.sshd.util.test.NoIoTestCase; +import org.junit.Assert; import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -58,4 +64,46 @@ public void testGetEOLBytes() { } } + /** + * Tests to make sure check exists does not follow symlinks. + * + * @throws IOException on failure + */ + @Test + public void testCheckExists() throws IOException { + testCheckExists(Paths.get("target/IoUtilsTest").toAbsolutePath()); + } + + public void testCheckExists(Path baseDir) throws IOException { + CommonTestSupportUtils.deleteRecursive(baseDir, LinkOption.NOFOLLOW_LINKS); + + Path folder = baseDir.resolve("folder1/folder2/"); + Files.createDirectories(folder); + + Path target = baseDir.resolve("folder1/target"); + Files.createDirectories(target); + + Path dirInTarget = baseDir.resolve("folder1/target/dirintarget"); + Files.createDirectories(dirInTarget); + + Files.createDirectories(target); + Path link = baseDir.resolve("folder1/folder2/link"); + Files.createSymbolicLink(link, target); + + Path link2 = baseDir.resolve("link"); + Files.createSymbolicLink(link2, target); + + Path targetWithLink = baseDir.resolve("folder1/folder2/link/dirintarget"); + + Assert.assertTrue("symlink follow should work", IoUtils.checkFileExists(targetWithLink)); + Assert.assertTrue("symlink follow should work", IoUtils.checkFileExistsAnySymlinks(targetWithLink, false)); + + Assert.assertFalse("Link at end shouldn't be followed", IoUtils.checkFileExistsAnySymlinks(link, true)); + Assert.assertFalse("Nofollow shouldn't follow directory", + IoUtils.checkFileExistsAnySymlinks(targetWithLink, true)); + Assert.assertFalse("Link at beginning shouldn't be followed", + IoUtils.checkFileExistsAnySymlinks(link2, true)); + Assert.assertTrue("Root directory must exist", + IoUtils.checkFileExistsAnySymlinks(baseDir, true)); + } } diff --git a/sshd-common/src/test/java/org/apache/sshd/util/test/CommonTestSupportUtils.java b/sshd-common/src/test/java/org/apache/sshd/util/test/CommonTestSupportUtils.java index 829f25bcd..57f43fbe3 100644 --- a/sshd-common/src/test/java/org/apache/sshd/util/test/CommonTestSupportUtils.java +++ b/sshd-common/src/test/java/org/apache/sshd/util/test/CommonTestSupportUtils.java @@ -495,11 +495,11 @@ private static Path getFile(String resource) { * @throws IOException If failed to access/remove some file(s) */ public static Path deleteRecursive(Path path, LinkOption... options) throws IOException { - if ((path == null) || (!Files.exists(path))) { + if ((path == null) || (!Files.exists(path, options))) { return path; } - if (Files.isDirectory(path)) { + if (Files.isDirectory(path, options)) { try (DirectoryStream<Path> ds = Files.newDirectoryStream(path)) { for (Path child : ds) { deleteRecursive(child, options); diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java index 4ea058e69..23664be71 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java @@ -621,7 +621,7 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep } // delete target if it exists and REPLACE_EXISTING is specified - Boolean status = IoUtils.checkFileExists(target, linkOptions); + Boolean status = IoUtils.checkFileExistsAnySymlinks(target, noFollowLinks); if (status == null) { throw new AccessDeniedException("Existence cannot be determined for copy target: " + target); } @@ -698,7 +698,7 @@ public void move(Path source, Path target, CopyOption... options) throws IOExcep } // delete target if it exists and REPLACE_EXISTING is specified - Boolean status = IoUtils.checkFileExists(target, linkOptions); + Boolean status = IoUtils.checkFileExistsAnySymlinks(target, noFollowLinks); if (status == null) { throw new AccessDeniedException("Existence cannot be determined for move target " + target); } diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java index 425234e96..035243daf 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java @@ -36,6 +36,7 @@ import java.nio.file.LinkOption; import java.nio.file.NoSuchFileException; import java.nio.file.NotDirectoryException; +import java.nio.file.OpenOption; import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; @@ -51,7 +52,6 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; -import java.util.EnumSet; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedList; @@ -645,9 +645,11 @@ protected Map<String, Object> doLStat(int id, String path, int flags) throws IOE * SSH_FXP_LSTAT does not. */ SftpFileSystemAccessor accessor = getFileSystemAccessor(); + + boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_LSTAT, "", p); LinkOption[] options = accessor.resolveFileAccessLinkOptions( this, p, SftpConstants.SSH_FXP_LSTAT, "", false); - return resolveFileAttributes(p, flags, options); + return resolveFileAttributes(p, flags, !followLinks, options); } protected void doSetStat( @@ -675,7 +677,7 @@ protected void doSetStat( Path p = resolveFile(path); if (followLinks == null) { - followLinks = resolvePathResolutionFollowLinks(cmd, extension, p); + followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_SETSTAT, extension, p); } doSetAttributes(cmd, extension, p, attrs, followLinks); } @@ -1408,12 +1410,14 @@ protected Map<String, Object> doStat(int id, String path, int flags) throws IOEx */ Path p = resolveFile(path); SftpFileSystemAccessor accessor = getFileSystemAccessor(); + boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_STAT, "", p); LinkOption[] options = accessor.resolveFileAccessLinkOptions( - this, p, SftpConstants.SSH_FXP_STAT, "", true); - return resolveFileAttributes(p, flags, options); + this, p, SftpConstants.SSH_FXP_STAT, "", followLinks); + return resolveFileAttributes(p, flags, !followLinks, options); } protected void doRealPath(Buffer buffer, int id) throws IOException { + // do things here. String path = buffer.getString(); boolean debugEnabled = log.isDebugEnabled(); ServerSession session = getServerSession(); @@ -1467,7 +1471,6 @@ protected void doRealPath(Buffer buffer, int id) throws IOException { result = doRealPathV6(id, path, extraPaths, p, options); p = result.getKey(); - options = getPathResolutionLinkOption(SftpConstants.SSH_FXP_REALPATH, "", p); Boolean status = result.getValue(); switch (control) { case SftpConstants.SSH_FXP_REALPATH_STAT_IF: @@ -1557,14 +1560,14 @@ protected SimpleImmutableEntry<Path, Boolean> validateRealPath( int id, String path, Path f, LinkOption... options) throws IOException { Path p = normalize(f); - Boolean status = IoUtils.checkFileExists(p, options); + Boolean status = IoUtils.checkFileExistsAnySymlinks(p, !IoUtils.followLinks(options)); return new SimpleImmutableEntry<>(p, status); } protected void doRemoveDirectory(Buffer buffer, int id) throws IOException { String path = buffer.getString(); try { - doRemoveDirectory(id, path, false); + doRemoveDirectory(id, path); } catch (IOException | RuntimeException e) { sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_RMDIR, path); @@ -1574,15 +1577,23 @@ protected void doRemoveDirectory(Buffer buffer, int id) throws IOException { sendStatus(prepareReply(buffer), id, SftpConstants.SSH_FX_OK, ""); } - protected void doRemoveDirectory(int id, String path, boolean followLinks) throws IOException { + protected void doRemoveDirectory(int id, String path) throws IOException { Path p = resolveFile(path); if (log.isDebugEnabled()) { log.debug("doRemoveDirectory({})[id={}] SSH_FXP_RMDIR (path={})[{}]", getServerSession(), id, path, p); } SftpFileSystemAccessor accessor = getFileSystemAccessor(); + + final boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_RMDIR, "", p); + Boolean symlinkCheck = validateParentExistWithNoSymlinksIfNeverFollowSymlinks(p, !followLinks); + if (!Boolean.TRUE.equals(symlinkCheck)) { + throw new AccessDeniedException(p.toString(), p.toString(), + "Parent directories do not exist ore are prohibited symlinks"); + } + LinkOption[] options = accessor.resolveFileAccessLinkOptions( - this, p, SftpConstants.SSH_FXP_RMDIR, "", followLinks); + this, p, SftpConstants.SSH_FXP_RMDIR, "", false); if (Files.isDirectory(p, options)) { doRemove(id, p, true); } else { @@ -1618,7 +1629,7 @@ protected void doMakeDirectory(Buffer buffer, int id) throws IOException { String path = buffer.getString(); Map<String, ?> attrs = readAttrs(buffer); try { - doMakeDirectory(id, path, attrs, false); + doMakeDirectory(id, path, attrs); } catch (IOException | RuntimeException e) { sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_MKDIR, path, attrs); @@ -1629,7 +1640,7 @@ protected void doMakeDirectory(Buffer buffer, int id) throws IOException { } protected void doMakeDirectory( - int id, String path, Map<String, ?> attrs, boolean followLinks) + int id, String path, Map<String, ?> attrs) throws IOException { Path resolvedPath = resolveFile(path); ServerSession session = getServerSession(); @@ -1640,14 +1651,21 @@ protected void doMakeDirectory( SftpFileSystemAccessor accessor = getFileSystemAccessor(); LinkOption[] options = accessor.resolveFileAccessLinkOptions( - this, resolvedPath, SftpConstants.SSH_FXP_MKDIR, "", followLinks); + this, resolvedPath, SftpConstants.SSH_FXP_MKDIR, "", false); + final boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_MKDIR, "", resolvedPath); SftpPathImpl.withAttributeCache(resolvedPath, p -> { - Boolean status = IoUtils.checkFileExists(p, options); - if (status == null) { + Boolean symlinkCheck = validateParentExistWithNoSymlinksIfNeverFollowSymlinks(p, !followLinks); + if (!Boolean.TRUE.equals(symlinkCheck)) { + throw new AccessDeniedException(p.toString(), p.toString(), + "Parent directories do not exist ore are prohibited symlinks"); + } + + Boolean fileExists = IoUtils.checkFileExists(p, options); + if (fileExists == null) { throw new AccessDeniedException(p.toString(), p.toString(), "Cannot validate make-directory existence"); } - if (status) { + if (fileExists) { if (Files.isDirectory(p, options)) { throw new FileAlreadyExistsException(p.toString(), p.toString(), "Target directory already exists"); } else { @@ -1661,7 +1679,6 @@ protected void doMakeDirectory( listener.creating(session, resolvedPath, attrs); try { accessor.createDirectory(this, resolvedPath); - followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_MKDIR, "", resolvedPath); doSetAttributes(SftpConstants.SSH_FXP_MKDIR, "", resolvedPath, attrs, followLinks); } catch (IOException | RuntimeException | Error e) { listener.created(session, resolvedPath, attrs, e); @@ -1676,7 +1693,7 @@ protected void doRemove(Buffer buffer, int id) throws IOException { /* * If 'filename' is a symbolic link, the link is removed, not the file it points to. */ - doRemoveFile(id, path, false); + doRemoveFile(id, path); } catch (IOException | RuntimeException e) { sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_REMOVE, path); return; @@ -1685,17 +1702,20 @@ protected void doRemove(Buffer buffer, int id) throws IOException { sendStatus(prepareReply(buffer), id, SftpConstants.SSH_FX_OK, ""); } - protected void doRemoveFile(int id, String path, boolean followLinks) throws IOException { + protected void doRemoveFile(int id, String path) throws IOException { Path resolvedPath = resolveFile(path); if (log.isDebugEnabled()) { log.debug("doRemoveFile({})[id={}] SSH_FXP_REMOVE (path={}[{}])", getServerSession(), id, path, resolvedPath); } + // whether to follow links in the dir up to the final file + boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_REMOVE, "", resolvedPath); SftpFileSystemAccessor accessor = getFileSystemAccessor(); + // never resolve links in the final path to remove as we want to remove the symlink, not the target LinkOption[] options = accessor.resolveFileAccessLinkOptions( - this, resolvedPath, SftpConstants.SSH_FXP_REMOVE, "", followLinks); + this, resolvedPath, SftpConstants.SSH_FXP_REMOVE, "", false); SftpPathImpl.withAttributeCache(resolvedPath, p -> { - Boolean status = IoUtils.checkFileExists(p, options); + Boolean status = checkSymlinkState(p, followLinks, options); if (status == null) { throw signalRemovalPreConditionFailure(id, path, p, new AccessDeniedException(p.toString(), p.toString(), "Cannot determine existence of remove candidate"), @@ -2293,8 +2313,9 @@ protected void writeDirEntry( int id, DirectoryHandle dir, Map<String, Path> entries, Buffer buffer, int index, Path f, String shortName, LinkOption... options) throws IOException { + boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_READDIR, "", f); Map<String, ?> attrs = resolveFileAttributes( - f, SftpConstants.SSH_FILEXFER_ATTR_ALL, options); + f, SftpConstants.SSH_FILEXFER_ATTR_ALL, !followLinks, options); entries.put(shortName, f); SftpFileSystemAccessor accessor = getFileSystemAccessor(); @@ -2392,10 +2413,10 @@ protected String getShortName(Path f) throws IOException { } protected NavigableMap<String, Object> resolveFileAttributes( - Path path, int flags, LinkOption... options) + Path path, int flags, boolean neverFollowSymLinks, LinkOption... options) throws IOException { return SftpPathImpl.withAttributeCache(path, file -> { - Boolean status = IoUtils.checkFileExists(file, options); + Boolean status = checkSymlinkState(file, neverFollowSymLinks, options); if (status == null) { return handleUnknownStatusFileAttributes(file, flags, options); } else if (!status) { @@ -2406,7 +2427,31 @@ protected NavigableMap<String, Object> resolveFileAttributes( }); } - protected void writeAttrs(Buffer buffer, Map<String, ?> attributes) throws IOException { + /** + * A utility function to validate that the directories leading up to a file are not symlinks + * + * @param path the file to check for symlink presence + * @param neverFollowSymLinks whether to never follow symlinks in the parent paths + * @param options whether the file itself can be a symlink + * @return whether there are symlinks in the path to this file, or null if unknown + */ + public Boolean checkSymlinkState(Path path, boolean neverFollowSymLinks, LinkOption[] options) { + Boolean status = validateParentExistWithNoSymlinksIfNeverFollowSymlinks(path, neverFollowSymLinks); + if (!Boolean.FALSE.equals(status)) { + status = IoUtils.checkFileExists(path, options); + } + return status; + } + + public Boolean validateParentExistWithNoSymlinksIfNeverFollowSymlinks(Path path, boolean neverFollowSymLinks) { + Boolean status = true; + if (neverFollowSymLinks && path.getParent() != null) { + status = IoUtils.checkFileExistsAnySymlinks(path.getParent(), true); + } + return status; + } + + protected void writeAttrs(Buffer buffer, Map<String, ?> attributes) { SftpHelper.writeAttrs(buffer, getVersion(), attributes); } @@ -2653,7 +2698,11 @@ protected void setFileAttributes( case IoUtils.SIZE_VIEW_ATTR: { long newSize = ((Number) value).longValue(); SftpFileSystemAccessor accessor = getFileSystemAccessor(); - Set<StandardOpenOption> openOptions = EnumSet.of(StandardOpenOption.WRITE); + Set<OpenOption> openOptions = new HashSet<>(); + openOptions.add(StandardOpenOption.WRITE); + if (!IoUtils.followLinks(options)) { + openOptions.add(LinkOption.NOFOLLOW_LINKS); + } try (SeekableByteChannel channel = accessor.openFile(this, null, file, null, openOptions)) { channel.truncate(newSize); accessor.closeFile(this, null, file, null, channel, openOptions); diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpFileSystemAccessor.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpFileSystemAccessor.java index 6d7c18fc1..2f6ab0553 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpFileSystemAccessor.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpFileSystemAccessor.java @@ -19,20 +19,14 @@ package org.apache.sshd.sftp.server; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.StreamCorruptedException; import java.nio.channels.Channel; import java.nio.channels.FileChannel; import java.nio.channels.FileLock; import java.nio.channels.SeekableByteChannel; -import java.nio.file.CopyOption; -import java.nio.file.DirectoryStream; -import java.nio.file.FileSystem; -import java.nio.file.Files; -import java.nio.file.InvalidPathException; -import java.nio.file.LinkOption; -import java.nio.file.OpenOption; -import java.nio.file.Path; +import java.nio.file.*; import java.nio.file.attribute.AclEntry; import java.nio.file.attribute.AclFileAttributeView; import java.nio.file.attribute.FileAttribute; @@ -44,8 +38,10 @@ import java.nio.file.attribute.UserPrincipalLookupService; import java.nio.file.attribute.UserPrincipalNotFoundException; import java.security.Principal; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.NavigableMap; @@ -214,7 +210,11 @@ default SeekableByteChannel openFile( attrs = IoUtils.EMPTY_FILE_ATTRIBUTES; } - return FileChannel.open(file, options, attrs); + // Don't use Set contains as this can fail with TreeSet + if (!Arrays.asList(options.toArray(new OpenOption[0])).contains(LinkOption.NOFOLLOW_LINKS)) { + return FileChannel.open(file, options, attrs); + } + return seekableByteChannelNoLinkFollow(file, options, attrs); } /** @@ -317,9 +317,12 @@ default void closeFile( * @throws IOException If failed to open */ default DirectoryStream<Path> openDirectory( - SftpSubsystemProxy subsystem, DirectoryHandle dirHandle, Path dir, String handle) + SftpSubsystemProxy subsystem, DirectoryHandle dirHandle, Path dir, String handle, LinkOption... linkOptions) throws IOException { - return Files.newDirectoryStream(dir); + if (IoUtils.followLinks(linkOptions)) { + return Files.newDirectoryStream(dir); + } + return secureResolveDirectoryStream(dir); } /** @@ -523,15 +526,84 @@ default void renameFile( default void copyFile( SftpSubsystemProxy subsystem, Path src, Path dst, Collection<CopyOption> opts) throws IOException { + + if (noFollow(opts)) { + try (SeekableByteChannel srcFile + = seekableByteChannelNoLinkFollow(src, Collections.singleton(StandardOpenOption.READ))) { + if (!(srcFile instanceof FileChannel)) { + throw new UnsupportedOperationException("Host file system must return a file channel"); + } + + try (SeekableByteChannel dstFile = seekableByteChannelNoLinkFollow(src, new HashSet<>(Arrays + .asList(StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.CREATE)))) { + ((FileChannel) srcFile).transferTo(0, srcFile.size(), dstFile); + } + } + return; + } Files.copy(src, dst, GenericUtils.isEmpty(opts) ? IoUtils.EMPTY_COPY_OPTIONS : opts.toArray(new CopyOption[opts.size()])); } + static SeekableByteChannel seekableByteChannelNoLinkFollow( + Path src, Set<? extends OpenOption> opts, FileAttribute<?>... fileAttributes) + throws IOException { + if (src.getNameCount() < 1) { + // opening root directory isn't supported. + throw new IllegalArgumentException(); + } + Path toResolve = src.isAbsolute() ? src : src.getFileSystem().getPath(src.toString()); + + if (!Files.isDirectory(src.getParent(), LinkOption.NOFOLLOW_LINKS)) { + throw new FileNotFoundException(src.getParent().toString()); + } + + toResolve = toResolve.normalize(); + try (SecureDirectoryStream<Path> ds = secureResolveDirectoryStream(toResolve.getParent())) { + Set<OpenOption> newOpts = new HashSet<>(opts); + newOpts.add(LinkOption.NOFOLLOW_LINKS); + return ds.newByteChannel(toResolve.getName(toResolve.getNameCount() - 1), newOpts, fileAttributes); + } + } + + static SecureDirectoryStream<Path> secureResolveDirectoryStream(Path toResolve) throws IOException { + toResolve = IoUtils.removeCdUpAboveRoot(toResolve); + DirectoryStream<Path> ds = Files.newDirectoryStream(toResolve.getRoot()); + for (int i = 0; i < toResolve.getNameCount(); i++) { + DirectoryStream<Path> dsOld = ds; + try { + ds = secure(ds).newDirectoryStream(toResolve.getName(i), LinkOption.NOFOLLOW_LINKS); + dsOld.close(); + } catch (IOException ex) { + ds.close(); + throw ex; + } + } + return secure(ds); + } + + static SecureDirectoryStream<Path> secure(DirectoryStream<Path> ds) { + if (ds instanceof SecureDirectoryStream) { + return (SecureDirectoryStream<Path>) ds; + } + // do we want to bomb? do we want a different fallback option? + throw new UnsupportedOperationException("FS Does not support secure directory streams."); + } + default void removeFile( SftpSubsystemProxy subsystem, Path path, boolean isDirectory) throws IOException { Files.delete(path); } + + default boolean noFollow(Collection<?> opts) { + for (Object opt : opts) { + if (LinkOption.NOFOLLOW_LINKS.equals(opt)) { + return true; + } + } + return false; + } } diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java index f4c12ab36..65e282a02 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java @@ -686,7 +686,7 @@ protected void doReadDir(Buffer buffer, int id) throws IOException { // the upstream server decide. if (!(file instanceof SftpPath)) { LinkOption[] options = getPathResolutionLinkOption(SftpConstants.SSH_FXP_READDIR, "", file); - Boolean status = IoUtils.checkFileExists(file, options); + Boolean status = IoUtils.checkFileExistsAnySymlinks(file, !IoUtils.followLinks(options)); if (status == null) { throw new AccessDeniedException(file.toString(), file.toString(), "Cannot determine existence of read-dir"); } @@ -747,7 +747,7 @@ protected void doReadDir(Buffer buffer, int id) throws IOException { @Override protected String doOpenDir(int id, String path, Path dir, LinkOption... options) throws IOException { SftpPathImpl.withAttributeCache(dir, p -> { - Boolean status = IoUtils.checkFileExists(p, options); + Boolean status = IoUtils.checkFileExistsAnySymlinks(p, !IoUtils.followLinks(options)); if (status == null) { throw signalOpenFailure(id, path, p, true, new AccessDeniedException(p.toString(), p.toString(), "Cannot determine open-dir existence")); @@ -807,7 +807,9 @@ protected Map<String, Object> doFStat(int id, String handle, int flags) throws I Path file = fileHandle.getFile(); LinkOption[] options = accessor.resolveFileAccessLinkOptions( this, file, SftpConstants.SSH_FXP_FSTAT, "", true); - return resolveFileAttributes(file, flags, options); + + boolean followLinks = resolvePathResolutionFollowLinks(SftpConstants.SSH_FXP_FSTAT, handle, file); + return resolveFileAttributes(file, flags, followLinks, options); } @Override diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java index b150e3aa0..a0b408d1b 100644 --- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java +++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTest.java @@ -383,7 +383,7 @@ private void testCannotEscapeRoot(boolean useAbsolutePath) throws Exception { SftpClient.Attributes attrs = sftp.stat(escapePath); fail("Unexpected escape success for path=" + escapePath + ": " + attrs); } catch (SftpException e) { - int expected = OsUtils.isWin32() || (!useAbsolutePath) + int expected = OsUtils.isWin32() && useAbsolutePath ? SftpConstants.SSH_FX_INVALID_FILENAME : SftpConstants.SSH_FX_NO_SUCH_FILE; assertEquals("Mismatched status for " + escapePath, @@ -711,10 +711,11 @@ public SeekableByteChannel openFile( @Override public DirectoryStream<Path> openDirectory( - SftpSubsystemProxy subsystem, DirectoryHandle dirHandle, Path dir, String handle) + SftpSubsystemProxy subsystem, DirectoryHandle dirHandle, Path dir, String handle, + LinkOption... linkOptions) throws IOException { dirHolder.set(dir); - return SftpFileSystemAccessor.super.openDirectory(subsystem, dirHandle, dir, handle); + return SftpFileSystemAccessor.super.openDirectory(subsystem, dirHandle, dir, handle, linkOptions); } @Override diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpVersionsTest.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpVersionsTest.java index d277bbc88..c18a3d7cd 100644 --- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpVersionsTest.java +++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpVersionsTest.java @@ -291,9 +291,11 @@ public void testSftpACLEncodeDecode() throws Exception { public Command createSubsystem(ChannelSession channel) throws IOException { SftpSubsystem subsystem = new SftpSubsystem(channel, this) { @Override - protected NavigableMap<String, Object> resolveFileAttributes(Path file, int flags, LinkOption... options) + protected NavigableMap<String, Object> resolveFileAttributes( + Path file, int flags, boolean neverFollowSymLinks, LinkOption... options) throws IOException { - NavigableMap<String, Object> attrs = super.resolveFileAttributes(file, flags, options); + NavigableMap<String, Object> attrs + = super.resolveFileAttributes(file, flags, neverFollowSymLinks, options); if (MapEntryUtils.isEmpty(attrs)) { attrs = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } @@ -412,9 +414,11 @@ public void testSftpExtensionsEncodeDecode() throws Exception { public Command createSubsystem(ChannelSession channel) throws IOException { SftpSubsystem subsystem = new SftpSubsystem(channel, this) { @Override - protected NavigableMap<String, Object> resolveFileAttributes(Path file, int flags, LinkOption... options) + protected NavigableMap<String, Object> resolveFileAttributes( + Path file, int flags, boolean neverFollowLinks, LinkOption... options) throws IOException { - NavigableMap<String, Object> attrs = super.resolveFileAttributes(file, flags, options); + NavigableMap<String, Object> attrs + = super.resolveFileAttributes(file, flags, neverFollowLinks, options); if (MapEntryUtils.isEmpty(attrs)) { attrs = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); }
Locations
Projects
Search
Status Monitor
Help
Open Build Service
OBS Manuals
API Documentation
OBS Portal
Reporting a Bug
Contact
Mailing List
Forums
Chat (IRC)
Twitter
Open Build Service (OBS)
is an
openSUSE project
.