OpenSSH ‘sftp-server’ Remote Security Vulnerability

In the ChangeLog of OpenSSH 7.6 there is only one security fix, and I couldn’t find any associated CVE ID to it. It is basically a design flaw of sftp-server when in read-only mode. Theoretically, read-only should not allow any write operations but Michal Zalewski discovered that you can create zero-length files.

 * sftp-server(8): in read-only mode, sftp-server was incorrectly
   permitting creation of zero-length files. Reported by Michal
   Zalewski.

The vulnerability was located in sftp-server.c source code file and it affected all versions from OpenSSH 5.5 until 7.6 where it was fixed. If the SFTP server starts with the -R flag, it will enable the “readonly” variable as you can see below. This indicates that the clients are not allowed to perform and write operations.

int
sftp_server_main(int argc, char **argv, struct passwd *user_pw)
{
	...
	while (!skipargs && (ch = getopt(argc, argv,
	    "d:f:l:P:p:Q:u:cehR")) != -1) {
		switch (ch) {
	...
		case 'R':
			readonly = 1;
			break;
	...
}

The file opening for SFTP server is performed via process_open() helper function which was opening the files for read-only mode as shown below.

static void
process_open(u_int32_t id)
{
	...
	if (readonly &&
	    ((flags & O_ACCMODE) == O_WRONLY ||
	    (flags & O_ACCMODE) == O_RDWR) != 0)) {
		verbose("Refusing open request in read-only mode");
		status = SSH2_FX_PERMISSION_DENIED;
	} else {
		fd = open(name, flags, mode);
	...
}

As you can see, if “readonly” flag is enabled, it will check if the opening flags “WRITE ONLY” or “READ/WRITE” exist and if this is the case it will error out with “Refusing open request in read-only mode”. Otherwise, it will open the file using open() system call. This looks correct but it doesn’t take into account other argument flags of open() system call such as O_CREAT (create file) or O_TRUNC (truncate file). Those were the flags that M. Zalewski used to create arbitrary files in a read-only SFTP server. To fix this, OpenSSH developers changed the logic of this check to ensure that it will error out if the flag is not O_RDONLY (Read-only) or it includes O_CREAT or O_TRUNC flags. You can see the patch below.

 	if (readonly &&
-	    ((flags & O_ACCMODE) == O_WRONLY ||
-	    (flags & O_ACCMODE) == O_RDWR)) {
+	    ((flags & O_ACCMODE) != O_RDONLY ||
+	    (flags & (O_CREAT|O_TRUNC)) != 0)) {
 		verbose("Refusing open request in read-only mode");