OPENAFS-SA-2024-003: xdr: Prevent XDR_DECODE buffer overruns

CVE-2024-10397

When making an RPC call from a client, output arguments that use
arrays (or array-like objects like strings and opaques) can be
allocated by XDR, like so:

{
    struct idlist ids;

    ids.idlist_val = NULL;
    ids.idlist_len = 0;
    code = PR_NameToID(rxconn, names, &ids);
    /* data inside ids.idlist_val[...] */
    xdr_free((xdrproc_t) xdr_idlist, &ids);
}

With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, then allocates enough memory to hold
that many elements, and then reads in the array elements.

Alternatively, the caller can provide preallocated memory, like so:

{
    struct idlist ids;
    afs_int32 ids_buf[30];

    ids.idlist_val = ids_buf;
    ids.idlist_len = 30;
    code = PR_NameToID(rxconn, names, &ids);
    /* data inside ids.idlist_val[...] */
}

With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, and then reads in the array elements
into the supplied buffer. However, in this case, xdr_array() never
checks that the number of array elements will actually fit into the
supplied buffer; the _len field provided by the caller is just ignored.
In this example, if the ptserver responds with 50 elements for the 'ids'
output argument, xdr_array() will write 50 afs_int32's into
'ids.idlist_val', going beyond the end of the 30 elements that are
actually allocated.

It's also possible, and in fact very easy, to use xdr-allocated
buffers and then reuse them as a preallocated buffer, possibly
accidentally. For example:

{
    struct idlist ids;

    ids.idlist_val = NULL;
    ids.idlist_len = 0;
    while (some_condition) {
        code = PR_NameToID(rxconn, names, &ids);
    }
}

In this case, the first call to PR_NameToID can cause the buffer for
'ids' to be allocated by XDR, which will then be reused by the
subsequent calls to PR_NameToId. Note that this can happen even if the
first PR_NameToID call fails; the call can be aborted after the output
array is allocated.

Retrying an RPC in this way is effectively what all ubik_Call*
codepaths do (including all ubik_* wrappers, e.g. ubik_PR_NameToID).
Or some callers retry effectively the same RPC when falling back to
earlier versions (e.g. VL_ListAttributesN2 -> VL_ListAttributesN).

To prevent this for arrays and opaques, change xdr_array (and
xdr_bytes) to check if the _len field for preallocated buffers is
large enough, and return failure if it's not.

Also perform the same check for the ka_CBS and ka_BBS structures. These
are mostly the same as opaques, but they have custom serialization
functions in src/kauth/kaaux.c. ka_BBS also has two lengths: the actual
length of bytes, and a 'max' length. ka_CBS isn't used for any RPC
output arguments, but fix it for consistency.

For strings, the situation is complicated by the fact that callers
cannot pass in how much space was allocated for the string, since
callers only provide a char**. So for strings, just refuse to use a
preallocated buffer at all, and return failure if one is provided.

Note that for some callers using preallocated arrays or strings, the
described buffer overruns are not possible, since the preallocated
buffers are larger than the max length specified in the relevant
RPC-L. For example, afs_DoBulkStat() allocates AFSCBMAX entries for
the output args for RXAFS_InlineBulkStatus, which is the max length
specified in the RPC-L, so a buffer overrun is impossible. But since
it is so easy to allow a buffer overrun, enforce the length checks for
everyone.

FIXES 135043

Change-Id: Iaf913e2156af2081c72125ec066d9636086c7d14
Reviewed-on: https://gerrit.openafs.org/15920
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>

src/kauth/kaaux.c

@@ -34,7 +34,17 @@ xdr_ka_CBS(XDR * x, struct ka_CBS *abbs)
 	xdr_afs_int32(x, &len);
 	if (len < 0 || len > MAXBS)
 	    return FALSE;
-	if (!abbs->SeqBody)
+	if (abbs->SeqBody != NULL) {
+	    if (len > abbs->SeqLen) {
+		/*
+		 * We've been given a preallocated buffer to decode into, but
+		 * we're decoding 'len' bytes, which is larger than the
+		 * provided buffer (only abbs->SeqLen bytes large). This won't
+		 * work.
+		 */
+		return FALSE;
+	    }
+	} else
 	    abbs->SeqBody = malloc(len);
 	abbs->SeqLen = len;
 	xdr_opaque(x, abbs->SeqBody, len);
@@ -61,7 +71,26 @@ xdr_ka_BBS(XDR * x, struct ka_BBS *abbs)
 	if (!xdr_afs_int32(x, &maxLen) || !xdr_afs_int32(x, &len) || (len < 0)
 	    || (len > MAXBS) || (len > maxLen))
 	    return FALSE;
