Repository: mina-sshd Updated Branches: refs/heads/master 80d718f3d -> 8e255459a
[SSHD-731] Vulnerability in SimpleAccessControlSftpEventListener implementation Project: http://git-wip-us.apache.org/repos/asf/mina-sshd/repo Commit: http://git-wip-us.apache.org/repos/asf/mina-sshd/commit/8e255459 Tree: http://git-wip-us.apache.org/repos/asf/mina-sshd/tree/8e255459 Diff: http://git-wip-us.apache.org/repos/asf/mina-sshd/diff/8e255459 Branch: refs/heads/master Commit: 8e255459acd21cd67c579dbfc6576bddc7161d6c Parents: 80d718f Author: Lyor Goldstein <lyor.goldst...@gmail.com> Authored: Thu Mar 2 20:57:17 2017 +0200 Committer: Lyor Goldstein <lyor.goldst...@gmail.com> Committed: Thu Mar 2 21:07:53 2017 +0200 ---------------------------------------------------------------------- .../SimpleAccessControlSftpEventListener.java | 26 +++- .../apache/sshd/common/util/GenericUtils.java | 13 ++ .../org/apache/sshd/common/util/io/IoUtils.java | 16 ++- .../sftp/AbstractSftpEventListenerAdapter.java | 8 ++ .../server/subsystem/sftp/DirectoryHandle.java | 8 ++ .../sshd/server/subsystem/sftp/FileHandle.java | 133 ++++++++++++------- .../sshd/server/subsystem/sftp/Handle.java | 13 ++ .../subsystem/sftp/SftpEventListener.java | 15 ++- .../server/subsystem/sftp/SftpSubsystem.java | 4 - 9 files changed, 178 insertions(+), 58 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/SimpleAccessControlSftpEventListener.java ---------------------------------------------------------------------- diff --git a/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/SimpleAccessControlSftpEventListener.java b/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/SimpleAccessControlSftpEventListener.java index 68fdb82..1a80bd0 100644 --- a/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/SimpleAccessControlSftpEventListener.java +++ b/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/SimpleAccessControlSftpEventListener.java @@ -23,9 +23,12 @@ import java.io.IOException; import java.nio.file.AccessDeniedException; import java.nio.file.CopyOption; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.util.Collection; import java.util.Map; +import org.apache.sshd.common.util.GenericUtils; +import org.apache.sshd.common.util.io.IoUtils; import org.apache.sshd.server.session.ServerSession; /** @@ -54,6 +57,28 @@ public abstract class SimpleAccessControlSftpEventListener extends AbstractSftpE } @Override + public void opening(ServerSession session, String remoteHandle, Handle localHandle) + throws IOException { + super.opening(session, remoteHandle, localHandle); + if (localHandle instanceof DirectoryHandle) { + if (!isAccessAllowed(session, remoteHandle, localHandle)) { + throw new AccessDeniedException(remoteHandle); + } + } else { + Collection<StandardOpenOption> options = ((FileHandle) localHandle).getOpenOptions(); + if (GenericUtils.containsAny(options, IoUtils.WRITEABLE_OPEN_OPTIONS)) { + if (!isModificationAllowed(session, remoteHandle, localHandle.getFile())) { + throw new AccessDeniedException(remoteHandle); + } + } else { + if (!isAccessAllowed(session, remoteHandle, localHandle)) { + throw new AccessDeniedException(remoteHandle); + } + } + } + } + + @Override public void read(ServerSession session, String remoteHandle, DirectoryHandle localHandle, Map<String, Path> entries) throws IOException { super.read(session, remoteHandle, localHandle, entries); @@ -166,5 +191,4 @@ public abstract class SimpleAccessControlSftpEventListener extends AbstractSftpE * @throws IOException If failed to handle the call */ protected abstract boolean isModificationAllowed(ServerSession session, String remoteHandle, Path localPath) throws IOException; - } http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/common/util/GenericUtils.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/util/GenericUtils.java b/sshd-core/src/main/java/org/apache/sshd/common/util/GenericUtils.java index 92d89b1..852cba6 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/util/GenericUtils.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/util/GenericUtils.java @@ -365,6 +365,19 @@ public final class GenericUtils { return result; } + public static <T> boolean containsAny(Collection<? extends T> coll, Iterable<? extends T> values) { + if (isEmpty(coll)) { + return false; + } + + for (T v : values) { + if (coll.contains(v)) { + return true; + } + } + + return false; + } public static <T> void forEach(Iterable<T> values, Consumer<T> consumer) { if (isNotEmpty(values)) { values.forEach(consumer); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/common/util/io/IoUtils.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/util/io/IoUtils.java b/sshd-core/src/main/java/org/apache/sshd/common/util/io/IoUtils.java index fae069d..3f9bfcd 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/util/io/IoUtils.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/util/io/IoUtils.java @@ -36,6 +36,7 @@ import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.OpenOption; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.UserPrincipal; @@ -73,7 +74,20 @@ public final class IoUtils { /** * The local O/S line separator */ - public static final String EOL = System.getProperty("line.separator"); + public static final String EOL = System.lineSeparator(); + + /** + * A {@link Set} of {@link StandardOpenOption}-s that indicate an intent + * to create/modify a file + */ + public static final Set<StandardOpenOption> WRITEABLE_OPEN_OPTIONS = + Collections.unmodifiableSet( + EnumSet.of( + StandardOpenOption.APPEND, StandardOpenOption.CREATE, + StandardOpenOption.CREATE_NEW, StandardOpenOption.DELETE_ON_CLOSE, + StandardOpenOption.DSYNC, StandardOpenOption.SYNC, + StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE)); + private static final byte[] EOL_BYTES = EOL.getBytes(StandardCharsets.UTF_8); private static final LinkOption[] NO_FOLLOW_OPTIONS = new LinkOption[]{LinkOption.NOFOLLOW_LINKS}; http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/AbstractSftpEventListenerAdapter.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/AbstractSftpEventListenerAdapter.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/AbstractSftpEventListenerAdapter.java index 56a12b0..6895bae 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/AbstractSftpEventListenerAdapter.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/AbstractSftpEventListenerAdapter.java @@ -56,6 +56,14 @@ public abstract class AbstractSftpEventListenerAdapter extends AbstractLoggingBe } @Override + public void opening(ServerSession session, String remoteHandle, Handle localHandle) throws IOException { + if (log.isTraceEnabled()) { + Path path = localHandle.getFile(); + log.trace("opening(" + session + ")[" + remoteHandle + "] " + (Files.isDirectory(path) ? "directory" : "file") + " " + path); + } + } + + @Override public void open(ServerSession session, String remoteHandle, Handle localHandle) { if (log.isTraceEnabled()) { Path path = localHandle.getFile(); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/DirectoryHandle.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/DirectoryHandle.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/DirectoryHandle.java index 82e691e..0ae60cf 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/DirectoryHandle.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/DirectoryHandle.java @@ -39,6 +39,7 @@ public class DirectoryHandle extends Handle implements Iterator<Path> { public DirectoryHandle(SftpSubsystem subsystem, Path dir, String handle) throws IOException { super(dir, handle); + signalHandleOpening(subsystem); SftpFileSystemAccessor accessor = subsystem.getFileSystemAccessor(); ServerSession session = subsystem.getServerSession(); @@ -49,6 +50,13 @@ public class DirectoryHandle extends Handle implements Iterator<Path> { sendDotDot = false; // if no parent then no need to send ".." } fileList = ds.iterator(); + + try { + signalHandleOpen(subsystem); + } catch (IOException e) { + close(); + throw e; + } } public boolean isDone() { http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/FileHandle.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/FileHandle.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/FileHandle.java index f737a91..b499524 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/FileHandle.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/FileHandle.java @@ -27,6 +27,7 @@ import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileAttribute; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.EnumSet; import java.util.Iterator; import java.util.LinkedList; @@ -41,79 +42,53 @@ import org.apache.sshd.common.util.GenericUtils; import org.apache.sshd.common.util.io.IoUtils; import org.apache.sshd.server.session.ServerSession; - /** * @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a> */ public class FileHandle extends Handle { - private final int access; private final SeekableByteChannel fileChannel; private final List<FileLock> locks = new ArrayList<>(); private final SftpSubsystem subsystem; + private final Set<StandardOpenOption> openOptions; + private final Collection<FileAttribute<?>> fileAttributes; public FileHandle(SftpSubsystem subsystem, Path file, String handle, int flags, int access, Map<String, Object> attrs) throws IOException { super(file, handle); this.subsystem = Objects.requireNonNull(subsystem, "No subsystem instance provided"); this.access = access; + this.openOptions = Collections.unmodifiableSet(getOpenOptions(flags, access)); + this.fileAttributes = Collections.unmodifiableCollection(toFileAttributes(attrs)); + signalHandleOpening(subsystem); - Set<StandardOpenOption> options = EnumSet.noneOf(StandardOpenOption.class); - if (((access & SftpConstants.ACE4_READ_DATA) != 0) || ((access & SftpConstants.ACE4_READ_ATTRIBUTES) != 0)) { - options.add(StandardOpenOption.READ); - } - if (((access & SftpConstants.ACE4_WRITE_DATA) != 0) || ((access & SftpConstants.ACE4_WRITE_ATTRIBUTES) != 0)) { - options.add(StandardOpenOption.WRITE); - } - - int accessDisposition = flags & SftpConstants.SSH_FXF_ACCESS_DISPOSITION; - switch (accessDisposition) { - case SftpConstants.SSH_FXF_CREATE_NEW: - options.add(StandardOpenOption.CREATE_NEW); - break; - case SftpConstants.SSH_FXF_CREATE_TRUNCATE: - options.add(StandardOpenOption.CREATE); - options.add(StandardOpenOption.TRUNCATE_EXISTING); - break; - case SftpConstants.SSH_FXF_OPEN_EXISTING: - break; - case SftpConstants.SSH_FXF_OPEN_OR_CREATE: - options.add(StandardOpenOption.CREATE); - break; - case SftpConstants.SSH_FXF_TRUNCATE_EXISTING: - options.add(StandardOpenOption.TRUNCATE_EXISTING); - break; - default: // ignored - } - if ((flags & SftpConstants.SSH_FXF_APPEND_DATA) != 0) { - options.add(StandardOpenOption.APPEND); - } - - Collection<FileAttribute<?>> attributes = null; - // Cannot use forEach because the referenced attributes variable is not effectively final - for (Map.Entry<String, Object> attr : attrs.entrySet()) { - FileAttribute<?> fileAttr = toFileAttribute(attr.getKey(), attr.getValue()); - if (fileAttr == null) { - continue; - } - if (attributes == null) { - attributes = new LinkedList<>(); - } - attributes.add(fileAttr); - } - - FileAttribute<?>[] fileAttrs = GenericUtils.isEmpty(attributes) + FileAttribute<?>[] fileAttrs = GenericUtils.isEmpty(fileAttributes) ? IoUtils.EMPTY_FILE_ATTRIBUTES - : attributes.toArray(new FileAttribute<?>[attributes.size()]); + : fileAttributes.toArray(new FileAttribute<?>[fileAttributes.size()]); SftpFileSystemAccessor accessor = subsystem.getFileSystemAccessor(); ServerSession session = subsystem.getServerSession(); SeekableByteChannel channel; try { - channel = accessor.openFile(session, subsystem, file, handle, options, fileAttrs); + channel = accessor.openFile(session, subsystem, file, handle, openOptions, fileAttrs); } catch (UnsupportedOperationException e) { - channel = accessor.openFile(session, subsystem, file, handle, options, IoUtils.EMPTY_FILE_ATTRIBUTES); + channel = accessor.openFile(session, subsystem, file, handle, openOptions, IoUtils.EMPTY_FILE_ATTRIBUTES); subsystem.doSetAttributes(file, attrs); } this.fileChannel = channel; + + try { + signalHandleOpen(subsystem); + } catch (IOException e) { + close(); + throw e; + } + } + + public final Set<StandardOpenOption> getOpenOptions() { + return openOptions; + } + + public final Collection<FileAttribute<?>> getFileAttributes() { + return fileAttributes; } public final SeekableByteChannel getFileChannel() { @@ -203,7 +178,28 @@ public class FileHandle extends Handle { lock.release(); } - public static FileAttribute<?> toFileAttribute(final String key, final Object val) { + public static Collection<FileAttribute<?>> toFileAttributes(Map<String, Object> attrs) { + if (GenericUtils.isEmpty(attrs)) { + return Collections.emptyList(); + } + + Collection<FileAttribute<?>> attributes = null; + // Cannot use forEach because the referenced attributes variable is not effectively final + for (Map.Entry<String, Object> attr : attrs.entrySet()) { + FileAttribute<?> fileAttr = toFileAttribute(attr.getKey(), attr.getValue()); + if (fileAttr == null) { + continue; + } + if (attributes == null) { + attributes = new LinkedList<>(); + } + attributes.add(fileAttr); + } + + return (attributes == null) ? Collections.emptyList() : attributes; + } + + public static FileAttribute<?> toFileAttribute(String key, Object val) { // Some ignored attributes sent by the SFTP client if ("isOther".equals(key)) { if ((Boolean) val) { @@ -236,4 +232,39 @@ public class FileHandle extends Handle { } }; } + + public static Set<StandardOpenOption> getOpenOptions(int flags, int access) { + Set<StandardOpenOption> options = EnumSet.noneOf(StandardOpenOption.class); + if (((access & SftpConstants.ACE4_READ_DATA) != 0) || ((access & SftpConstants.ACE4_READ_ATTRIBUTES) != 0)) { + options.add(StandardOpenOption.READ); + } + if (((access & SftpConstants.ACE4_WRITE_DATA) != 0) || ((access & SftpConstants.ACE4_WRITE_ATTRIBUTES) != 0)) { + options.add(StandardOpenOption.WRITE); + } + + int accessDisposition = flags & SftpConstants.SSH_FXF_ACCESS_DISPOSITION; + switch (accessDisposition) { + case SftpConstants.SSH_FXF_CREATE_NEW: + options.add(StandardOpenOption.CREATE_NEW); + break; + case SftpConstants.SSH_FXF_CREATE_TRUNCATE: + options.add(StandardOpenOption.CREATE); + options.add(StandardOpenOption.TRUNCATE_EXISTING); + break; + case SftpConstants.SSH_FXF_OPEN_EXISTING: + break; + case SftpConstants.SSH_FXF_OPEN_OR_CREATE: + options.add(StandardOpenOption.CREATE); + break; + case SftpConstants.SSH_FXF_TRUNCATE_EXISTING: + options.add(StandardOpenOption.TRUNCATE_EXISTING); + break; + default: // ignored + } + if ((flags & SftpConstants.SSH_FXF_APPEND_DATA) != 0) { + options.add(StandardOpenOption.APPEND); + } + + return options; + } } http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/Handle.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/Handle.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/Handle.java index ad166a9..a860eec 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/Handle.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/Handle.java @@ -24,6 +24,7 @@ import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.sshd.common.util.ValidateUtils; +import org.apache.sshd.server.session.ServerSession; /** * @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a> @@ -38,6 +39,18 @@ public abstract class Handle implements java.nio.channels.Channel { this.handle = ValidateUtils.checkNotNullAndNotEmpty(handle, "No assigned handle for %s", file); } + protected void signalHandleOpening(SftpSubsystem subsystem) throws IOException { + SftpEventListener listener = subsystem.getSftpEventListenerProxy(); + ServerSession session = subsystem.getServerSession(); + listener.opening(session, handle, this); + } + + protected void signalHandleOpen(SftpSubsystem subsystem) throws IOException { + SftpEventListener listener = subsystem.getSftpEventListenerProxy(); + ServerSession session = subsystem.getServerSession(); + listener.open(session, handle, this); + } + public Path getFile() { return file; } http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpEventListener.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpEventListener.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpEventListener.java index 6af7199..c518af3 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpEventListener.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpEventListener.java @@ -56,6 +56,19 @@ public interface SftpEventListener extends SshdEventListener { } /** + * Specified file / directory is being opened + * + * @param session The {@link ServerSession} through which the request was handled + * @param remoteHandle The (opaque) assigned handle for the file / directory + * @param localHandle The associated file / directory {@link Handle} + * @throws IOException If failed to handle the call + */ + default void opening(ServerSession session, String remoteHandle, Handle localHandle) + throws IOException { + // ignored + } + + /** * Specified file / directory has been opened * * @param session The {@link ServerSession} through which the request was handled @@ -65,7 +78,7 @@ public interface SftpEventListener extends SshdEventListener { */ default void open(ServerSession session, String remoteHandle, Handle localHandle) throws IOException { - // ignored + // ignored } /** http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/8e255459/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java index dab5960..fb41d33 100644 --- a/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java +++ b/sshd-core/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystem.java @@ -1902,8 +1902,6 @@ public class SftpSubsystem } else { String handle = generateFileHandle(p); DirectoryHandle dirHandle = new DirectoryHandle(this, p, handle); - SftpEventListener listener = getSftpEventListenerProxy(); - listener.open(getServerSession(), handle, dirHandle); handles.put(handle, dirHandle); return handle; } @@ -2228,8 +2226,6 @@ public class SftpSubsystem Path file = resolveFile(path); String handle = generateFileHandle(file); FileHandle fileHandle = new FileHandle(this, file, handle, pflags, access, attrs); - SftpEventListener listener = getSftpEventListenerProxy(); - listener.open(getServerSession(), handle, fileHandle); handles.put(handle, fileHandle); return handle; }
