apacheGH-403: Fix SFTP handle size

Change the file handle implementation: use raw bytes, not hexified
bytes. Representing file handles internally as hexified strings doubled
the size, so with the default setting of 16, an Apache MINA file handle
was actually 32 bytes on the network.

SftpModuleProperties.FILE_HANDLE_SIZE specified that its the size in
bytes of the file handle; so the final handle as sent to the client
should not be twice the configured size.

Because many APIs in Apache MINA sshd use String as the type to pass
around such file handles, we cannot easily change that without
potentially breaking a lot of user code. So keep storing file handles
as strings, but use ISO-8859-1 to convert: this lets us store any byte
value as a character.

When printing file handles, for instance in log messages, hexify them
explicitly.

Bug: apache#403

CHANGES.md

@@ -43,8 +43,44 @@
 
 * [GH-356](https://github.com/apache/mina-sshd/issues/356) Publish snapshot maven artifacts to the [Apache Snapshots](https://repository.apache.org/content/repositories/snapshots) maven repository.
 
+# Behavioral changes and enhancements
 
-## Major Code Re-factoring
+### SFTP file handle size
+
+Previous versions of Apache MINA sshd used SFTP file handles that were twice
+as long a configured via `SftpModuleProperties.FILE_HANDLE_SIZE`. The reason for
+this was that the file handle bytes were stringified, representing each byte
+as two hex characters. This stringified file handle was then send over the wire.
+If `SftpModuleProperties.FILE_HANDLE_SIZE` was configured as 16, the actual file
+handle size was thus 32 bytes.
+
+This has been fixed in this version.
+
+## Potential compatibility issues
+
+### Server-side SFTP file handle encoding
+
+The aforementioned fix for the size of SFTP file handles has the potential to
+have undesired effects on existing server-side code that assumed that such SFTP
+file handles contained only printable bytes. This is no longer the case. For
+historical reasons, Apache MINA sshd stores these SFTP file handles as Java
+`String`s, and it's not possible to change this without a great many APIs.
+So this was kept, but the strings are now encoded as ISO-8859-1 and may
+contain arbitrary characters in the range from 0 to 255. This change
+*should* be transparent as SFTP file handles are supposed to be opaque, but
+there is one caveat:
+
+If you have implemented your own server and have subclassed `SftpSubsystem` or
+if you install an `SftpEventListener` that stores or logs raw SFTP file handles,
+your code may need to be adapted. There is a new method
+`String Handle.safe(String rawHandle)` that can be used to convert an SFTP file
+handle to a printable string.
+
+Otherwise the change is transparent to server implementors and to SFTP clients.
+(On the client side, Apache MINA sshd already used `byte[]` to represent SFTP
+file handles.) 
+
+### Major Code Re-factoring
 
 As part of the fix for [GH-371](https://github.com/apache/mina-sshd/issues/371)
 the channel pool in `SftpFileSystem` was rewritten completely. Previous code also

sshd-sftp/src/main/java/org/apache/sshd/sftp/server/AbstractSftpSubsystemHelper.java

@@ -530,11 +530,11 @@ protected <E extends IOException> E signalOpenFailure(
  }
 
  protected void doClose(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  try {
  doClose(id, handle);
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_CLOSE, handle);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_CLOSE, Handle.safe(handle));
  return;
  }
 
@@ -544,15 +544,15 @@ protected void doClose(Buffer buffer, int id) throws IOException {
  protected abstract void doClose(int id, String handle) throws IOException;
 
  protected void doRead(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  long offset = buffer.getLong();
  int requestedLength = buffer.getInt();
  ServerSession session = getServerSession();
  int maxAllowed = SftpModuleProperties.MAX_READDATA_PACKET_LENGTH.getRequired(session);
  int readLen = Math.min(requestedLength, maxAllowed);
  if (log.isTraceEnabled()) {
  log.trace("doRead({})[id={}]({})[offset={}] - req={}, max={}, effective={}",
- session, id, handle, offset, requestedLength, maxAllowed, readLen);
+ session, id, Handle.safe(handle), offset, requestedLength, maxAllowed, readLen);
  }
 
  try {
@@ -571,7 +571,8 @@ protected void doRead(Buffer buffer, int id) throws IOException {
  int startPos = buffer.wpos();
  int len = doRead(id, handle, offset, readLen, buffer.array(), startPos, eofRef);
  if (len < 0) {
- throw new EOFException("Unable to read " + readLen + " bytes from offset=" + offset + " of " + handle);
+ throw new EOFException(
+ "Unable to read " + readLen + " bytes from offset=" + offset + " of " + Handle.safe(handle));
  }
  buffer.wpos(startPos + len);
  BufferUtils.updateLengthPlaceholder(buffer, lenPos, len);
@@ -585,7 +586,7 @@ protected void doRead(Buffer buffer, int id) throws IOException {
  }
  }
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_READ, handle, offset, requestedLength);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_READ, Handle.safe(handle), offset, requestedLength);
  return;
  }
 
