[SSHD-775] SftpSubSystem::sendStatus leaks Exception information
Project: http://git-wip-us.apache.org/repos/asf/mina-sshd/repo Commit: http://git-wip-us.apache.org/repos/asf/mina-sshd/commit/2529a4c3 Tree: http://git-wip-us.apache.org/repos/asf/mina-sshd/tree/2529a4c3 Diff: http://git-wip-us.apache.org/repos/asf/mina-sshd/diff/2529a4c3 Branch: refs/heads/master Commit: 2529a4c3da8635ca350cd85ae76b0df5ac3b39d0 Parents: 654bb26 Author: Lyor Goldstein <lyor.goldst...@gmail.com> Authored: Fri Oct 6 18:42:41 2017 +0300 Committer: Lyor Goldstein <lyor.goldst...@gmail.com> Committed: Sat Oct 7 18:59:47 2017 +0300 ---------------------------------------------------------------------- README.md | 7 + .../DetailedSftpErrorStatusDataHandler.java | 60 + .../sshd/common/subsystem/sftp/SftpHelper.java | 81 +- .../apache/sshd/common/util/GenericUtils.java | 2 +- .../sftp/AbstractSftpSubsystemHelper.java | 2582 ++++++++++++++++++ .../sftp/SftpErrorStatusDataHandler.java | 83 + .../subsystem/sftp/SftpFileSystemAccessor.java | 22 + .../server/subsystem/sftp/SftpSubsystem.java | 2565 +---------------- .../sftp/SftpSubsystemEnvironment.java | 67 + .../subsystem/sftp/SftpSubsystemFactory.java | 24 +- .../subsystem/sftp/SftpFileSystemTest.java | 4 +- .../sshd/client/subsystem/sftp/SftpTest.java | 33 +- .../subsystem/sftp/SftpVersionSelectorTest.java | 18 +- .../client/subsystem/sftp/SftpVersionsTest.java | 23 +- .../SpaceAvailableExtensionImplTest.java | 3 +- .../openssh/helpers/OpenSSHExtensionsTest.java | 3 +- .../subsystem/sftp/SftpConstantsTest.java | 9 + .../sftp/ApacheSshdSftpSessionFactory.java | 5 +- 18 files changed, 3035 insertions(+), 2556 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/2529a4c3/README.md ---------------------------------------------------------------------- diff --git a/README.md b/README.md index 260c024..2c29d77 100644 --- a/README.md +++ b/README.md @@ -746,6 +746,13 @@ One can skip all the conditional code if a specific known extension is required: ``` +### Internal exceptions and error message handling + +If an exception is thrown during processing of an SFTP command, then the exception is translated into a `SSH_FXP_STATUS` message +using a registered `SftpErrorStatusDataHandler`. The default implementation provides a short description of the failure based on the thrown +exception type. However, users may override it when creating the `SftpSubsystemFactory` and provide their own codes and/or messages - e.g., for debugging one can register a `DetailedSftpErrorStatusDataHandler` (see `sshd-contrib`) that "leaks" more information in the generated message. + + ## Port forwarding ### Standard port forwarding http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/2529a4c3/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/DetailedSftpErrorStatusDataHandler.java ---------------------------------------------------------------------- diff --git a/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/DetailedSftpErrorStatusDataHandler.java b/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/DetailedSftpErrorStatusDataHandler.java new file mode 100644 index 0000000..3990eab --- /dev/null +++ b/sshd-contrib/src/main/java/org/apache/sshd/server/subsystem/sftp/DetailedSftpErrorStatusDataHandler.java @@ -0,0 +1,60 @@ +/* + * 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.server.subsystem.sftp; + +import java.nio.file.FileSystemException; +import java.util.Objects; + +import org.apache.sshd.common.subsystem.sftp.SftpException; + +/** + * An {@link SftpErrorStatusDataHandler} implementation that returns an elaborate + * message string for the thrown exception - thus potentially "leaking" + * information about the internal implementation and/or real paths. Recommended for + * debugging or systems where such leakage is not considered a security risk + * + * @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a> + */ +public class DetailedSftpErrorStatusDataHandler implements SftpErrorStatusDataHandler { + public static final DetailedSftpErrorStatusDataHandler INSTANCE = new DetailedSftpErrorStatusDataHandler(); + + public DetailedSftpErrorStatusDataHandler() { + super(); + } + + @Override + public String resolveErrorMessage( + SftpSubsystemEnvironment sftpSubsystem, int id, Throwable e, int subStatus, int cmd, Object... args) { + if (e instanceof FileSystemException) { + FileSystemException fse = (FileSystemException) e; + String file = fse.getFile(); + String otherFile = fse.getOtherFile(); + String message = fse.getReason(); + return e.getClass().getSimpleName() + + "[file=" + file + "]" + + (Objects.equals(file, otherFile) ? "" : "[other=" + otherFile + "]") + + ": " + message; + } else if (e instanceof SftpException) { + return e.toString(); + } else { + return "Internal " + e.getClass().getSimpleName() + ": " + e.getMessage(); + } + } +} http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/2529a4c3/sshd-core/src/main/java/org/apache/sshd/common/subsystem/sftp/SftpHelper.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/subsystem/sftp/SftpHelper.java b/sshd-core/src/main/java/org/apache/sshd/common/subsystem/sftp/SftpHelper.java index 5b8b143..ead2764 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/subsystem/sftp/SftpHelper.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/subsystem/sftp/SftpHelper.java @@ -20,6 +20,7 @@ package org.apache.sshd.common.subsystem.sftp; import java.io.EOFException; import java.io.FileNotFoundException; +import java.net.UnknownServiceException; import java.nio.channels.OverlappingFileLockException; import java.nio.charset.StandardCharsets; import java.nio.file.AccessDeniedException; @@ -42,9 +43,11 @@ import java.security.Principal; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.EnumSet; import java.util.List; import java.util.Map; +import java.util.NavigableMap; import java.util.Objects; import java.util.Set; import java.util.TreeMap; @@ -77,6 +80,48 @@ public final class SftpHelper { */ public static final boolean DEFAULT_APPEND_END_OF_LIST_INDICATOR = true; + public static final NavigableMap<Integer, String> DEFAULT_SUBSTATUS_MESSAGE = + Collections.unmodifiableNavigableMap(new TreeMap<Integer, String>(Comparator.naturalOrder()) { + // Not serializing it + private static final long serialVersionUID = 1L; + + { + put(SftpConstants.SSH_FX_OK, "Success"); + put(SftpConstants.SSH_FX_EOF, "End of file"); + put(SftpConstants.SSH_FX_NO_SUCH_FILE, "No such file or directory"); + put(SftpConstants.SSH_FX_PERMISSION_DENIED, "Permission denied"); + put(SftpConstants.SSH_FX_FAILURE, "General failure"); + put(SftpConstants.SSH_FX_BAD_MESSAGE, "Bad message data"); + put(SftpConstants.SSH_FX_NO_CONNECTION, "No connection to server"); + put(SftpConstants.SSH_FX_CONNECTION_LOST, "Connection lost"); + put(SftpConstants.SSH_FX_OP_UNSUPPORTED, "Unsupported operation requested"); + put(SftpConstants.SSH_FX_INVALID_HANDLE, "Invalid handle value"); + put(SftpConstants.SSH_FX_NO_SUCH_PATH, "No such path"); + put(SftpConstants.SSH_FX_FILE_ALREADY_EXISTS, "File/Directory already exists"); + put(SftpConstants.SSH_FX_WRITE_PROTECT, "File/Directory is write-protected"); + put(SftpConstants.SSH_FX_NO_MEDIA, "No such meadia"); + put(SftpConstants.SSH_FX_NO_SPACE_ON_FILESYSTEM, "No space left on device"); + put(SftpConstants.SSH_FX_QUOTA_EXCEEDED, "Quota exceeded"); + put(SftpConstants.SSH_FX_UNKNOWN_PRINCIPAL, "Unknown user/group"); + put(SftpConstants.SSH_FX_LOCK_CONFLICT, "Lock conflict"); + put(SftpConstants.SSH_FX_DIR_NOT_EMPTY, "Directory not empty"); + put(SftpConstants.SSH_FX_NOT_A_DIRECTORY, "Accessed location is not a directory"); + put(SftpConstants.SSH_FX_INVALID_FILENAME, "Invalid filename"); + put(SftpConstants.SSH_FX_LINK_LOOP, "Link loop"); + put(SftpConstants.SSH_FX_CANNOT_DELETE, "Cannot remove"); + put(SftpConstants.SSH_FX_INVALID_PARAMETER, "Invalid parameter"); + put(SftpConstants.SSH_FX_FILE_IS_A_DIRECTORY, "Accessed location is a directory"); + put(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_CONFLICT, "Range lock conflict"); + put(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_REFUSED, "Range lock refused"); + put(SftpConstants.SSH_FX_DELETE_PENDING, "Delete pending"); + put(SftpConstants.SSH_FX_FILE_CORRUPT, "Corrupted file/directory"); + put(SftpConstants.SSH_FX_OWNER_INVALID, "Invalid file/directory owner"); + put(SftpConstants.SSH_FX_GROUP_INVALID, "Invalid file/directory group"); + put(SftpConstants.SSH_FX_NO_MATCHING_BYTE_RANGE_LOCK, "No matching byte range lock"); + } + }); + + private SftpHelper() { throw new UnsupportedOperationException("No instance allowed"); } @@ -452,14 +497,13 @@ public final class SftpHelper { return SftpConstants.SSH_FX_EOF; } else if (t instanceof OverlappingFileLockException) { return SftpConstants.SSH_FX_LOCK_CONFLICT; - } else if (t instanceof UnsupportedOperationException) { + } else if ((t instanceof UnsupportedOperationException) + || (t instanceof UnknownServiceException)) { return SftpConstants.SSH_FX_OP_UNSUPPORTED; } else if (t instanceof InvalidPathException) { return SftpConstants.SSH_FX_INVALID_FILENAME; } else if (t instanceof IllegalArgumentException) { return SftpConstants.SSH_FX_INVALID_PARAMETER; - } else if (t instanceof UnsupportedOperationException) { - return SftpConstants.SSH_FX_OP_UNSUPPORTED; } else if (t instanceof UserPrincipalNotFoundException) { return SftpConstants.SSH_FX_UNKNOWN_PRINCIPAL; } else if (t instanceof FileSystemLoopException) { @@ -471,18 +515,13 @@ public final class SftpHelper { } } - public static String resolveStatusMessage(Throwable t) { - if (t == null) { - return ""; - } else if (t instanceof SftpException) { - return t.toString(); - } else { - return "Internal " + t.getClass().getSimpleName() + ": " + t.getMessage(); - } + public static String resolveStatusMessage(int subStatus) { + String message = DEFAULT_SUBSTATUS_MESSAGE.get(subStatus); + return GenericUtils.isEmpty(message) ? ("Unknown error: " + subStatus) : message; } - public static Map<String, Object> readAttrs(Buffer buffer, int version) { - Map<String, Object> attrs = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + public static NavigableMap<String, Object> readAttrs(Buffer buffer, int version) { + NavigableMap<String, Object> attrs = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); int flags = buffer.getInt(); if (version >= SftpConstants.SFTP_V4) { int type = buffer.getUByte(); @@ -592,10 +631,10 @@ public final class SftpHelper { return attrs; } - public static Map<String, byte[]> readExtensions(Buffer buffer) { + public static NavigableMap<String, byte[]> readExtensions(Buffer buffer) { int count = buffer.getInt(); // NOTE - Map<String, byte[]> extended = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + NavigableMap<String, byte[]> extended = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); for (int i = 0; i < count; i++) { String key = buffer.getString(); byte[] val = buffer.getBytes(); @@ -623,13 +662,13 @@ public final class SftpHelper { } } - public static Map<String, String> toStringExtensions(Map<String, ?> extensions) { + public static NavigableMap<String, String> toStringExtensions(Map<String, ?> extensions) { if (GenericUtils.isEmpty(extensions)) { - return Collections.emptyMap(); + return Collections.emptyNavigableMap(); } // NOTE: even though extensions are probably case sensitive we do not allow duplicate name that differs only in case - Map<String, String> map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + NavigableMap<String, String> map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); extensions.forEach((key, value) -> { ValidateUtils.checkNotNull(value, "No value for extension=%s", key); String prev = map.put(key, (value instanceof byte[]) ? new String((byte[]) value, StandardCharsets.UTF_8) : value.toString()); @@ -639,13 +678,13 @@ public final class SftpHelper { return map; } - public static Map<String, byte[]> toBinaryExtensions(Map<String, String> extensions) { + public static NavigableMap<String, byte[]> toBinaryExtensions(Map<String, String> extensions) { if (GenericUtils.isEmpty(extensions)) { - return Collections.emptyMap(); + return Collections.emptyNavigableMap(); } // NOTE: even though extensions are probably case sensitive we do not allow duplicate name that differs only in case - Map<String, byte[]> map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + NavigableMap<String, byte[]> map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); extensions.forEach((key, value) -> { ValidateUtils.checkNotNull(value, "No value for extension=%s", key); byte[] prev = map.put(key, value.getBytes(StandardCharsets.UTF_8)); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/2529a4c3/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 7280288..6bf6d20 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 @@ -878,7 +878,7 @@ public final class GenericUtils { this.map = new LinkedHashMap<>(); } - public MapBuilder(Comparator<K> comparator) { + public MapBuilder(Comparator<? super K> comparator) { this.map = new TreeMap<>(comparator); }
