From 0b1ccb0dbc3b7673558eceff3d672971f5bb0197 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Mon, 4 Nov 2024 20:33:16 -0800 Subject: [PATCH] OPENAFS-SA-2024-002: verify FetchACL returned a valid string CVE-2024-10396 Analogously to how a call to RXAFS_StoreACL() with a malformed ACL string can cause a fileserver to perform invalid memory operations, a malformed ACL string returned in response to a call to RXAFS_FetchACL() can cause a client to perform invalid memory operations. Modify all the in-tree callers of the RPC to verify that the ACL data, which is conveyed as an XDR 'opaque' but whose contents are actually expected to be a string, is a valid C string. If a zero-length opaque or one without a trailing NUL is received, treat that as an error response from the fileserver rather than returning success. The Unix cache manager's pioctl handler already has logic to cope with a zero-length reply by emitting a single NUL byte to userspace. This special-casing seems to have been in place from the original IBM import, though it does so by confusingly "skipping over" a NUL byte already put in place. For historical compatibility, preserve that behavior rather than treating the zero-length reply as an error as we do for the other callers. It seems likely that this location should treat a zero-length reply as an error just as the other call sites do, but that can be done as a later change. Change-Id: Ibf685e54e7e3fca6a4caac63c961cfcfb2f4732a Reviewed-on: https://gerrit.openafs.org/15914 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/cm_ioctl.c | 4 ++++ src/afs/afs_pioctl.c | 10 ++++++++-- src/libafscp/afscp_acl.c | 6 ++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index 8b82c55eb6..b8047b3dda 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -447,6 +447,10 @@ cm_IoctlGetACL(cm_ioctl_t *ioctlp, cm_user_t *userp, cm_scache_t *scp, cm_req_t if (code) return code; + /* Ensure the ACL is a string before trying to treat it like one. */ + if (acl.AFSOpaque_len == 0 || memchr(acl.AFSOpaque_val, '\0', + acl.AFSOpaque_len) == NULL) + return CM_ERROR_INVAL; } /* skip over return data */ tlen = (int)strlen(ioctlp->outDatap) + 1; diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index 37b1ae1152..17715f4b49 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1610,10 +1610,16 @@ DECL_PIOCTL(PGetAcl) SHARED_LOCK, NULL)); if (code == 0) { - if (acl.AFSOpaque_len == 0) + if (acl.AFSOpaque_len == 0) { afs_pd_skip(aout, 1); /* leave the NULL */ - else + + } else if (memchr(acl.AFSOpaque_val, '\0', acl.AFSOpaque_len) == NULL) { + /* Do not return an unterminated ACL string. */ + code = EINVAL; + + } else { afs_pd_skip(aout, acl.AFSOpaque_len); /* Length of the ACL */ + } } return code; } diff --git a/src/libafscp/afscp_acl.c b/src/libafscp/afscp_acl.c index cd76125bab..f29be29813 100644 --- a/src/libafscp/afscp_acl.c +++ b/src/libafscp/afscp_acl.c @@ -58,6 +58,12 @@ afscp_FetchACL(const struct afscp_venusfid *dir, struct AFSOpaque *acl) code = RXAFS_FetchACL(server->conns[j], &df, acl, &dfst, &vs); if (code >= 0) break; + /* Zero-length or non-string ACLs are malformed. */ + if (acl->AFSOpaque_len == 0 || memchr(acl->AFSOpaque_val, '\0', + acl->AFSOpaque_len) == NULL) { + code = EIO; + break; + } } } if (code >= 0)