@@ -597,13 +598,13 @@ protected abstract int doRead(
  throws IOException;
 
  protected void doWrite(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  long offset = buffer.getLong();
  int length = buffer.getInt();
  try {
  doWrite(id, handle, offset, length, buffer.array(), buffer.rpos(), buffer.available());
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_WRITE, handle, offset, length);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_WRITE, Handle.safe(handle), offset, length);
  return;
  }
 
@@ -683,7 +684,7 @@ protected void doSetStat(
  }
 
  protected void doFStat(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  int flags = SftpConstants.SSH_FILEXFER_ATTR_ALL;
  int version = getVersion();
  if (version >= SftpConstants.SFTP_V4) {
@@ -694,7 +695,7 @@ protected void doFStat(Buffer buffer, int id) throws IOException {
  try {
  attrs = doFStat(id, handle, flags);
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_FSTAT, handle, flags);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_FSTAT, Handle.safe(handle), flags);
  return;
  }
 
@@ -704,12 +705,12 @@ protected void doFStat(Buffer buffer, int id) throws IOException {
  protected abstract Map<String, Object> doFStat(int id, String handle, int flags) throws IOException;
 
  protected void doFSetStat(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  Map<String, Object> attrs = readAttrs(buffer);
  try {
  doFSetStat(id, handle, attrs);
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_FSETSTAT, handle, attrs);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_FSETSTAT, Handle.safe(handle), attrs);
  return;
  }
 
@@ -871,14 +872,14 @@ protected SpaceAvailableExtensionInfo doSpaceAvailable(int id, String path) thro
  }
 
  protected void doTextSeek(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  long line = buffer.getLong();
  try {
  // TODO : implement text-seek - see https://tools.ietf.org/html/draft-ietf-secsh-filexfer-03#section-6.3
  doTextSeek(id, handle, line);
  } catch (IOException | RuntimeException e) {
  sendStatus(prepareReply(buffer), id, e,
- SftpConstants.SSH_FXP_EXTENDED, SftpConstants.EXT_TEXT_SEEK, handle, line);
+ SftpConstants.SSH_FXP_EXTENDED, SftpConstants.EXT_TEXT_SEEK, Handle.safe(handle), line);
  return;
  }
 
