system/2912: Little security improvements for ftpd

From: <j(at)pureftpd.org>
Date: Wed Sep 04 2002 - 16:15:04 EDT

>Number: 2912
>Category: system
>Synopsis: Little security improvements for ftpd
>Confidential: no

	System      : OpenBSD 3.1
	Architecture: OpenBSD.i386
	Machine     : i386

>Description:

ftpd(8) regains effective root privileges too much.

• There's no need to seteuid(0) before ftpdlogwtmp() except the very first time (before chroot(), so we already are root, no need to seteuid()) . The purpose of ftpdlogwtmp() is to keep the file open. wtmp/utmp logging works perfectly without seteuid(0) .
• In getdatasock(), seteuid(0) is made too early. Only bind() needs it.
• Just after the pasv_error label, there's also absolutely no need to call seteuid(pw->pw_uid) in any way, as no seteuid(0) ever occurs in the function (and there's no reason to ever do it) .
• In long_passive(), bind() is done after seteuid(0). Why? We don't bind any privileged port. seteuid(0) can be removed, and everything's fine.
• exit(0) in end_login() . This is always very risky to 'reset' the state to switch from one user to another one. If we forget to reset anything, bad things can happen. If an user wants to change his identity, he just has to open another session. Dot.

We end up with only _one_ place where seteuid(0) is called : to bind port 20 for active data sockets. This is not needed. Even though port 20 is known as 'ftp-data', RFCs don't demand port 20. There are plenty of FTP servers that don't use port 20 for outgoing connections, like DJB's publicfile, OpenFTPd, and most Windows FTP servers. No client choke on this AFAIK. Port 20 is just handy for firewalling, if we want to filter outgoing connections.

Missing from the following patch because your mileage may vary :

We can replace the initial seteuid() call (line 1066 in pass()), by a plain setuid() call, and replace data_source.su_port = htons(20) in getdatasock() by data_source.su_port = 0, and then drop all remaining seteuid() calls. The result is a perfectly working daemon, with only two commands running as root (user and pass, and it will work only once at startup), and everything else with real user privileges.

>How-To-Repeat:

-
>Fix:

diff -u libexec/ftpd.orig/ftpd.c libexec/ftpd/ftpd.c

--- libexec/ftpd.orig/ftpd.c	Fri Aug 30 00:51:38 2002
+++ libexec/ftpd/ftpd.c	Wed Sep  4 22:08:56 2002

@@ -862,7 +862,6 @@
 {  

 	sigprocmask (SIG_BLOCK, &allsigs, NULL);
-	(void) seteuid((uid_t)0);
 	if (logged_in) {
 		ftpdlogwtmp(ttyline, "", "");
 		if (doutmp)
@@ -876,9 +875,8 @@
 		syslog(LOG_NOTICE, "setusercontext: %m");
 		exit(1);
 	}
-	logged_in = 0;
-	guest = 0;
-	dochroot = 0;

+ reply(530, "Please reconnect to work as another user"); + exit(0);
 }  

 void
@@ -1300,7 +1298,6 @@

 	if (data >= 0)
 		return (fdopen(data, mode));
 	sigprocmask (SIG_BLOCK, &allsigs, NULL);
-	(void) seteuid((uid_t)0);
 	s = socket(ctrl_addr.su_family, SOCK_STREAM, 0);
 	if (s < 0)
 		goto bad;
@@ -1310,12 +1307,15 @@
 	/* anchor socket to avoid multi-homing problems */
 	data_source = ctrl_addr;
 	data_source.su_port = htons(20); /* ftp-data port */
+	(void) seteuid((uid_t)0);    
 	for (tries = 1; ; tries++) {
 		if (bind(s, (struct sockaddr *)&data_source,
 		    data_source.su_len) >= 0)
 			break;
-		if (errno != EADDRINUSE || tries > 10)
+		if (errno != EADDRINUSE || tries > 10) {
+            (void) seteuid((uid_t)pw->pw_uid);
 			goto bad;
+        }
 		sleep(tries);
 	}
 	(void) seteuid((uid_t)pw->pw_uid);

@@ -1350,7 +1350,6 @@
 bad:

 	/* Return the real value of errno (close may change it) */
 	t = errno;
-	(void) seteuid((uid_t)pw->pw_uid);
 	sigprocmask (SIG_UNBLOCK, &allsigs, NULL);
 	(void) close(s);
 	errno = t;

@@ -2152,7 +2151,6 @@  

 	if (logged_in) {
 		sigprocmask(SIG_BLOCK, &allsigs, NULL);
-		(void) seteuid((uid_t)0);
 		ftpdlogwtmp(ttyline, "", "");
 		if (doutmp)
 			logout(utmp.ut_line);
@@ -2256,7 +2254,6 @@
 	return;
 
 pasv_error:
-	(void) seteuid((uid_t)pw->pw_uid);
 	(void) close(pdata);
 	pdata = -1;
 	perror_reply(425, "Can't open passive connection");

@@ -2382,12 +2379,9 @@  

 	pasv_addr = ctrl_addr;
 	pasv_addr.su_port = 0;
-	(void) seteuid((uid_t) 0);
 	if (bind(pdata, (struct sockaddr *) &pasv_addr, pasv_addr.su_len) < 0) {
Do you need more help?
In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software:
Learn more about Pantek's Technical Support offerings
Place an order for 24/7/365 Technical Support here
Learn more about Pantek, who we are, and what we do
Chat with a Live Operator
Related openbsd.org Links
advocacy
announce
bugs
ipv6
misc
ports
ports-changes
security-announce
smp
source-changes
tech
Request more information about Pantek
-		(void) seteuid((uid_t) pw->pw_uid);
 		goto pasv_error;
 	}
-	(void) seteuid((uid_t) pw->pw_uid);
 	len = pasv_addr.su_len;
 	if (getsockname(pdata, (struct sockaddr *) &pasv_addr, &len) < 0)
 		goto pasv_error;

>Release-Note:
Received on Thu Nov 7 15:45:22 2002