From b2b1110ddd9e19670dbc6a3217dc2a74af432f82 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 13 Jun 2024 15:30:50 -0500 Subject: [PATCH] OPENAFS-SA-2024-003: xdr: Set _len for prealloc'd opaque/array OUT args CVE-2024-10397 Currently, a few RPCs with arrays or opaque OUT arguments are called with preallocated memory for the arg, but also provide a _len of 0 (or an uninitialized _len). This makes it impossible for the xdr routine to tell whether we have allocated enough space to actually hold the response from the server. To help this situation, either specify an appropriate _len for the preallocated value (cm_IoctlGetACL, fsprobe_LWP), or don't provide a preallocated buffer at all and let xdr allocate a buffer for us (PGetAcl). Note that this commit doesn't change xdr to actually check the value of the given _len; but now a future commit can do so without breaking callers. FIXES 135043 Change-Id: Ieb50aaa5ae9a1bde027999ce1c668e0c99b4d82b Reviewed-on: https://gerrit.openafs.org/15919 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/cm_ioctl.c | 2 +- src/afs/afs_pioctl.c | 8 ++++---- src/fsprobe/fsprobe.c | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index 015ed14a28..ffe5f70ae8 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -433,7 +433,7 @@ cm_IoctlGetACL(cm_ioctl_t *ioctlp, cm_user_t *userp, cm_scache_t *scp, cm_req_t afid.Unique = scp->fid.unique; do { acl.AFSOpaque_val = ioctlp->outDatap; - acl.AFSOpaque_len = 0; + acl.AFSOpaque_len = SMB_IOCTL_MAXDATA - (ioctlp->outDatap - ioctlp->outAllocp); code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp); if (code) continue; diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index 2302a6351d..0157bdc269 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1593,11 +1593,10 @@ DECL_PIOCTL(PGetAcl) return ERANGE; Fid.Vnode |= (ain->remaining << 30); } - acl.AFSOpaque_val = aout->ptr; + memset(&acl, 0, sizeof(acl)); do { tconn = afs_Conn(&avc->f.fid, areq, SHARED_LOCK, &rxconn); if (tconn) { - acl.AFSOpaque_val[0] = '\0'; XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_FETCHACL); RX_AFS_GUNLOCK(); code = RXAFS_FetchACL(rxconn, &Fid, &acl, &OutStatus, &tsync); @@ -1611,7 +1610,7 @@ DECL_PIOCTL(PGetAcl) if (code == 0) { if (acl.AFSOpaque_len == 0) { - afs_pd_skip(aout, 1); /* leave the NULL */ + code = afs_pd_putBytes(aout, "\0", 1); /* leave a single NUL byte */ } else if (memchr(acl.AFSOpaque_val, '\0', acl.AFSOpaque_len) == NULL) { /* Do not return an unterminated ACL string. */ @@ -1622,9 +1621,10 @@ DECL_PIOCTL(PGetAcl) code = EINVAL; } else { - afs_pd_skip(aout, acl.AFSOpaque_len); /* Length of the ACL */ + code = afs_pd_putBytes(aout, acl.AFSOpaque_val, acl.AFSOpaque_len); } } + xdr_free((xdrproc_t) xdr_AFSOpaque, &acl); return code; } diff --git a/src/fsprobe/fsprobe.c b/src/fsprobe/fsprobe.c index 7690992904..27a14d21b6 100644 --- a/src/fsprobe/fsprobe.c +++ b/src/fsprobe/fsprobe.c @@ -224,6 +224,7 @@ fsprobe_LWP(void *unused) struct ProbeViceStatistics *curr_stats; /*Current stats region */ int *curr_probeOK; /*Current probeOK field */ ViceStatistics64 stats64; /*Current stats region */ + stats64.ViceStatistics64_len = STATS64_VERSION; stats64.ViceStatistics64_val = malloc(STATS64_VERSION * sizeof(afs_uint64)); while (1) { /*Service loop */