@@ -889,12 +890,12 @@ protected void doTextSeek(Buffer buffer, int id) throws IOException {
 
  // see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL section 10
  protected void doOpenSSHFsync(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  try {
  doOpenSSHFsync(id, handle);
  } catch (IOException | RuntimeException e) {
  sendStatus(prepareReply(buffer), id, e,
- SftpConstants.SSH_FXP_EXTENDED, FsyncExtensionParser.NAME, handle);
+ SftpConstants.SSH_FXP_EXTENDED, FsyncExtensionParser.NAME, Handle.safe(handle));
  return;
  }
 
@@ -1340,15 +1341,15 @@ protected void doCopyFile(
  }
 
  protected void doBlock(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  long offset = buffer.getLong();
  long length = buffer.getLong();
  int mask = buffer.getInt();
 
  try {
  doBlock(id, handle, offset, length, mask);
  } catch (IOException | RuntimeException e) {
- sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_BLOCK, handle, offset, length, mask);
+ sendStatus(prepareReply(buffer), id, e, SftpConstants.SSH_FXP_BLOCK, Handle.safe(handle), offset, length, mask);
  return;
  }
 
@@ -1360,14 +1361,14 @@ protected abstract void doBlock(
  throws IOException;
 
  protected void doUnblock(Buffer buffer, int id) throws IOException {
- String handle = buffer.getString();
+ String handle = buffer.getString(StandardCharsets.ISO_8859_1);
  long offset = buffer.getLong();
  long length = buffer.getLong();
  try {
  doUnblock(id, handle, offset, length);
  } catch (IOException | RuntimeException e) {
  sendStatus(prepareReply(buffer), id, e,
- SftpConstants.SSH_FXP_UNBLOCK, handle, offset, length);
+ SftpConstants.SSH_FXP_UNBLOCK, Handle.safe(handle), offset, length);
  return;
  }
 
@@ -2157,7 +2158,7 @@ protected void appendSupported2Extension(Buffer buffer, Collection<String> extra
  protected void sendHandle(Buffer buffer, int id, String handle) throws IOException {
  buffer.putByte((byte) SftpConstants.SSH_FXP_HANDLE);
  buffer.putInt(id);
- buffer.putString(handle);
+ buffer.putString(handle, StandardCharsets.ISO_8859_1);
  send(buffer);
  }
 

sshd-sftp/src/main/java/org/apache/sshd/sftp/server/Handle.java

@@ -19,6 +19,7 @@
 package org.apache.sshd.sftp.server;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.Collection;
 import java.util.Collections;
@@ -32,6 +33,7 @@
 import org.apache.sshd.common.AttributeRepository;
 import org.apache.sshd.common.AttributeStore;
 import org.apache.sshd.common.util.ValidateUtils;
+import org.apache.sshd.common.util.buffer.BufferUtils;
 import org.apache.sshd.server.session.ServerSession;
 
 /**
@@ -47,7 +49,7 @@ public abstract class Handle implements java.nio.channels.Channel, AttributeStor
  protected Handle(SftpSubsystem subsystem, Path file, String handle) {
  this.sftpSubsystem = Objects.requireNonNull(subsystem, "No subsystem instance provided");
  this.file = Objects.requireNonNull(file, "No local file path");
- this.handle = ValidateUtils.checkNotNullAndNotEmpty(handle, "No assigned handle for %s", file);
+ this.handle = ValidateUtils.checkNotNull(handle, "No assigned handle for %s", file);
  }
 
  protected SftpSubsystem getSubsystem() {
@@ -72,6 +74,11 @@ public Path getFile() {
  return file;
  }
 
+ /**
+ * Retrieves the raw opaque file handle, which may contain characters not safe for printing.
+ *
+ * @return the raw file handle
+ */
  public String getFileHandle() {
  return handle;
  }
@@ -136,4 +143,21 @@ public void close() throws IOException {
  public String toString() {
  return Objects.toString(getFile());
  }
+
+ /**
+ * Converts a file handle, which may contain non-printable characters, to a hex representation of its bytes, which
+ * is safe to write to logs or exception messages.
+ * <p>
+ * For historical reasons, Apache MINA sshd represents file handles as strings internally.
+ * </p>
+ *
+ * @param handle to convert
+ * @return the printable handle string
+ */
+ protected static String safe(String handle) {
+ if (handle == null) {
+ return "null";
+ }
+ return BufferUtils.toHex(BufferUtils.EMPTY_HEX_SEPARATOR, handle.getBytes(StandardCharsets.ISO_8859_1));
+ }
 }