diff mbox series

[v2] kernel/sys.c: fix and improve control flow in __sys_setres[ug]id()

Message ID 20230217162154.837549-1-omosnace@redhat.com
State New
Headers
Series [v2] kernel/sys.c: fix and improve control flow in __sys_setres[ug]id() |

Commit Message

Ondrej Mosnacek Feb. 17, 2023, 4:21 p.m. UTC 
  Linux Security Modules (LSMs) that implement the "capable" hook will
usually emit an access denial message to the audit log whenever they
"block" the current task from using the given capability based on their
security policy.

The occurrence of a denial is used as an indication that the given task
has attempted an operation that requires the given access permission, so
the callers of functions that perform LSM permission checks must take
care to avoid calling them too early (before it is decided if the
permission is actually needed to perform the requested operation).

The __sys_setres[ug]id() functions violate this convention by first
calling ns_capable_setid() and only then checking if the operation
requires the capability or not. It means that any caller that has the
capability granted by DAC (task's capability set) but not by MAC (LSMs)
will generate a "denied" audit record, even if is doing an operation for
which the capability is not required.

Fix this by reordering the checks such that ns_capable_setid() is
checked last and -EPERM is returned immediately if it returns false.

While there, also do two small optimizations:
* move the capability check before prepare_creds() and
* bail out early in case of a no-op.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v2: improve commit message

 kernel/sys.c | 69 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

Comments

Ondrej Mosnacek March 13, 2023, 1:15 p.m. UTC | #1 
On Fri, Feb 17, 2023 at 5:21 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Linux Security Modules (LSMs) that implement the "capable" hook will
> usually emit an access denial message to the audit log whenever they
> "block" the current task from using the given capability based on their
> security policy.
>
> The occurrence of a denial is used as an indication that the given task
> has attempted an operation that requires the given access permission, so
> the callers of functions that perform LSM permission checks must take
> care to avoid calling them too early (before it is decided if the
> permission is actually needed to perform the requested operation).
>
> The __sys_setres[ug]id() functions violate this convention by first
> calling ns_capable_setid() and only then checking if the operation
> requires the capability or not. It means that any caller that has the
> capability granted by DAC (task's capability set) but not by MAC (LSMs)
> will generate a "denied" audit record, even if is doing an operation for
> which the capability is not required.
>
> Fix this by reordering the checks such that ns_capable_setid() is
> checked last and -EPERM is returned immediately if it returns false.
>
> While there, also do two small optimizations:
> * move the capability check before prepare_creds() and
> * bail out early in case of a no-op.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v2: improve commit message
>
>  kernel/sys.c | 69 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 29 deletions(-)

Ping?
Andrew Morton March 13, 2023, 6:29 p.m. UTC | #2 
On Fri, 17 Feb 2023 17:21:54 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:

> Linux Security Modules (LSMs) that implement the "capable" hook will
> usually emit an access denial message to the audit log whenever they
> "block" the current task from using the given capability based on their
> security policy.
> 
> The occurrence of a denial is used as an indication that the given task
> has attempted an operation that requires the given access permission, so
> the callers of functions that perform LSM permission checks must take
> care to avoid calling them too early (before it is decided if the
> permission is actually needed to perform the requested operation).
> 
> The __sys_setres[ug]id() functions violate this convention by first
> calling ns_capable_setid() and only then checking if the operation
> requires the capability or not. It means that any caller that has the
> capability granted by DAC (task's capability set) but not by MAC (LSMs)
> will generate a "denied" audit record, even if is doing an operation for
> which the capability is not required.
> 
> Fix this by reordering the checks such that ns_capable_setid() is
> checked last and -EPERM is returned immediately if it returns false.
> 
> While there, also do two small optimizations:
> * move the capability check before prepare_creds() and
> * bail out early in case of a no-op.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Looks and sounds good to me, so I queued it up for some testing.  I'd
ask that someone more familiar with this code perform review, please.

I assume that you believe that a -stable backport is desirable?  I'll
add a cc:stable to the patch for now.
Ondrej Mosnacek March 14, 2023, 9:16 a.m. UTC | #3 
On Mon, Mar 13, 2023 at 7:29 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 17 Feb 2023 17:21:54 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> > Linux Security Modules (LSMs) that implement the "capable" hook will
> > usually emit an access denial message to the audit log whenever they
> > "block" the current task from using the given capability based on their
> > security policy.
> >
> > The occurrence of a denial is used as an indication that the given task
> > has attempted an operation that requires the given access permission, so
> > the callers of functions that perform LSM permission checks must take
> > care to avoid calling them too early (before it is decided if the
> > permission is actually needed to perform the requested operation).
> >
> > The __sys_setres[ug]id() functions violate this convention by first
> > calling ns_capable_setid() and only then checking if the operation
> > requires the capability or not. It means that any caller that has the
> > capability granted by DAC (task's capability set) but not by MAC (LSMs)
> > will generate a "denied" audit record, even if is doing an operation for
> > which the capability is not required.
> >
> > Fix this by reordering the checks such that ns_capable_setid() is
> > checked last and -EPERM is returned immediately if it returns false.
> >
> > While there, also do two small optimizations:
> > * move the capability check before prepare_creds() and
> > * bail out early in case of a no-op.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>
> Looks and sounds good to me, so I queued it up for some testing.  I'd
> ask that someone more familiar with this code perform review, please.
>
> I assume that you believe that a -stable backport is desirable?  I'll
> add a cc:stable to the patch for now.

Yes, it's a minor bug, but we hit it while testing on Fedora and it's
better for us to have the fix in stable kernels than adding a
workaround elsewhere.

Thanks,
diff mbox series

Patch

diff --git a/kernel/sys.c b/kernel/sys.c
index 5fd54bf0e8867..6fd88686cd06f 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -664,6 +664,7 @@  long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
 	struct cred *new;
 	int retval;
 	kuid_t kruid, keuid, ksuid;
+	bool ruid_new, euid_new, suid_new;
 
 	kruid = make_kuid(ns, ruid);
 	keuid = make_kuid(ns, euid);
@@ -678,25 +679,29 @@  long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
 	if ((suid != (uid_t) -1) && !uid_valid(ksuid))
 		return -EINVAL;
 
+	old = current_cred();
+
+	/* check for no-op */
+	if ((ruid == (uid_t) -1 || uid_eq(kruid, old->uid)) &&
+	    (euid == (uid_t) -1 || (uid_eq(keuid, old->euid) &&
+				    uid_eq(keuid, old->fsuid))) &&
+	    (suid == (uid_t) -1 || uid_eq(ksuid, old->suid)))
+		return 0;
+
+	ruid_new = ruid != (uid_t) -1        && !uid_eq(kruid, old->uid) &&
+		   !uid_eq(kruid, old->euid) && !uid_eq(kruid, old->suid);
+	euid_new = euid != (uid_t) -1        && !uid_eq(keuid, old->uid) &&
+		   !uid_eq(keuid, old->euid) && !uid_eq(keuid, old->suid);
+	suid_new = suid != (uid_t) -1        && !uid_eq(ksuid, old->uid) &&
+		   !uid_eq(ksuid, old->euid) && !uid_eq(ksuid, old->suid);
+	if ((ruid_new || euid_new || suid_new) &&
+	    !ns_capable_setid(old->user_ns, CAP_SETUID))
+		return -EPERM;
+
 	new = prepare_creds();
 	if (!new)
 		return -ENOMEM;
 
-	old = current_cred();
-
-	retval = -EPERM;
-	if (!ns_capable_setid(old->user_ns, CAP_SETUID)) {
-		if (ruid != (uid_t) -1        && !uid_eq(kruid, old->uid) &&
-		    !uid_eq(kruid, old->euid) && !uid_eq(kruid, old->suid))
-			goto error;
-		if (euid != (uid_t) -1        && !uid_eq(keuid, old->uid) &&
-		    !uid_eq(keuid, old->euid) && !uid_eq(keuid, old->suid))
-			goto error;
-		if (suid != (uid_t) -1        && !uid_eq(ksuid, old->uid) &&
-		    !uid_eq(ksuid, old->euid) && !uid_eq(ksuid, old->suid))
-			goto error;
-	}
-
 	if (ruid != (uid_t) -1) {
 		new->uid = kruid;
 		if (!uid_eq(kruid, old->uid)) {
@@ -761,6 +766,7 @@  long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
 	struct cred *new;
 	int retval;
 	kgid_t krgid, kegid, ksgid;
+	bool rgid_new, egid_new, sgid_new;
 
 	krgid = make_kgid(ns, rgid);
 	kegid = make_kgid(ns, egid);
@@ -773,23 +779,28 @@  long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
 	if ((sgid != (gid_t) -1) && !gid_valid(ksgid))
 		return -EINVAL;
 
+	old = current_cred();
+
+	/* check for no-op */
+	if ((rgid == (gid_t) -1 || gid_eq(krgid, old->gid)) &&
+	    (egid == (gid_t) -1 || (gid_eq(kegid, old->egid) &&
+				    gid_eq(kegid, old->fsgid))) &&
+	    (sgid == (gid_t) -1 || gid_eq(ksgid, old->sgid)))
+		return 0;
+
+	rgid_new = rgid != (gid_t) -1        && !gid_eq(krgid, old->gid) &&
+		   !gid_eq(krgid, old->egid) && !gid_eq(krgid, old->sgid);
+	egid_new = egid != (gid_t) -1        && !gid_eq(kegid, old->gid) &&
+		   !gid_eq(kegid, old->egid) && !gid_eq(kegid, old->sgid);
+	sgid_new = sgid != (gid_t) -1        && !gid_eq(ksgid, old->gid) &&
+		   !gid_eq(ksgid, old->egid) && !gid_eq(ksgid, old->sgid);
+	if ((rgid_new || egid_new || sgid_new) &&
+	    !ns_capable_setid(old->user_ns, CAP_SETGID))
+		return -EPERM;
+
 	new = prepare_creds();
 	if (!new)
 		return -ENOMEM;
-	old = current_cred();
-
-	retval = -EPERM;
-	if (!ns_capable_setid(old->user_ns, CAP_SETGID)) {
-		if (rgid != (gid_t) -1        && !gid_eq(krgid, old->gid) &&
-		    !gid_eq(krgid, old->egid) && !gid_eq(krgid, old->sgid))
-			goto error;
-		if (egid != (gid_t) -1        && !gid_eq(kegid, old->gid) &&
-		    !gid_eq(kegid, old->egid) && !gid_eq(kegid, old->sgid))
-			goto error;
-		if (sgid != (gid_t) -1        && !gid_eq(ksgid, old->gid) &&
-		    !gid_eq(ksgid, old->egid) && !gid_eq(ksgid, old->sgid))
-			goto error;
-	}
 
 	if (rgid != (gid_t) -1)
 		new->gid = krgid;