openssh unix dev - Nov 2010 - SFTP subsystem and umask

Hello,

I have noticed that the -u parameter to the sftp-server or internal-sftp
subsystem is not working correctly.  For openssh-5.6p1 I believe that the
problem lies in this code, starting at line 1414 in sftp-server.c:
----------------------------------------------------------
case 'u':
        mask = (mode_t)strtonum(optarg, 0, 0777, &errmsg);
        if (errmsg != NULL)
                fatal("Invalid umask \"%s\": %s",
                    optarg, errmsg);
        (void)umask(mask);
        break;
----------------------------------------------------------

I think that adherence to strtonum() in this instance causes unexpected results
due to "mask" being set to decimal/base 10.

For example, say you had the following in sshd_config:
Subsystem       sftp    /usr/local/libexec/sftp-server -u 022

Then say you upload a file with permissions of 777 via sftp -p, you would expect
the uploaded file to end up with permission of 755, right?  In this case you get
a file with permissions of 751 or -rwxr-x--x.  Why?

I believe it is because decimal 22 == octal 026.  Further compounding the
problem is that sftp-server doggedly insists upon accepting permissions from the
sftp client before it applies umask.

To test, set the following in your sshd_config:
Subsystem       sftp    /your/path/to/sftp-server -u 18

Then kill -HUP sshd and create a file on the client with permissions of 777. 
Upload the file via sftp -p and observe permissions of the file created by
sftp-server. I ended up with a file with permissions of 755.  I think it's
because decimal 18 == octal 022.....

Is the solution to use strtol() or strtoul()?  You would end up having to trust
the user to use sane values in sshd_config, or trust that a user will run
sftp-server with sane parameters.

I tested using RHEL4 and Ubuntu 10.04.

Best regards,

Rob Candland

On Tue, 2 Nov 2010, Rob C wrote:
> Hello,
> 
> I have noticed that the -u parameter to the sftp-server or
> internal-sftp subsystem is not working correctly. For openssh-5.6p1 I
> believe that the problem lies in this code, starting at line 1414 in
> sftp-server.c:
>
> ----------------------------------------------------------
> case 'u':
>         mask = (mode_t)strtonum(optarg, 0, 0777, &errmsg);
>         if (errmsg != NULL)
>                 fatal("Invalid umask \"%s\": %s",
>                     optarg, errmsg);
>         (void)umask(mask);
>         break;
> ----------------------------------------------------------
Yep, that is completely broken. Please try this patch:


Index: sftp-server.c
==================================================================RCS file:
/cvs/src/usr.bin/ssh/sftp-server.c,v
retrieving revision 1.91
diff -u -p -r1.91 sftp-server.c
--- sftp-server.c	13 Jan 2010 01:40:16 -0000	1.91
+++ sftp-server.c	3 Nov 2010 01:52:50 -0000
@@ -1349,8 +1349,7 @@ sftp_server_main(int argc, char **argv, 
 	ssize_t len, olen, set_size;
 	SyslogFacility log_facility = SYSLOG_FACILITY_AUTH;
 	char *cp, buf[4*4096];
-	const char *errmsg;
-	mode_t mask;
+	long mask;
 
 	extern char *optarg;
 	extern char *__progname;
@@ -1383,11 +1382,12 @@ sftp_server_main(int argc, char **argv, 
 				error("Invalid log facility \"%s\"", optarg);
 			break;
 		case 'u':
-			mask = (mode_t)strtonum(optarg, 0, 0777, &errmsg);
-			if (errmsg != NULL)
-				fatal("Invalid umask \"%s\": %s",
-				    optarg, errmsg);
-			(void)umask(mask);
+			errno = 0;
+			mask = strtol(optarg, &cp, 8);
+			if (mask < 0 || mask > 0777 || *cp != '\0' ||
+			    cp == optarg || (mask == 0 && errno != 0))
+				fatal("Invalid umask \"%s\"", optarg);
+			(void)umask((mode_t)mask);
 			break;
 		case 'h':
 		default: