[jira] [Commented] (SSHD-775) SftpSubSystem::sendStatus leaks Exception information

["Mark Ebbers (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SSHD-775?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16194458#comment-16194458 ] 

Mark Ebbers commented on SSHD-775:
----------------------------------

I did not find any guidelines regarding the 'error message' string part of the SSH_FX_STATUS response message and the latest draft RFC only stated the following:

{quote}
*9.1.  Status Response*
 error message
      Human readable description of the error.
{quote}

So I did a quick investigation of the source of the latest version of OpenSSH and it appears that OpenSSH only returns a small set of error messages.

{code:java}
Openssh-7.2p2/sftp-server.c
status_to_message(u_int32_t status)
{
	const char *status_messages[] = {
		"Success",			/* SSH_FX_OK */
		"End of file",			/* SSH_FX_EOF */
		"No such file",			/* SSH_FX_NO_SUCH_FILE */
		"Permission denied",		/* SSH_FX_PERMISSION_DENIED */
		"Failure",			/* SSH_FX_FAILURE */
		"Bad message",			/* SSH_FX_BAD_MESSAGE */
		"No connection",		/* SSH_FX_NO_CONNECTION */
		"Connection lost",		/* SSH_FX_CONNECTION_LOST */
		"Operation unsupported",	/* SSH_FX_OP_UNSUPPORTED */
		"Unknown error"			/* Others */
	};
	return (status_messages[MIN(status,SSH2_FX_MAX)]);
}

openssh-7.2p2/sftp-common.c
/* Convert from SSH2_FX_ status to text error message */
const char *
fx2txt(int status)
{
	switch (status) {
	case SSH2_FX_OK:
		return("No error");
	case SSH2_FX_EOF:
		return("End of file");
	case SSH2_FX_NO_SUCH_FILE:
		return("No such file or directory");
	case SSH2_FX_PERMISSION_DENIED:
		return("Permission denied");
	case SSH2_FX_FAILURE:
		return("Failure");
	case SSH2_FX_BAD_MESSAGE:
		return("Bad message");
	case SSH2_FX_NO_CONNECTION:
		return("No connection");
	case SSH2_FX_CONNECTION_LOST:
		return("Connection lost");
	case SSH2_FX_OP_UNSUPPORTED:
		return("Operation unsupported");
	default:
		return("Unknown status");
	}
	/* NOTREACHED */
}
{code}

Based on the source of OpenSSH I suggest two possible options:

h3. Option 1
{code:java}
public static String resolveStatusMessage(final Throwable t) {
        if (t == null) {
            return "";
        } else if ((t instanceof NoSuchFileException) || (t instanceof FileNotFoundException)) {
            return "No such file or directory";
        } else if (t instanceof InvalidHandleException) {
            return "Invalid handle";
        } else if (t instanceof FileAlreadyExistsException) {
            return "File already exists";
        } else if (t instanceof DirectoryNotEmptyException) {
            return "Directory not empty";
        } else if (t instanceof NotDirectoryException) {
            return "Not a directory";
        } else if (t instanceof AccessDeniedException) {
            return "Permission denied";
        } else if (t instanceof EOFException) {
            return "End of file";
        } else if (t instanceof OverlappingFileLockException) {
            return "Lock conflict";
        } else if (t instanceof UnsupportedOperationException) {
            return "Operation unsupported";
        } else if (t instanceof InvalidPathException) {
            return "Invalid filename";
        } else if (t instanceof IllegalArgumentException) {
            return "Invalid parameter";
        } else if (t instanceof UnsupportedOperationException) {
            return "Operation unsupported";
        } else if (t instanceof UserPrincipalNotFoundException) {
            return "Unknown principal";
        } else if (t instanceof FileSystemLoopException) {
            return "Link loop";
        } else if (t instanceof SftpException) {
            return "Failure";
        }

        return "Unknown error";
    }
{code}

h4. Motivation
* This version is somewhat more verbose and clearer to the client in the case something goes wrong than OpenSSH.
* No use of the exception message itself to prevent information leakage. Also not needed. For example the NoSuchFileException has a reference to the file which could not be found, but the client already knowns which file because it initiated the request.
* The SFTPException can be a problem. For example see _SftpSubSystem::doRemove(): throw new SftpException(SftpConstants.SSH_FX_FILE_IS_A_DIRECTORY, p.toString() + " is a folder");_ If this is the case the client will receive a "Failure". However the client also received the substatus SSH_FX_FILE_IS_A_DIRECTORY....

h3. Option 2
{code:java}
 private static String resolveSubStatusMessage(int status) {

        switch (status) {
            case SSH_FX_OK:
                return "Success";
            case SSH_FX_EOF:
                return "End of file";
            case SSH_FX_NO_SUCH_FILE:
                return "No such file or directory";
            case SSH_FX_PERMISSION_DENIED:
                return "Permission denied";
            case SSH_FX_FAILURE:
                return "Failure";
            case SSH_FX_OP_UNSUPPORTED:
                return "Operation unsupported";
        }

        return "Unknown error";
    }
{code}

h4. Motivation
* Inline with OpenSSH.
* Based on substatus instead of Throwable.
* Does not make the client smarter then needed.
* One might consider to also use this option to resolve a message for _SftpException.getStatus()_.

I go for option 2 in my project. I think the combination of status + substatus is already enough information for the client especially with option 2.

h4. Sources
*SFTP RFC draft 13*
* https://tools.ietf.org/html/draft-ietf-secsh-filexfer-13
* 9.1.  Status Response
* 11.  Implementation Considerations
* 13.  Security Considerations

*OpenSSH 7.2p2 source code*
* http://archive.ubuntu.com/ubuntu/pool/main/o/openssh/openssh_7.2p2.orig.tar.gz

> SftpSubSystem::sendStatus leaks Exception information
> -----------------------------------------------------
>
>                 Key: SSHD-775
>                 URL: https://issues.apache.org/jira/browse/SSHD-775
>             Project: MINA SSHD
>          Issue Type: Improvement
>    Affects Versions: 1.6.0
>            Reporter: Mark Ebbers
>            Priority: Minor
>              Labels: security
>
> I'm using SSHD-core 1.6.0 in my own Sftp server implementation and make use of the rooted file-system. Now did I notice that a client did try to rename a file, which was no longer available, and got a response with the substatus SSH_FX_NO_SUCH_FILE and the message ' Internal NoSuchFileException: /srv/sftp/chroot/11738/file.txt'.
> As a client I now know the following two things:
> * The full path on the file-system.
> * The server was written in Java. (NoSuchFileException)
> I noticed that the SftpSubsystem.sendStatus(Buffer, int, Throwable) uses the SftpHelper.resolveStatusMessage() method to create a message string to be send to the client without further checking what information is inside the Exception message. 



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)