From 00a1b266af51a828a022c23e7bb006a39740eaad Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 13 Jun 2024 15:28:38 -0500 Subject: [PATCH] OPENAFS-SA-2024-003: xdr: Avoid prealloc'd string OUT args CVE-2024-10397 Currently, several callers call RPCs with string OUT arguments, and provide preallocated memory for those arguments. This can easily allow a response from the server to overrun the allocated buffer, stomping over stack or heap memory. We could simply make our preallocated buffers larger than the maximum size that the RPC allows, but relying on that is error prone, and there's no way for XDR to check if a string buffer is large enough. Instead, to make sure we don't overrun a given preallocated buffer, avoid giving a preallocated buffer to such RPCs, and let XDR allocate the memory for us. Specifically, this commit changes several callers to RXAFS_GetVolumeStatus(), and one caller of BOZO_GetInstanceParm(), to avoid passing in a preallocated string buffer. All other callers of RPCs with string OUT args already let XDR allocate the buffers for them. FIXES 135043 Change-Id: If42e2cc983903cff9766e1bab487142d4d493a17 Reviewed-on: https://gerrit.openafs.org/15918 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/cm_ioctl.c | 26 +++++++++++++++++----- src/WINNT/afsd/cm_vnodeops.c | 18 +++++++-------- src/WINNT/afsd/cm_volume.c | 19 ++++++++-------- src/WINNT/afsrdr/user/RDRFunction.c | 34 ++++++++++++++--------------- src/afs/afs_pioctl.c | 20 ++++++++--------- src/libadmin/bos/afs_bosAdmin.c | 18 +++++++++++---- 6 files changed, 77 insertions(+), 58 deletions(-) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index e8c31b5ac6..015ed14a28 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -764,9 +764,6 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach cm_conn_t *connp; AFSFetchVolumeStatus volStat; char *cp; - char *Name; - char *OfflineMsg; - char *MOTD; struct rx_connection * rxconnp; #ifdef AFS_FREELANCE_CLIENT @@ -785,19 +782,23 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach { cm_fid_t vfid; cm_scache_t *vscp; + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; cm_SetFid(&vfid, scp->fid.cell, scp->fid.volume, 1, 1); code = cm_GetSCache(&vfid, NULL, &vscp, userp, reqp); if (code) return code; - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; do { code = cm_ConnFromFID(&vfid, userp, reqp, &connp); if (code) continue; + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + rxconnp = cm_GetRxConn(connp); code = RXAFS_GetVolumeStatus(rxconnp, vfid.volume, &volStat, &Name, &OfflineMsg, &MOTD); @@ -807,6 +808,19 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach code = cm_MapRPCError(code, reqp); cm_ReleaseSCache(vscp); + + strncpy(volName, Name, sizeof(volName)); + volName[sizeof(volName) - 1] = '\0'; + + strncpy(offLineMsg, OfflineMsg, sizeof(offLineMsg)); + offLineMsg[sizeof(offLineMsg) - 1] = '\0'; + + strncpy(motd, MOTD, sizeof(motd)); + motd[sizeof(motd) - 1] = '\0'; + + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); } if (code) diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c index 074a970450..226bf7af00 100644 --- a/src/WINNT/afsd/cm_vnodeops.c +++ b/src/WINNT/afsd/cm_vnodeops.c @@ -2743,12 +2743,6 @@ cm_IsSpaceAvailable(cm_fid_t * fidp, osi_hyper_t *sizep, cm_user_t *userp, cm_re AFSFetchVolumeStatus volStat; cm_volume_t *volp = NULL; afs_uint32 volType; - char *Name; - char *OfflineMsg; - char *MOTD; - char volName[32]="(unknown)"; - char offLineMsg[256]="server temporarily inaccessible"; - char motd[256]="server temporarily inaccessible"; osi_hyper_t freespace; cm_fid_t vfid; cm_scache_t *vscp; @@ -2778,11 +2772,11 @@ cm_IsSpaceAvailable(cm_fid_t * fidp, osi_hyper_t *sizep, cm_user_t *userp, cm_re CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_GETSTATUS); lock_ReleaseWrite(&vscp->rw); if (code == 0) { - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; - do { + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + code = cm_ConnFromFID(&vfid, userp, reqp, &connp); if (code) continue; @@ -2791,6 +2785,10 @@ cm_IsSpaceAvailable(cm_fid_t * fidp, osi_hyper_t *sizep, cm_user_t *userp, cm_re &volStat, &Name, &OfflineMsg, &MOTD); rx_PutConnection(rxconnp); + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + } while (cm_Analyze(connp, userp, reqp, &vfid, NULL, 0, NULL, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, reqp); } diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c index 017c50f597..5cd262857c 100644 --- a/src/WINNT/afsd/cm_volume.c +++ b/src/WINNT/afsd/cm_volume.c @@ -1310,24 +1310,14 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 cm_conn_t *connp; long code; AFSFetchVolumeStatus volStat; - char *Name; - char *OfflineMsg; - char *MOTD; cm_req_t req; struct rx_connection * rxconnp; - char volName[32]; afs_uint32 volType; - char offLineMsg[256]; - char motd[256]; long alldown, alldeleted; cm_serverRef_t *serversp; cm_fid_t vfid; cm_scache_t *vscp = NULL; - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; - volType = cm_VolumeType(volp, volID); if (statep->ID != 0 && (!volID || volID == statep->ID)) { @@ -1373,6 +1363,10 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 code = cm_GetSCache(&vfid, NULL, &vscp, cm_rootUserp, &req); if (code = 0) { do { + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + code = cm_ConnFromVolume(volp, statep->ID, cm_rootUserp, &req, &connp); if (code) continue; @@ -1381,6 +1375,11 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 code = RXAFS_GetVolumeStatus(rxconnp, statep->ID, &volStat, &Name, &OfflineMsg, &MOTD); rx_PutConnection(rxconnp); + + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + } while (cm_Analyze(connp, cm_rootUserp, &req, &vfid, NULL, 0, NULL, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, &req); diff --git a/src/WINNT/afsrdr/user/RDRFunction.c b/src/WINNT/afsrdr/user/RDRFunction.c index cffb9b58c6..db767cbb8a 100644 --- a/src/WINNT/afsrdr/user/RDRFunction.c +++ b/src/WINNT/afsrdr/user/RDRFunction.c @@ -5820,14 +5820,8 @@ RDR_GetVolumeInfo( IN cm_user_t *userp, DWORD status; FILETIME ft = {0x832cf000, 0x01abfcc4}; /* October 1, 1982 00:00:00 +0600 */ - char volName[32]="(unknown)"; - char offLineMsg[256]="server temporarily inaccessible"; - char motd[256]="server temporarily inaccessible"; cm_conn_t *connp; AFSFetchVolumeStatus volStat; - char *Name; - char *OfflineMsg; - char *MOTD; struct rx_connection * rxconnp; int scp_locked = 0; @@ -5928,13 +5922,14 @@ RDR_GetVolumeInfo( IN cm_user_t *userp, if (code == -1) { - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; lock_ReleaseWrite(&scp->rw); scp_locked = 0; do { + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + code = cm_ConnFromFID(&scp->fid, userp, &req, &connp); if (code) continue; @@ -5943,6 +5938,10 @@ RDR_GetVolumeInfo( IN cm_user_t *userp, &volStat, &Name, &OfflineMsg, &MOTD); rx_PutConnection(rxconnp); + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + } while (cm_Analyze(connp, userp, &req, &scp->fid, NULL, 0, NULL, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, &req); @@ -6056,14 +6055,8 @@ RDR_GetVolumeSizeInfo( IN cm_user_t *userp, cm_req_t req; DWORD status; - char volName[32]="(unknown)"; - char offLineMsg[256]="server temporarily inaccessible"; - char motd[256]="server temporarily inaccessible"; cm_conn_t *connp; AFSFetchVolumeStatus volStat; - char *Name; - char *OfflineMsg; - char *MOTD; struct rx_connection * rxconnp; int scp_locked = 0; @@ -6141,13 +6134,14 @@ RDR_GetVolumeSizeInfo( IN cm_user_t *userp, if (code == -1) { - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; lock_ReleaseWrite(&scp->rw); scp_locked = 0; do { + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + code = cm_ConnFromFID(&scp->fid, userp, &req, &connp); if (code) continue; @@ -6156,6 +6150,10 @@ RDR_GetVolumeSizeInfo( IN cm_user_t *userp, &volStat, &Name, &OfflineMsg, &MOTD); rx_PutConnection(rxconnp); + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + } while (cm_Analyze(connp, userp, &req, &scp->fid, NULL, 0, NULL, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, &req); diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index f8a4715555..2302a6351d 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1997,32 +1997,31 @@ DECL_PIOCTL(PSetTokens) */ DECL_PIOCTL(PGetVolumeStatus) { - char volName[32]; - char *offLineMsg = afs_osi_Alloc(256); - char *motd = afs_osi_Alloc(256); + char *volName = NULL; + char *offLineMsg = NULL; + char *motd = NULL; struct afs_conn *tc; afs_int32 code = 0; struct AFSFetchVolumeStatus volstat; - char *Name; struct rx_connection *rxconn; XSTATS_DECLS; - osi_Assert(offLineMsg != NULL); - osi_Assert(motd != NULL); AFS_STATCNT(PGetVolumeStatus); if (!avc) { code = EINVAL; goto out; } - Name = volName; do { tc = afs_Conn(&avc->f.fid, areq, SHARED_LOCK, &rxconn); if (tc) { XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_GETVOLUMESTATUS); RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_string, &volName); + xdr_free((xdrproc_t) xdr_string, &offLineMsg); + xdr_free((xdrproc_t) xdr_string, &motd); code = RXAFS_GetVolumeStatus(rxconn, avc->f.fid.Fid.Volume, &volstat, - &Name, &offLineMsg, &motd); + &volName, &offLineMsg, &motd); RX_AFS_GLOCK(); XSTATS_END_TIME; } else @@ -2043,8 +2042,9 @@ DECL_PIOCTL(PGetVolumeStatus) if (afs_pd_putString(aout, motd) != 0) return E2BIG; out: - afs_osi_Free(offLineMsg, 256); - afs_osi_Free(motd, 256); + xdr_free((xdrproc_t) xdr_string, &volName); + xdr_free((xdrproc_t) xdr_string, &offLineMsg); + xdr_free((xdrproc_t) xdr_string, &motd); return code; } diff --git a/src/libadmin/bos/afs_bosAdmin.c b/src/libadmin/bos/afs_bosAdmin.c index 76436c0496..8ec0129770 100644 --- a/src/libadmin/bos/afs_bosAdmin.c +++ b/src/libadmin/bos/afs_bosAdmin.c @@ -1289,6 +1289,8 @@ bos_ProcessNotifierGet(const void *serverHandle, const char *processName, int rc = 0; afs_status_t tst = 0; bos_server_p b_handle = (bos_server_p) serverHandle; + char *tnotif = NULL; + size_t len; if (!isValidServerHandle(b_handle, &tst)) { goto fail_bos_ProcessNotifierGet; @@ -1305,14 +1307,22 @@ bos_ProcessNotifierGet(const void *serverHandle, const char *processName, } tst = BOZO_GetInstanceParm(b_handle->server, (char *)processName, - 999, ¬ifier); - - if (tst == 0) { - rc = 1; + 999, &tnotif); + if (tst != 0) { + goto fail_bos_ProcessNotifierGet; } + len = strlcpy(notifier, tnotif, BOS_MAX_NAME_LEN); + if (len >= BOS_MAX_NAME_LEN) { + tst = ADMRPCTOOBIG; + goto fail_bos_ProcessNotifierGet; + } + + rc = 1; + fail_bos_ProcessNotifierGet: + xdr_free((xdrproc_t)xdr_string, &tnotif); if (st != NULL) { *st = tst; }