sys/rpc: UNIX auth: Fix OOB accesses, notably writes on decode

When the received authentication message had more than XU_NGROUPS, we
would write group IDs beyond the end of cr_groups[] in the 'struct
xucred' being filled (as 'ngroups_max' is always greater than
XU_NGROUPS).

For robustness, prevent various OOB accesses that would result from
a change of value of XU_NGROUPS or a 'struct xucred' with an invalid
'cr_ngroups' field, even if these cases are unlikely.

Reviewed by:    rmacklem
Fixes:          dfdcada31e79 ("Add the new kernel-mode NFS Lock Manager.")
MFC after:      2 days
Sponsored by:   The FreeBSD Foundation
Differential Revision:  https://reviews.freebsd.org/D52960

1 files changed, 21 insertions, 19 deletions

diff --git a/sys/rpc/authunix_prot.c b/sys/rpc/authunix_prot.c
index f63a6d3f9dc6..89f0ab3ed44e 100644
--- a/sys/rpc/authunix_prot.c
+++ b/sys/rpc/authunix_prot.c
@@ -75,7 +75,6 @@ xdr_authunix_parms(XDR *xdrs, uint32_t *time, struct xucred *cred)
 	} else {
 		namelen = 0;
 	}
-	junk = 0;
 
 	if (!xdr_uint32_t(xdrs, time)
 	    || !xdr_uint32_t(xdrs, &namelen))
@@ -93,15 +92,25 @@ xdr_authunix_parms(XDR *xdrs, uint32_t *time, struct xucred *cred)
 
 	if (!xdr_uint32_t(xdrs, &cred->cr_uid))
 		return (FALSE);
+
+	/*
+	 * Safety check: The protocol needs at least one group (access to
+	 * 'cr_gid', decrementation of 'cr_ngroups' below).
+	 */
+	if (xdrs->x_op == XDR_ENCODE && cred->cr_ngroups == 0)
+		return (FALSE);
 	if (!xdr_uint32_t(xdrs, &cred->cr_gid))
 		return (FALSE);
 
 	if (xdrs->x_op == XDR_ENCODE) {
 		/*
-		 * Note that this is a `struct xucred`, which maintains its
-		 * historical layout of preserving the egid in cr_ngroups and
-		 * cr_groups[0] == egid.
+		 * Note that this is a 'struct xucred', which still has the
+		 * historical layout where the effective GID is in cr_groups[0]
+		 * and is accounted in 'cr_ngroups'.  We substract 1 to obtain
+		 * the number of "supplementary" groups, passed in the AUTH_SYS
+		 * credentials variable-length array called gids[] in RFC 5531.
 		 */
+		MPASS(cred->cr_ngroups <= XU_NGROUPS);
 		supp_ngroups = cred->cr_ngroups - 1;
 		if (supp_ngroups > NGRPS)
 			supp_ngroups = NGRPS;
@@ -109,22 +118,15 @@ xdr_authunix_parms(XDR *xdrs, uint32_t *time, struct xucred *cred)
 
 	if (!xdr_uint32_t(xdrs, &supp_ngroups))
 		return (FALSE);
-	for (i = 0; i < supp_ngroups; i++) {
-		if (i < ngroups_max) {
-			if (!xdr_uint32_t(xdrs, &cred->cr_groups[i + 1]))
-				return (FALSE);
-		} else {
-			if (!xdr_uint32_t(xdrs, &junk))
-				return (FALSE);
-		}
-	}
 
-	if (xdrs->x_op == XDR_DECODE) {
-		if (supp_ngroups > ngroups_max)
-			cred->cr_ngroups = ngroups_max + 1;
-		else
-			cred->cr_ngroups = supp_ngroups + 1;
-	}
+	junk = 0;
+	for (i = 0; i < supp_ngroups; ++i)
+		if (!xdr_uint32_t(xdrs, i < XU_NGROUPS - 1 ?
+		    &cred->cr_sgroups[i] : &junk))
+			return (FALSE);
+
+	if (xdrs->x_op != XDR_ENCODE)
+		cred->cr_ngroups = MIN(supp_ngroups + 1, XU_NGROUPS);
 
 	return (TRUE);
 }