From 25ad3931d5c03ead625a96e6b626febeb3e20453 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 16 Oct 2020 10:52:03 -0500 Subject: [PATCH] OPENAFS-SA-2024-003: Run xdr_free for retried RPCs CVE-2024-10397 A few areas of code retry the same RPC, like so: do { code = VL_SomeRPC(rxconn, &array_out); } while (some_condition); xdr_free((xdrproc_t) xdr_foo, &array_out); Or try a different version/variant of an RPC (e.g. VLDB_ListAttributesN2 -> VLDB_ListAttributes). If the first RPC call causes the output array to be allocated with length N, then the subsequent RPC calls may fail if the server responds with an array larger than N. Furthermore, if the subsequent call responds with an array smaller than N, then when we xdr_free the array, our length will be smaller than the actual number of allocated elements. That results in two potential issues: - We'll fail to free the elements at the end of the array. This is only a problem if each element in the array also uses dynamically-allocated memory (e.g. each element contains a string or another array). Fortunately, there are only a few such structures in any of our RPC-L definitions: SysNameList and CredInfos. And neither of those are used in such a retry loop, so this isn't a problem. - We'll give the wrong length to osi_free when freeing the array itself. This only matters for KERNEL, and only on some platforms (such as Solaris), since the length given to osi_free is ignored everywhere else. To avoid these possible issues, change the relevant retry loops to free our xdr-allocated arrays on every iteration of the loop, like this: do { xdr_free((xdrproc_t) xdr_foo, &array_out); code = VL_SomeRPC(rxconn, &array_out); } while (some_condition); xdr_free((xdrproc_t) xdr_foo, &array_out); Or like this: do { code = VL_SomeRPC(rxconn, &array_out); xdr_free((xdrproc_t) xdr_foo, &array_out); } while (some_condition); FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15923 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 1f5e1ef9e35f6b5e8693c91199c976d5e030c0d0) Change-Id: I77ce3a904d502784cbf356e113972dfab838256e Reviewed-on: https://gerrit.openafs.org/15945 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/cm_getaddrs.c | 1 + src/afs/afs_pioctl.c | 1 + src/afs/afs_volume.c | 1 + src/ptserver/pts.c | 1 + src/ptserver/testpt.c | 1 + src/volser/vos.c | 1 + 6 files changed, 6 insertions(+) diff --git a/src/WINNT/afsd/cm_getaddrs.c b/src/WINNT/afsd/cm_getaddrs.c index 48d42489be..a165271e2a 100644 --- a/src/WINNT/afsd/cm_getaddrs.c +++ b/src/WINNT/afsd/cm_getaddrs.c @@ -234,6 +234,7 @@ cm_GetAddrsU(cm_cell_t *cellp, cm_user_t *userp, cm_req_t *reqp, code = cm_ConnByMServers(cellp->vlServersp, 0, userp, reqp, &connp); if (code) continue; + xdr_free((xdrproc_t) xdr_bulkaddrs, &addrs); rxconnp = cm_GetRxConn(connp); code = VL_GetAddrsU(rxconnp, &attrs, &uuid, &unique, &nentries, &addrs); diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index 5337986c4e..faafdd3d2d 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1596,6 +1596,7 @@ DECL_PIOCTL(PGetAcl) if (tconn) { XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_FETCHACL); RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_AFSOpaque, &acl); code = RXAFS_FetchACL(rxconn, &Fid, &acl, &OutStatus, &tsync); RX_AFS_GLOCK(); XSTATS_END_TIME; diff --git a/src/afs/afs_volume.c b/src/afs/afs_volume.c index 0600ccbfaf..42f1ba9d91 100644 --- a/src/afs/afs_volume.c +++ b/src/afs/afs_volume.c @@ -1205,6 +1205,7 @@ LockAndInstallUVolumeEntry(struct volume *av, struct uvldbentry *ve, int acell, 0, &rxconn); if (tconn) { RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_bulkaddrs, &addrs); code = VL_GetAddrsU(rxconn, &attrs, &uuid, &unique, &nentries, &addrs); diff --git a/src/ptserver/pts.c b/src/ptserver/pts.c index c97307bdf1..05508a70df 100644 --- a/src/ptserver/pts.c +++ b/src/ptserver/pts.c @@ -681,6 +681,7 @@ CheckEntry(struct cmd_syndesc *as, void *arock) lids.idlist_val[0] = aentry.owner; lids.idlist_val[1] = aentry.creator; + xdr_free((xdrproc_t) xdr_namelist, &lnames); code = pr_IdToName(&lids, &lnames); if (code) { rcode = code; diff --git a/src/ptserver/testpt.c b/src/ptserver/testpt.c index c359f8f465..50f0029f6f 100644 --- a/src/ptserver/testpt.c +++ b/src/ptserver/testpt.c @@ -112,6 +112,7 @@ ListUsedIds(struct cmd_syndesc *as, void *arock) startId++; } lids.idlist_len = i; + xdr_free((xdrproc_t) xdr_namelist, &lnames); code = pr_IdToName(&lids, &lnames); if (code) { afs_com_err(whoami, code, "converting id to name"); diff --git a/src/volser/vos.c b/src/volser/vos.c index 007bfe5064..6efc1c73fd 100644 --- a/src/volser/vos.c +++ b/src/volser/vos.c @@ -4520,6 +4520,7 @@ ListVLDB(struct cmd_syndesc *as, void *arock) &arrayEntries, &nextindex); if (vcode == RXGEN_OPCODE) { /* Vlserver not running with ListAttributesN2. Fall back */ + xdr_free((xdrproc_t) xdr_nbulkentries, &arrayEntries); vcode = VLDB_ListAttributes(&attributes, ¢ries, &arrayEntries); nextindex = -1;