[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    [PATCH] check_suid: verify permissions to `setres{g,u}id`
From:       Leif Middelschulte <leif.middelschulte () klsmartin ! com>
Date:       2019-11-22 12:19:21
Message-ID: 20191122121921.6031-1-leif.middelschulte () klsmartin ! com
[Download RAW message or body]

The documentation states the following for `x` mode:
> x: USER/GROUP/others are allowed to execute APPLET.
>    No UID/GID change will be done when it is run.

Yet, even if a user was only allowed to execute a binary, the
effective user and group would be set to the
defined USER.GROUP values.

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 libbb/appletlib.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/libbb/appletlib.c b/libbb/appletlib.c
index f842e73cc..5f5c91ea6 100644
--- a/libbb/appletlib.c
+++ b/libbb/appletlib.c
@@ -615,6 +615,7 @@ static void check_suid(int applet_no)
 		uid_t uid;
 		struct suid_config_t *sct;
 		mode_t m;
+		bool executed_by_other = true;
 
 		for (sct = suid_config; sct; sct = sct->m_next) {
 			if (sct->m_applet == applet_no)
@@ -624,12 +625,16 @@ static void check_suid(int applet_no)
  found:
 		/* Is this user allowed to run this applet? */
 		m = sct->m_mode;
-		if (sct->m_ugid.uid == ruid)
+		if (sct->m_ugid.uid == ruid) {
 			/* same uid */
 			m >>= 6;
-		else if ((sct->m_ugid.gid == rgid) || ingroup(ruid, sct->m_ugid.gid))
+			executed_by_other = false;
+		}
+		else if ((sct->m_ugid.gid == rgid) || ingroup(ruid, sct->m_ugid.gid)) {
 			/* same group / in group */
 			m >>= 3;
+			executed_by_other = false;
+		}
 		if (!(m & S_IXOTH)) /* is x bit not set? */
 			bb_simple_error_msg_and_die("you have no permission to run this applet");
 
@@ -639,7 +644,7 @@ static void check_suid(int applet_no)
 		/* Are we directed to change gid
 		 * (APPLET = *s* USER.GROUP or APPLET = *S* USER.GROUP)?
 		 */
-		if (sct->m_mode & S_ISGID)
+		if ((sct->m_mode & S_ISGID) && !executed_by_other)
 			rgid = sct->m_ugid.gid;
 		/* else: we will set egid = rgid, thus dropping sgid effect */
 		if (setresgid(-1, rgid, rgid))
@@ -649,7 +654,7 @@ static void check_suid(int applet_no)
 		 * (APPLET = s** USER.GROUP or APPLET = S** USER.GROUP)?
 		 */
 		uid = ruid;
-		if (sct->m_mode & S_ISUID)
+		if ((sct->m_mode & S_ISUID) && !executed_by_other)
 			uid = sct->m_ugid.uid;
 		/* else: we will set euid = ruid, thus dropping suid effect */
 		if (setresuid(-1, uid, uid))
-- 
2.23.0

_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]