From eb8b93a971c6293cdfbf8cd3d9a6351a8cb76f81 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 21 Aug 2024 00:29:34 -0500 Subject: [PATCH] OPENAFS-SA-2024-002: viced: Introduce 'rawACL' in StoreACL CVE-2024-10396 Change our StoreACL implementation to refer to the 'AccessList' argument via a new local variable called 'rawACL'. This makes it clearer to users that the data is a string, and makes it easier for future commits to make sure we don't access the 'AccessList' argument in certain situations. Update almost all users in StoreACL to refer to 'rawACL' instead of 'AccessList'. Change the name of 'AccessList' to 'uncheckedACL' to make sure we don't miss any users. Update our check_acl() call to use 'uncheckedACL' (and not 'rawACL'), because it must use an AFSOpaque to check the ACL. Change RXStore_AccessList() and printableACL() to accept a plain char* instead of a struct AFSOpaque. This commit should not incur any noticeable behavior change. Technically printableACL() is changed to run strlen() on the given string, but this should not cause any noticeable change in behavior: This change could cause printableACL() to process less of the string than before, if the string contains a NUL byte before the end of the AFSOpaque buffer. But this doesn't matter, since the all of our code after this treats the ACL as a plain string, and so doesn't look at any data beyond the first NUL. It's not possible for printableACL() to process more data than before, because check_acl() has already checked that the ACL string contains a NUL byte, so we must process AFSOpaque_len bytes or fewer. FIXES 135445 Change-Id: I26229a39528fec788d96c77012c042c278a214c9 Reviewed-on: https://gerrit.openafs.org/15912 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/viced/afsfileprocs.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c index 3ac5042f09..3810655a35 100644 --- a/src/viced/afsfileprocs.c +++ b/src/viced/afsfileprocs.c @@ -1246,12 +1246,12 @@ RXFetch_AccessList(Vnode * targetptr, Vnode * parentwhentargetnotdir, * the target dir's vnode storage. */ static afs_int32 -RXStore_AccessList(Vnode * targetptr, struct AFSOpaque *AccessList) +RXStore_AccessList(Vnode * targetptr, char *AccessList) { int code; struct acl_accessList *newACL = NULL; - if (acl_Internalize_pr(hpr_NameToId, AccessList->AFSOpaque_val, &newACL) + if (acl_Internalize_pr(hpr_NameToId, AccessList, &newACL) != 0) { code = EINVAL; goto done; @@ -3068,20 +3068,21 @@ SRXAFS_StoreData64(struct rx_call * acall, struct AFSFid * Fid, } static char * -printableACL(struct AFSOpaque *AccessList) +printableACL(char *AccessList) { char *s; size_t i; + size_t len = strlen(AccessList); - s = calloc(1, AccessList->AFSOpaque_len + 1); + s = calloc(1, len + 1); if (s == NULL) return NULL; - for (i = 0; i < AccessList->AFSOpaque_len; i++) { - if (AccessList->AFSOpaque_val[i] == '\n') + for (i = 0; i < len; i++) { + if (AccessList[i] == '\n') s[i] = ' '; else - s[i] = AccessList->AFSOpaque_val[i]; + s[i] = AccessList[i]; } return s; } @@ -3119,7 +3120,7 @@ check_acl(struct AFSOpaque *AccessList) static afs_int32 common_StoreACL(afs_uint64 opcode, struct rx_call * acall, struct AFSFid * Fid, - struct AFSOpaque * AccessList, + struct AFSOpaque *uncheckedACL, struct AFSFetchStatus * OutStatus, struct AFSVolSync * Sync) { Vnode *targetptr = 0; /* pointer to input fid */ @@ -3135,6 +3136,7 @@ common_StoreACL(afs_uint64 opcode, struct in_addr logHostAddr; /* host ip holder for inet_ntoa */ struct fsstats fsstats; char *displayACL = NULL; + char *rawACL = uncheckedACL->AFSOpaque_val; int newOpcode = (opcode == opcode_RXAFS_StoreACL); fsstats_StartOp(&fsstats, FS_STATS_RPCIDX_STOREACL); @@ -3142,7 +3144,7 @@ common_StoreACL(afs_uint64 opcode, if ((errorCode = CallPreamble(acall, ACTIVECALL, Fid, &tcon, &thost))) goto Bad_StoreACL; - errorCode = check_acl(AccessList); + errorCode = check_acl(uncheckedACL); if (errorCode != 0) { goto Bad_StoreACL; } @@ -3150,12 +3152,12 @@ common_StoreACL(afs_uint64 opcode, /* Get ptr to client data for user Id for logging */ t_client = (struct client *)rx_GetSpecific(tcon, rxcon_client_key); logHostAddr.s_addr = rxr_HostOf(tcon); - displayACL = printableACL(AccessList); + displayACL = printableACL(rawACL); ViceLog(newOpcode ? 1 : 0, ("SAFS_StoreACL%s, Fid = %u.%u.%u, ACL=%s, Host %s:%d, Id %d\n", newOpcode ? "" : " CVE-2018-7168", Fid->Volume, Fid->Vnode, Fid->Unique, - displayACL == NULL ? AccessList->AFSOpaque_val : displayACL, + displayACL == NULL ? rawACL : displayACL, inet_ntoa(logHostAddr), ntohs(rxr_PortOf(tcon)), t_client->z.ViceId)); FS_LOCK; AFSCallStats.StoreACL++, AFSCallStats.TotalCalls++; @@ -3190,7 +3192,7 @@ common_StoreACL(afs_uint64 opcode, } /* Build and store the new Access List for the dir */ - if ((errorCode = RXStore_AccessList(targetptr, AccessList))) { + if ((errorCode = RXStore_AccessList(targetptr, rawACL))) { goto Bad_StoreACL; } @@ -3222,7 +3224,7 @@ common_StoreACL(afs_uint64 opcode, osi_auditU(acall, StoreACLEvent, errorCode, AUD_ID, t_client ? t_client->z.ViceId : 0, AUD_FID, Fid, AUD_ACL, - displayACL == NULL ? AccessList->AFSOpaque_val : displayACL, + displayACL == NULL ? rawACL : displayACL, AUD_END); free(displayACL); return errorCode;