sys/rpc: UNIX auth: Fix OOB reads on too short message

In the inline version (_svcauth_unix()), fix multiple possible OOB reads
when the credentials part of a request is too short to contain mandatory
fields or with respect to the hostname length or number of groups it
advertises.  The previously existing check was arriving too late and
relied on possibly wrong data coming from earlier OOB reads.

While here, use 'uint32_t' as the length/size type, as it is more than
enough and removes the need for conversions, explicit or implicit.
While here, factor out setting 'stat' to AUTH_BADCRED and then jumping
to 'done' on error, through the new 'badcred' label.  While here,
through comments, refer to what the non-inline version is doing
(xdr_authunix_parms() in 'authunix_prot.c') and the reasons.

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

Author: OlCe2

sys/rpc/svc_auth_unix.c

@@ -59,70 +59,91 @@ _svcauth_unix(struct svc_req *rqst, struct rpc_msg *msg)
 	enum auth_stat stat;
 	XDR xdrs;
 	int32_t *buf;
-	uint32_t time;
 	struct xucred *xcr;
-	u_int auth_len;
-	size_t str_len, supp_ngroups;
-	u_int i;
+	uint32_t auth_len, time;
 
 	xcr = rqst->rq_clntcred;
 	auth_len = (u_int)msg->rm_call.cb_cred.oa_length;
 	xdrmem_create(&xdrs, msg->rm_call.cb_cred.oa_base, auth_len,
 	    XDR_DECODE);
 	buf = XDR_INLINE(&xdrs, auth_len);
 	if (buf != NULL) {
-		time = IXDR_GET_UINT32(buf);
-		str_len = (size_t)IXDR_GET_UINT32(buf);
-		if (str_len > AUTH_SYS_MAX_HOSTNAME) {
-			stat = AUTH_BADCRED;
-			goto done;
+		/* 'time', 'str_len', UID, GID and 'supp_ngroups'. */
+		const uint32_t min_len = 5 * BYTES_PER_XDR_UNIT;
+		uint32_t str_len, supp_ngroups;
+
+		if (auth_len < min_len) {
+			(void)printf("AUTH_SYS: Too short credentials (%u)\n",
+			    auth_len);
+			goto badcred;
 		}
+		time = IXDR_GET_UINT32(buf);
+		str_len = IXDR_GET_UINT32(buf);
+		if (str_len > AUTH_SYS_MAX_HOSTNAME)
+			goto badcred;
 		str_len = RNDUP(str_len);
+		/*
+		 * Recheck message length now that we know the value of
+		 * 'str_len' (and that it won't cause an overflow in additions
+		 * below) to protect access to the credentials part.
+		 */
+		if (auth_len < min_len + str_len) {
+			(void)printf("AUTH_SYS: Inconsistent credentials and "
+			    "host name lengths (%u, %u)\n",
+			    auth_len, str_len);
+			goto badcred;
+		}
 		buf += str_len / sizeof (int32_t);
 		xcr->cr_uid = IXDR_GET_UINT32(buf);
 		xcr->cr_gid = IXDR_GET_UINT32(buf);
-		supp_ngroups = (size_t)IXDR_GET_UINT32(buf);
-		if (supp_ngroups > AUTH_SYS_MAX_GROUPS) {
-			stat = AUTH_BADCRED;
-			goto done;
-		}
-		for (i = 0; i < supp_ngroups; i++) {
-			/*
-			 * Note that this is a `struct xucred`, which maintains
-			 * its historical layout of preserving the egid in
-			 * cr_ngroups and cr_groups[0] == egid.
-			 */
-			if (i + 1 < XU_NGROUPS)
-				xcr->cr_groups[i + 1] = IXDR_GET_INT32(buf);
-			else
-				buf++;
+		supp_ngroups = IXDR_GET_UINT32(buf);
+		/*
+		 * See the herald comment before a similar test at the end of
+		 * xdr_authunix_parms() for why we strictly respect RFC 5531 and
+		 * why we may have to drop the last supplementary group when
+		 * there are AUTH_SYS_MAX_GROUPS of them.
+		 */
+		if (supp_ngroups > AUTH_SYS_MAX_GROUPS)
+			goto badcred;
+		/*
+		 * Final message length check, as we now know how much we will
+		 * read in total.
+		 */
+		if (auth_len < min_len + str_len +
+		    supp_ngroups * BYTES_PER_XDR_UNIT) {
+			(void)printf("AUTH_SYS: Inconsistent lengths "
+			    "(credentials %u, machine name %u, "
+			    "supplementary groups %u)\n",
+			    auth_len, str_len,
+			    supp_ngroups * BYTES_PER_XDR_UNIT);
+			goto badcred;
 		}
-		if (supp_ngroups + 1 > XU_NGROUPS)
-			xcr->cr_ngroups = XU_NGROUPS;
-		else
-			xcr->cr_ngroups = supp_ngroups + 1;
 
 		/*
-		 * five is the smallest unix credentials structure -
-		 * timestamp, hostname len (0), uid, gid, and gids len (0).
+		 * Note that 'xcr' 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'.
 		 */
-		if ((5 + supp_ngroups) * BYTES_PER_XDR_UNIT + str_len > auth_len) {
-			(void) printf("bad auth_len gid %ld str %ld auth %u\n",
-			    (long)supp_ngroups, (long)str_len, auth_len);
-			stat = AUTH_BADCRED;
-			goto done;
+		for (uint32_t i = 0; i < supp_ngroups; ++i) {
+			if (i < XU_NGROUPS - 1)
+				xcr->cr_sgroups[i] = IXDR_GET_INT32(buf);
+			else
+				buf++;
 		}
-	} else if (! xdr_authunix_parms(&xdrs, &time, xcr)) {
-		stat = AUTH_BADCRED;
-		goto done;
-	}
+		xcr->cr_ngroups = MIN(supp_ngroups + 1, XU_NGROUPS);
+	} else if (! xdr_authunix_parms(&xdrs, &time, xcr))
+		goto badcred;
 
 	rqst->rq_verf = _null_auth;
 	stat = AUTH_OK;
 done:
 	XDR_DESTROY(&xdrs);
 
 	return (stat);
+
+badcred:
+	stat = AUTH_BADCRED;
+	goto done;
 }