-	if (!abbs->SeqBody)
+	if (abbs->SeqBody != NULL) {
+	    if (len > abbs->MaxSeqLen) {
+		/*
+		 * We've been given a preallocated buffer to decode into, but
+		 * we're decoding 'len' bytes, which is larger than the
+		 * provided buffer (only abbs->MaxSeqLen bytes large). This
+		 * won't work.
+		 */
+		return FALSE;
+	    }
+	    if (maxLen > abbs->MaxSeqLen) {
+		/*
+		 * Our preallocated buffer only has space for MaxSeqLen bytes.
+		 * Don't let the peer change 'abbs' so that it looks like we
+		 * have space for more bytes than that; that could cause us to
+		 * access memory beyond what we've actually allocated.
+		 */
+		return FALSE;
+	    }
+	} else
 	    abbs->SeqBody = malloc(maxLen);
 	abbs->MaxSeqLen = maxLen;
 	abbs->SeqLen = len;

src/rx/xdr.c

@@ -404,10 +404,28 @@ xdr_bytes(XDR * xdrs, char **cpp, u_int * sizep,
     /*
      * first deal with the length since xdr bytes are counted
      */
-    if (!xdr_u_int(xdrs, sizep)) {
-	return (FALSE);
+
+    if (xdrs->x_op == XDR_DECODE && sp != NULL) {
+	/*
+	 * We've been given a preallocated array to decode into. Before we
+	 * modify *sizep, check that we have enough space to fit the elements
+	 * that follow.
+	 */
+	if (!xdr_u_int(xdrs, &nodesize)) {
+	   return FALSE;
+	}
+	if (nodesize > *sizep) {
+	   return FALSE;
+	}
+	*sizep = nodesize;
+
+    } else {
+	if (!xdr_u_int(xdrs, sizep)) {
+	    return (FALSE);
+	}
+	nodesize = *sizep;
     }
-    nodesize = *sizep;
+
     if ((nodesize > maxsize) && (xdrs->x_op != XDR_FREE)) {
 	return (FALSE);
     }
@@ -541,8 +559,16 @@ xdr_string(XDR * xdrs, char **cpp, u_int maxsize)
     switch (xdrs->x_op) {
 
     case XDR_DECODE:
-	if (sp == NULL)
-	    *cpp = sp = (char *)osi_alloc(nodesize);
+	if (sp != NULL) {
+	    /*
+	     * Refuse to use preallocated strings, since we cannot verify
+	     * whether enough memory has been allocated to handle the decoded
+	     * string.
+	     */
+	    return FALSE;
+	}
+
+	*cpp = sp = (char *)osi_alloc(nodesize);
 	if (sp == NULL) {
 	    return (FALSE);
 	}

src/rx/xdr_array.c

@@ -93,10 +93,28 @@ xdr_array(XDR * xdrs, caddr_t * addrp, u_int * sizep, u_int maxsize,
 	maxsize = i;
 
     /* like strings, arrays are really counted arrays */
-    if (!xdr_u_int(xdrs, sizep)) {
-	return (FALSE);
+
+    if (xdrs->x_op == XDR_DECODE && target != NULL) {
+	/*
+	 * We've been given a preallocated array to decode into. Before we
+	 * modify *sizep, check that we have enough space to fit the elements
+	 * that follow.
+	 */
+	if (!xdr_u_int(xdrs, &c)) {
+	    return FALSE;
+	}
+	if (c > *sizep) {
+	    return FALSE;
+	}
+	*sizep = c;
+
+    } else {
+	if (!xdr_u_int(xdrs, sizep)) {
+	    return (FALSE);
+	}
+	c = *sizep;
     }
-    c = *sizep;
+
     if ((c > maxsize) && (xdrs->x_op != XDR_FREE)) {
 	return (FALSE);
     }

src/rx/xdr_arrayn.c

@@ -93,10 +93,28 @@ xdr_arrayN(XDR * xdrs, caddr_t * addrp, u_int * sizep, u_int maxsize,
 	maxsize = i;
 
     /* like strings, arrays are really counted arrays */
-    if (!xdr_u_int(xdrs, sizep)) {
-	return (FALSE);
+
+    if (xdrs->x_op == XDR_DECODE && target != NULL) {
+	/*
+	 * We've been given a preallocated array to decode into. Before we
+	 * modify *sizep, check that we have enough space to fit the elements
+	 * that follow.
+	 */
+	if (!xdr_u_int(xdrs, &c)) {
+	    return FALSE;
+	}
+	if (c > *sizep) {
+	    return FALSE;
+	}
+	*sizep = c;
+
+    } else {
+	if (!xdr_u_int(xdrs, sizep)) {
+	    return (FALSE);
+	}
+	c = *sizep;
     }
-    c = *sizep;
+
     if ((c > maxsize) && (xdrs->x_op != XDR_FREE)) {
 	return (FALSE);
     }