Re: setuid/setgid return values not checked in rlogin, rsh, rshd and uucpd

2023-07-31 Thread Jeffrey
> 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

2023-07-31 Thread Simon Josefsson via Bug reports for the GNU Internet utilities
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

2023-07-24 Thread Jeffrey
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

2023-07-22 Thread Simon Josefsson via Bug reports for the GNU Internet utilities
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

2023-07-03 Thread Simon Josefsson via Bug reports for the GNU Internet utilities
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

2023-07-01 Thread Jeffrey
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