Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
> Thank you! I have pushed this to git. Next time, please run 'make > syntax-check' to check your patches (code indentation caused troubles > now, but I fixed it) and feel free to include the NEWS blurb in the git > patch itself. > Thank you. I will. > I'll reach out to the netbsd-tnftpd folks to see if they are interested, > looks like their ftpd has similar issues. > Sure thing.
Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
Jeffrey writes: > Patch attached. Thank you! I have pushed this to git. Next time, please run 'make syntax-check' to check your patches (code indentation caused troubles now, but I fixed it) and feel free to include the NEWS blurb in the git patch itself. I'll reach out to the netbsd-tnftpd folks to see if they are interested, looks like their ftpd has similar issues. /Simon signature.asc Description: PGP signature
Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
Patch attached. Not sure what should be a headline or not in NEWS (release numbers etc.). Find below a proposal for such entry: ** ftpd, rcp, rlogin, rsh, rshd, uucpd *** Avoid potential privilege escalations due to absence of checking set*id() return values. Reported by Jeffrey Bencteux in < https://lists.gnu.org/archive/html/bug-inetutils/2023-07/msg0.html>. -- Jeffrey BENCTEUX Le sam. 22 juil. 2023 à 10:36, Simon Josefsson a écrit : > Jeffrey writes: > > > I found more occurences of unchecked values for set*id() functions in > other > > inetutils programs: ftpd, rcp. > > > > It has different security impact if it can be triggered: > > > > * rcp: local privilege escalation to the user running the binary > > * ftpd: undefined behaviour without privilege escalation as all calls are > > to seteuid(0) (gaining root privileges, not dropping it) > > > > I am attaching a consolidated patch to fix these and the previous ones. > > Thanks again -- copyright papers have now arrived, and I looked at the > patch, and it seems good. However the patch does not apply cleanly due > to whitespace and line-wrapping problems, can you re-send the patch as > an attachment instead of inline in your email? Please also add NEWS > entries (look at earlier entries as templates). > > /Simon > 0001-ftpd-rcp-rlogin-rsh-rshd-uucpd-fix-check-set-id-retu.patch Description: Binary data
Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
Jeffrey writes: > I found more occurences of unchecked values for set*id() functions in other > inetutils programs: ftpd, rcp. > > It has different security impact if it can be triggered: > > * rcp: local privilege escalation to the user running the binary > * ftpd: undefined behaviour without privilege escalation as all calls are > to seteuid(0) (gaining root privileges, not dropping it) > > I am attaching a consolidated patch to fix these and the previous ones. Thanks again -- copyright papers have now arrived, and I looked at the patch, and it seems good. However the patch does not apply cleanly due to whitespace and line-wrapping problems, can you re-send the patch as an attachment instead of inline in your email? Please also add NEWS entries (look at earlier entries as templates). /Simon signature.asc Description: PGP signature
Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
Thank you Jeffrey, have you signed the copyright assignment form? I'll email it to you privately. /Simon signature.asc Description: PGP signature
Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
I found more occurences of unchecked values for set*id() functions in other
inetutils programs: ftpd, rcp.
It has different security impact if it can be triggered:
* rcp: local privilege escalation to the user running the binary
* ftpd: undefined behaviour without privilege escalation as all calls are
to seteuid(0) (gaining root privileges, not dropping it)
I am attaching a consolidated patch to fix these and the previous ones.
---
>From 05bca16ab557abe18c9deca0e64e2ce5a43cb875 Mon Sep 17 00:00:00 2001
From: Jeffrey Bencteux
Date: Fri, 30 Jun 2023 19:02:45 +0200
Subject: [PATCH] ftpd,rcp,rlogin,rsh,rshd,uucpd: fix: check set*id() return
values
Several setuid(), setgid(), seteuid() and setguid() return values
were not checked in ftpd/rcp/rlogin/rsh/rshd/uucpd code potentially
leading to potential security issues.
---
ftpd/ftpd.c | 10 +++---
src/rcp.c| 39 +--
src/rlogin.c | 11 +--
src/rsh.c| 25 +
src/rshd.c | 20 +---
src/uucpd.c | 15 +--
6 files changed, 100 insertions(+), 20 deletions(-)
diff --git a/ftpd/ftpd.c b/ftpd/ftpd.c
index 92b2cca..28dd523 100644
--- a/ftpd/ftpd.c
+++ b/ftpd/ftpd.c
@@ -862,7 +862,9 @@ end_login (struct credentials *pcred)
char *remotehost = pcred->remotehost;
int atype = pcred->auth_type;
- seteuid ((uid_t) 0);
+ if (seteuid ((uid_t) 0) == -1)
+_exit (EXIT_FAILURE);
+
if (pcred->logged_in)
{
logwtmp_keep_open (ttyline, "", "");
@@ -1151,7 +1153,8 @@ getdatasock (const char *mode)
if (data >= 0)
return fdopen (data, mode);
- seteuid ((uid_t) 0);
+ if (seteuid ((uid_t) 0) == -1)
+_exit (EXIT_FAILURE);
s = socket (ctrl_addr.ss_family, SOCK_STREAM, 0);
if (s < 0)
goto bad;
@@ -1978,7 +1981,8 @@ passive (int epsv, int af)
else /* !AF_INET6 */
((struct sockaddr_in *) _addr)->sin_port = 0;
- seteuid ((uid_t) 0);
+ if (seteuid ((uid_t) 0) == -1)
+_exit (EXIT_FAILURE);
if (bind (pdata, (struct sockaddr *) _addr, pasv_addrlen) < 0)
{
if (seteuid ((uid_t) cred.uid))
diff --git a/src/rcp.c b/src/rcp.c
index 75adb25..cdcf850 100644
--- a/src/rcp.c
+++ b/src/rcp.c
@@ -345,14 +345,23 @@ main (int argc, char *argv[])
if (from_option)
{ /* Follow "protocol", send data. */
response ();
- setuid (userid);
+
+ if (setuid (userid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (setuid()
failed)");
+ }
+
source (argc, argv);
exit (errs);
}
if (to_option)
{ /* Receive data. */
- setuid (userid);
+ if (setuid (userid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (setuid()
failed)");
+ }
+
sink (argc, argv);
exit (errs);
}
@@ -537,7 +546,11 @@ toremote (char *targ, int argc, char *argv[])
if (response () < 0)
exit (EXIT_FAILURE);
free (bp);
- setuid (userid);
+
+ if (setuid (userid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges
(setuid() failed)");
+ }
}
source (1, argv + i);
close (rem);
@@ -630,7 +643,12 @@ tolocal (int argc, char *argv[])
++errs;
continue;
}
- seteuid (userid);
+
+ if (seteuid (userid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (seteuid()
failed)");
+ }
+
#if defined IP_TOS && defined IPPROTO_IP && defined IPTOS_THROUGHPUT
sslen = sizeof (ss);
(void) getpeername (rem, (struct sockaddr *) , );
@@ -643,7 +661,12 @@ tolocal (int argc, char *argv[])
#endif
vect[0] = target;
sink (1, vect);
- seteuid (effuid);
+
+ if (seteuid (effuid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (seteuid()
failed)");
+ }
+
close (rem);
rem = -1;
#ifdef SHISHI
@@ -1441,7 +1464,11 @@ susystem (char *s, int userid)
return (127);
case 0:
- setuid (userid);
+ if (setuid (userid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (setuid()
failed)");
+ }
+
execl (PATH_BSHELL, "sh", "-c", s, NULL);
_exit (127);
}
diff --git a/src/rlogin.c b/src/rlogin.c
index aa6426f..c543de0 100644
--- a/src/rlogin.c
+++ b/src/rlogin.c
@@ -647,8 +647,15 @@ try_connect:
/* Now change to the real user ID. We have to be set-user-ID root
to get the privileged port that rcmd () uses. We now want, however,
to run as the real user who invoked us. */
- seteuid (uid);
- setuid (uid);
+ if (seteuid (uid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (seteuid()
failed)");
+ }
+
+ if (setuid (uid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (setuid() failed)");
+ }
doit (); /* The old mask will activate SIGURG and SIGUSR1! */
diff --git a/src/rsh.c b/src/rsh.c
index 2d622ca..6f60667 100644
setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd
Hi,
Several setuid(), setgid(), seteuid() and setguid() return values are not
checked in rlogin/rsh/rshd/uucpd code:
rlogin.c:
647 /* Now change to the real user ID. We have to be set-user-ID root
648 to get the privileged port that rcmd () uses. We now want,
however,
649 to run as the real user who invoked us. */
650 seteuid (uid);
651 setuid (uid);
652
653 doit (); /* The old mask will activate SIGURG and
SIGUSR1! */
rsh.c:
274 /* If no further arguments, must have been called as rlogin. */
275 if (!argv[index])
276 {
277 if (asrsh)
278 *argv = (char *) "rlogin";
279 seteuid (getuid ());
280 setuid (getuid ());
281 execv (PATH_RLOGIN, argv);
282 error (EXIT_FAILURE, errno, "cannot execute %s", PATH_RLOGIN);
283 }
[...]
541 error (0, errno, "setsockopt DEBUG (ignored)");
542 }
543
544 seteuid (uid);
545 setuid (uid);
546 #ifdef HAVE_SIGACTION
547 sigemptyset ();
rshd.c:
1846 if (*pwd->pw_shell == '\0')
1847 pwd->pw_shell = PATH_BSHELL;
1848
1849 /* Set the gid, then uid to become the user specified by
"locuser" */
1850 setegid ((gid_t) pwd->pw_gid);
1851 setgid ((gid_t) pwd->pw_gid);
1852 #ifdef HAVE_INITGROUPS
1853 initgroups (pwd->pw_name, pwd->pw_gid); /* BSD groups */
[...]
1871 #endif /* WITH_PAM */
1872
1873 setuid ((uid_t) pwd->pw_uid);
1874
1875 /* We'll execute the client's command in the home directory
1876* of locuser. Note, that the chdir must be executed after
1877* setuid(), otherwise it may fail on NFS mounted directories
1878* (root mapped to nobody).
1879*/
1880 if (chdir (pwd->pw_dir) < 0)
uucpd.c:
252 snprintf (Username, sizeof (Username), "USER=%s", user);
253 snprintf (Logname, sizeof (Logname), "LOGNAME=%s", user);
254 dologin (pw, sap, salen);
255 setgid (pw->pw_gid); <=
256 #ifdef HAVE_INITGROUPS
257 initgroups (pw->pw_name, pw->pw_gid);
258 #endif
259 if (chdir (pw->pw_dir) < 0)
260 {
261 fprintf (stderr, "Login incorrect.");
262 return;
263 }
264 setuid (pw->pw_uid); <=
265 execl (uucico_location, "uucico", NULL);
266 perror ("uucico server: execl");
There are cases where set*id() functions can fail, for example multiple
calls to the clone() function can cause setuid() to fail when the user
process limit is reached.
man 2 setuid():
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and errno
is set to indicate the error.
Note: there are cases where setuid() can fail even when the caller
is UID 0; it is a grave security error to omit checking for a failure
return from setuid().
The above code could be abused in different ways to trigger such failures,
potentially remotely in the case of rshd and uucpd.
Here are some example scenarios:
* rshd: if daemon run as root, privilege escalation is possible as any user
logging in after a set*id() failure would have its session started as root.
* rlogin: potential local privilege escalation as the binary is setUID root
* uucpd: potential remote privilege escalation to root for already valid
users
I believe the below patch mitigates the issue, let me know if that suits
you.
Regards,
---
From: Jeffrey Bencteux
Date: Fri, 30 Jun 2023 19:02:45 +0200
Subject: [PATCH] rlogin,rsh,rshd,uucpd: fix: check set*id() return values
Several setuid(), setgid(), seteuid() and setguid() return values
were not checked in rlogin/rsh/rshd/uucpd code potentially
leading to security issues.
---
src/rlogin.c | 11 +--
src/rsh.c| 25 +
src/rshd.c | 20 +---
src/uucpd.c | 15 +--
4 files changed, 60 insertions(+), 11 deletions(-)
diff --git a/src/rlogin.c b/src/rlogin.c
index aa6426f..c543de0 100644
--- a/src/rlogin.c
+++ b/src/rlogin.c
@@ -647,8 +647,15 @@ try_connect:
/* Now change to the real user ID. We have to be set-user-ID root
to get the privileged port that rcmd () uses. We now want, however,
to run as the real user who invoked us. */
- seteuid (uid);
- setuid (uid);
+ if (seteuid (uid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (seteuid()
failed)");
+ }
+
+ if (setuid (uid) == -1)
+ {
+error (EXIT_FAILURE, 0, "Could not drop privileges (setuid() failed)");
+ }
doit (); /* The old mask will activate SIGURG and SIGUSR1! */
diff --git a/src/rsh.c b/src/rsh.c
index 2d622ca..6f60667 100644
--- a/src/rsh.c
+++ b/src/rsh.c
@@ -276,8 +276,17 @@ main (int argc, char **argv)
{
if (asrsh)
*argv = (char *) "rlogin";
- seteuid (getuid ());
- setuid (getuid ());
+
+ if (seteuid (getuid ()) == -1)
+ {
+error (EXIT_FAILURE, errno, "seteuid()
