From 35d218c1d17973c1412ea5dff1e23d9aae50c4c7 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Tue, 19 Sep 2023 15:44:08 -0500 Subject: [PATCH 1/8] OPENAFS-SA-2024-002: acl: Do not parse beyond end of ACL CVE-2024-10396 The early parsing code in acl_Internalize_pr() tries to advance 'nextc' to go beyond the first two newlines in the given ACL string. But if the given ACL string has no newlines, or only 1 newline, then 'nextc' will point beyond the end of the ACL string, potentially pointing to garbage. Intuitively, it may look like the ACL string must contain at least 2 newlines because we have sscanf()'d the string with "%d\n%\d". However, whitespace characters in sscanf() are not matched exactly like non-whitespace characters are; a sequence of whitespace characters matches any amount of whitespace (including none). So, a string like "1 2" will be parsed by "%d\n%d\n", but will not contain any newline characters. Usually this should result in a parse error from acl_Internalize_pr(), but if the garbage happens to parse successfully, this could result in unrelated memory getting stored to the ACL. To fix this, don't advance 'nextc' if we're already at the end of the ACL string. FIXES 135445 Change-Id: Ie009b59bec9a75afc81fee201c2fca6955f484e4 Reviewed-on: https://gerrit.openafs.org/15910 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/libacl/aclprocs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libacl/aclprocs.c b/src/libacl/aclprocs.c index cd07e813c9..ab6b21dac2 100644 --- a/src/libacl/aclprocs.c +++ b/src/libacl/aclprocs.c @@ -264,10 +264,12 @@ acl_Internalize_pr(int (*func)(namelist *names, idlist *ids), char *elist, struc nextc = elist; while (*nextc && *nextc != '\n') nextc++; - nextc++; + if (*nextc != '\0') + nextc++; while (*nextc && *nextc != '\n') nextc++; - nextc++; /* now at the beginning of the entry list */ + if (*nextc != '\0') + nextc++; /* now at the beginning of the entry list */ for (i = 0; i < (*acl)->positive; i++) { int k; if (sscanf(nextc, "%63s\t%d\n", lnames.namelist_val[i], &k) != 2) { -- 2.45.2 From 96ab2c6f8a614d597a523b45871c5f64a50a7040 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Tue, 19 Sep 2023 15:55:42 -0500 Subject: [PATCH 2/8] OPENAFS-SA-2024-002: acl: Error on missing newlines when parsing ACL CVE-2024-10396 In acl_Internalize_pr(), each line in an ACL granting rights (positive or negative) is sscanf()'d with "%63s\t%d\n", and then we try to advance 'nextc' beyond the next newline character. However, sscanf()'ing "%63s\t%d\n" does not guarantee that there is a newline in the given string. Whitespace characters in sscanf() are not matched exactly, and may match any amount of whitespace (including none at all). For example, a string like "foo 4" may be parsed by sscanf(), but does not contain any newlines. If this happens, strchr(nextc, '\n') will return NULL, and we'll advance 'nextc' to 0x1, causing a segfault when we next try to dereference 'nextc'. To avoid this, check if 'nextc' is NULL after the strchr() call, and return an error if so. FIXES 135445 Change-Id: I6bcbbaf88a16202fb84c0932578dd8d5712726dd Reviewed-on: https://gerrit.openafs.org/15911 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/libacl/aclprocs.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libacl/aclprocs.c b/src/libacl/aclprocs.c index ab6b21dac2..fbdfaa6f9c 100644 --- a/src/libacl/aclprocs.c +++ b/src/libacl/aclprocs.c @@ -278,6 +278,10 @@ acl_Internalize_pr(int (*func)(namelist *names, idlist *ids), char *elist, struc } (*acl)->entries[i].rights = k; nextc = strchr(nextc, '\n'); + if (nextc == NULL) { + free(lnames.namelist_val); + return (-1); + } nextc++; /* 1 + index can cast ptr to integer */ } j = i; @@ -290,6 +294,10 @@ acl_Internalize_pr(int (*func)(namelist *names, idlist *ids), char *elist, struc return (-1); } nextc = strchr(nextc, '\n'); + if (nextc == NULL) { + free(lnames.namelist_val); + return (-1); + } nextc++; } lids.idlist_len = 0; -- 2.45.2 From eb8b93a971c6293cdfbf8cd3d9a6351a8cb76f81 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 21 Aug 2024 00:29:34 -0500 Subject: [PATCH 3/8] 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; -- 2.45.2 From c9eae1e8b26144063e5d1db23d47ee82c4b9ef3a Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 21 Aug 2024 00:41:49 -0500 Subject: [PATCH 4/8] OPENAFS-SA-2024-002: viced: Avoid unchecked ACL in StoreACL audit log CVE-2024-10396 Currently in SRXAFS_StoreACL, if CallPreamble() or check_acl() fail, we will jump to Bad_StoreACL, which will pass the ACL string from the client to osi_auditU. Since check_acl() hasn't yet checked if the given ACL contains a NUL byte, the ACL may be an unterminated string. If auditing is enabled, this can cause garbage to be logged to the audit log, or cause the fileserver to crash. To avoid this, set 'rawACL' to NULL at first, only setting it to the actual ACL string after check_acl() has succeeded. This ensures that all code accessing 'rawACL' is guaranteed to be using a terminated string. This may mean that we pass a NULL AUD_ACL to osi_auditU. Our auditing code explicitly checks for and handles handles NULL strings, so this is fine. FIXES 135445 Change-Id: Iecde5677805a28d55c833b135732a14fd86cc985 Reviewed-on: https://gerrit.openafs.org/15913 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/viced/afsfileprocs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c index 3810655a35..290df17495 100644 --- a/src/viced/afsfileprocs.c +++ b/src/viced/afsfileprocs.c @@ -3136,7 +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; + char *rawACL = NULL; int newOpcode = (opcode == opcode_RXAFS_StoreACL); fsstats_StartOp(&fsstats, FS_STATS_RPCIDX_STOREACL); @@ -3148,6 +3148,7 @@ common_StoreACL(afs_uint64 opcode, if (errorCode != 0) { goto Bad_StoreACL; } + rawACL = uncheckedACL->AFSOpaque_val; /* Get ptr to client data for user Id for logging */ t_client = (struct client *)rx_GetSpecific(tcon, rxcon_client_key); -- 2.45.2 From 0b1ccb0dbc3b7673558eceff3d672971f5bb0197 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Mon, 4 Nov 2024 20:33:16 -0800 Subject: [PATCH 5/8] 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) -- 2.45.2 From 7e13414e8ea995d438cde3e60988225f3ab4cbcd Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Mon, 4 Nov 2024 20:50:50 -0800 Subject: [PATCH 6/8] OPENAFS-SA-2024-002: verify FetchACL returned only a string CVE-2024-10396 Supplement the previous commit by additionally verifying that the returned ACL string occupies the entire XDR opaque, rejecting any values returned that have an internal NUL prior to the end of the opaque. Change-Id: Iefa3d00a9a0e25ef66b7166fe952aae0603ee3d7 Reviewed-on: https://gerrit.openafs.org/15915 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/cm_ioctl.c | 3 +++ src/afs/afs_pioctl.c | 4 ++++ src/libafscp/afscp_acl.c | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index b8047b3dda..e8c31b5ac6 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -451,6 +451,9 @@ cm_IoctlGetACL(cm_ioctl_t *ioctlp, cm_user_t *userp, cm_scache_t *scp, cm_req_t if (acl.AFSOpaque_len == 0 || memchr(acl.AFSOpaque_val, '\0', acl.AFSOpaque_len) == NULL) return CM_ERROR_INVAL; + /* Reject "strings" with trailing data after the NUL. */ + if (strlen(acl.AFSOpaque_val) + 1 != acl.AFSOpaque_len) + 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 17715f4b49..f8a4715555 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1617,6 +1617,10 @@ DECL_PIOCTL(PGetAcl) /* Do not return an unterminated ACL string. */ code = EINVAL; + } else if (strlen(acl.AFSOpaque_val) + 1 != acl.AFSOpaque_len) { + /* Do not return an ACL string that has data beyond the trailing NUL. */ + code = EINVAL; + } else { afs_pd_skip(aout, acl.AFSOpaque_len); /* Length of the ACL */ } diff --git a/src/libafscp/afscp_acl.c b/src/libafscp/afscp_acl.c index f29be29813..5632feebf7 100644 --- a/src/libafscp/afscp_acl.c +++ b/src/libafscp/afscp_acl.c @@ -64,6 +64,10 @@ afscp_FetchACL(const struct afscp_venusfid *dir, struct AFSOpaque *acl) code = EIO; break; } + if (strlen(acl->AFSOpaque_val) + 1 != acl->AFSOpaque_len) { + code = EIO; + break; + } } } if (code >= 0) -- 2.45.2 From a4ecb050540528a1bff840ff08d21f99e6ef3fbf Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Mon, 4 Nov 2024 20:50:50 -0800 Subject: [PATCH 7/8] OPENAFS-SA-2024-002: make VIOCGETAL consumers stay within string bounds CVE-2024-10396 After the preceding commits, the data returned by the VIOCGETAL pioctl (a RXAFS_FetchAcl wrapper) will safely be NUL-terminated. However, the callers that attempt to parse the ACL string make assumptions that the returned data will be properly formatted, and implement a "skip to next line" functionality (under various names) that blindly increments a char* until it finds a newline character, which can read past the end of even a properly NUL-terminated string if there is not a newline where one is expected. Adjust the various "skip to next line" functionality to keep the current string pointer at the trailing NUL if the end of the string is reached while searching for a newline. Change-Id: I7fb7f23d7d6f68608f3e656a1530a7fc40b4a567 Reviewed-on: https://gerrit.openafs.org/15916 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/fs_acl.c | 5 +++-- src/kauth/kkids.c | 2 +- src/kauth/test/test_interim_ktc.c | 5 +++-- src/libadmin/client/afs_clientAdmin.c | 6 ++++++ src/sys/rmtsysnet.c | 5 +++-- src/uss/uss_acl.c | 5 +++-- src/venus/fs.c | 5 +++-- 7 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/WINNT/afsd/fs_acl.c b/src/WINNT/afsd/fs_acl.c index 8012891cd8..351fe05a4f 100644 --- a/src/WINNT/afsd/fs_acl.c +++ b/src/WINNT/afsd/fs_acl.c @@ -81,9 +81,10 @@ PruneList (struct AclEntry **ae, int dfs) static char * SkipLine (char *astr) { - while (*astr !='\n') + while (*astr != '\0' && *astr !='\n') astr++; - astr++; + if (*astr == '\n') + astr++; return astr; } diff --git a/src/kauth/kkids.c b/src/kauth/kkids.c index d5437f152f..ebc4c331e6 100644 --- a/src/kauth/kkids.c +++ b/src/kauth/kkids.c @@ -190,7 +190,7 @@ find_me(char *arg, char *parent_dir) return 1; /* found it */ } -#define SkipLine(str) { while (*str !='\n') str++; str++; } +#define SkipLine(str) do { while (*str != '\0' && *str !='\n') str++; if (*str == '\n') str++; } while(0) /* this function returns TRUE (1) if the file is in AFS, otherwise false (0) */ static int diff --git a/src/kauth/test/test_interim_ktc.c b/src/kauth/test/test_interim_ktc.c index 58cf061fd5..8af9035187 100644 --- a/src/kauth/test/test_interim_ktc.c +++ b/src/kauth/test/test_interim_ktc.c @@ -385,9 +385,10 @@ char * SkipLine(astr) char *astr; { - while (*astr != '\n') + while (*astr != '\0' && *astr != '\n') + astr++; + if (*astr == '\n') astr++; - astr++; return astr; } diff --git a/src/libadmin/client/afs_clientAdmin.c b/src/libadmin/client/afs_clientAdmin.c index c5192fe009..44c1382e0c 100644 --- a/src/libadmin/client/afs_clientAdmin.c +++ b/src/libadmin/client/afs_clientAdmin.c @@ -1652,9 +1652,13 @@ afsclient_ACLEntryAdd(const char *directory, const char *user, sscanf(old_acl_string, "%d dfs:%d %1024s", &cur_acl.nplus, &cur_acl.dfs, cur_acl.cell); ptr = strchr(old_acl_string, '\n'); + if (ptr == NULL) + goto fail_afsclient_ACLEntryAdd; ptr++; sscanf(ptr, "%d", &cur_acl.nminus); ptr = strchr(ptr, '\n'); + if (ptr == NULL) + goto fail_afsclient_ACLEntryAdd; ptr++; if (is_dfs == 3) { tst = ADMMISCNODFSACL; @@ -1681,6 +1685,8 @@ afsclient_ACLEntryAdd(const char *directory, const char *user, if (strcmp(cur_user, user)) { ptr = strchr(ptr, '\n'); + if (ptr == NULL) + goto fail_afsclient_ACLEntryAdd; ptr++; sprintf(tmp, "%s %d\n", cur_user, cur_user_acl); strcat(new_acl_string, tmp); diff --git a/src/sys/rmtsysnet.c b/src/sys/rmtsysnet.c index da6b3e64e4..bdb5c0d72a 100644 --- a/src/sys/rmtsysnet.c +++ b/src/sys/rmtsysnet.c @@ -55,9 +55,10 @@ struct ClearToken { char * RSkipLine(char *astr) { - while (*astr != '\n') + while (*astr != '\0' && *astr != '\n') + astr++; + if (*astr == '\n') astr++; - astr++; return astr; } diff --git a/src/uss/uss_acl.c b/src/uss/uss_acl.c index 003eff8b93..b3450ba2b2 100644 --- a/src/uss/uss_acl.c +++ b/src/uss/uss_acl.c @@ -339,9 +339,10 @@ static char * SkipLine(char *a_str) { /*SkipLine */ - while (*a_str != '\n') + while (*a_str != '\0' && *a_str != '\n') + a_str++; + if (*a_str == '\n') a_str++; - a_str++; return (a_str); } /*SkipLine */ diff --git a/src/venus/fs.c b/src/venus/fs.c index 3f2d5b453e..e0e5342d42 100644 --- a/src/venus/fs.c +++ b/src/venus/fs.c @@ -553,9 +553,10 @@ PruneList(struct AclEntry **ae, int dfs) static char * SkipLine(char *astr) { - while (*astr != '\n') + while (*astr != '\0' && *astr != '\n') + astr++; + if (*astr == '\n') astr++; - astr++; return astr; } -- 2.45.2 From ac602a0a5624b0f0ab04df86f618d09f2a4ad063 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Tue, 5 Nov 2024 23:40:24 -0600 Subject: [PATCH 8/8] OPENAFS-SA-2024-002: Avoid uninitialized memory when parsing ACLs CVE-2024-10396 Several places in the tree parse ACLs using sscanf() calls that look similar to this: sscanf(str, "%d dfs:%d %s", &nplus, &dfs, cell); sscanf(str, "%100s %d", tname, &trights); Some callers check whether the scanf() returns negative or 0, but some callers do not check the return code at all. If only some of the fields are present in the sscanf()'d string (because, for instance, the ACL is malformed), some of the arguments are left alone, and may be set to garbage if the relevant variable was never initialized. If the parsed ACL is copied to another ACL, this can result in the copied ACL containing uninitialized memory. To avoid this, make sure all of the variables passed to sscanf() and similar calls are initialized before parsing. This commit does not guarantee that the results make sense, but at least the results do not contain uninitialized memory. Change-Id: Ib0becffbce5a6e15f91ac4390bb9c422d478bea6 Reviewed-on: https://gerrit.openafs.org/15917 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- src/WINNT/afsd/fs_acl.c | 10 +++++----- src/kauth/kkids.c | 4 ++-- src/kauth/test/test_interim_ktc.c | 10 +++++----- src/libadmin/client/afs_clientAdmin.c | 3 +++ src/sys/rmtsysnet.c | 8 ++++---- src/uss/uss_acl.c | 8 ++++---- src/venus/fs.c | 12 ++++++------ 7 files changed, 29 insertions(+), 26 deletions(-) diff --git a/src/WINNT/afsd/fs_acl.c b/src/WINNT/afsd/fs_acl.c index 351fe05a4f..45218ae3d8 100644 --- a/src/WINNT/afsd/fs_acl.c +++ b/src/WINNT/afsd/fs_acl.c @@ -167,7 +167,7 @@ EmptyAcl(char *astr) struct Acl *tp; int junk; - tp = (struct Acl *)malloc(sizeof (struct Acl)); + tp = (struct Acl *)calloc(sizeof(*tp), 1); assert(tp); tp->nplus = tp->nminus = 0; tp->pluslist = tp->minuslist = 0; @@ -232,9 +232,9 @@ CleanAcl(struct Acl *aa, char *cellname) struct Acl * ParseAcl (char *astr, int astr_size) { - int nplus, nminus, i, trights, ret; + int nplus, nminus, i, trights = 0, ret; size_t len; - char tname[ACL_MAXNAME]; + char tname[ACL_MAXNAME + 1] = ""; struct AclEntry *first, *next, *last, *tl; struct Acl *ta; @@ -281,7 +281,7 @@ ParseAcl (char *astr, int astr_size) if (ret <= 0) goto nplus_err; astr = SkipLine(astr); - tl = (struct AclEntry *) malloc(sizeof (struct AclEntry)); + tl = (struct AclEntry *) calloc(sizeof(*tl), 1); if (tl == NULL) goto nplus_err; if (!first) @@ -309,7 +309,7 @@ ParseAcl (char *astr, int astr_size) if (ret <= 0) goto nminus_err; astr = SkipLine(astr); - tl = (struct AclEntry *) malloc(sizeof (struct AclEntry)); + tl = (struct AclEntry *) calloc(sizeof(*tl), 1); if (tl == NULL) goto nminus_err; if (!first) diff --git a/src/kauth/kkids.c b/src/kauth/kkids.c index ebc4c331e6..7e9043fdd6 100644 --- a/src/kauth/kkids.c +++ b/src/kauth/kkids.c @@ -228,7 +228,7 @@ struct AclEntry { static struct Acl * ParseAcl(char *astr) { - int nplus, nminus, i, trights; + int nplus = 0, nminus = 0, i, trights = 0; char tname[MAXNAME + 1] = ""; struct AclEntry *first, *last, *tl; struct Acl *ta; @@ -237,7 +237,7 @@ ParseAcl(char *astr) sscanf(astr, "%d", &nminus); SkipLine(astr); - ta = malloc(sizeof(struct Acl)); + ta = calloc(sizeof(*ta), 1); ta->nplus = nplus; last = 0; diff --git a/src/kauth/test/test_interim_ktc.c b/src/kauth/test/test_interim_ktc.c index 8af9035187..bc58f8668b 100644 --- a/src/kauth/test/test_interim_ktc.c +++ b/src/kauth/test/test_interim_ktc.c @@ -396,8 +396,8 @@ struct Acl * ParseAcl(astr) char *astr; { - int nplus, nminus, i, trights; - char tname[MAXNAME]; + int nplus = 0, nminus = 0, i, trights = 0; + char tname[MAXNAME + 1] = ""; struct AclEntry *first, *last, *tl; struct Acl *ta; sscanf(astr, "%d", &nplus); @@ -405,7 +405,7 @@ ParseAcl(astr) sscanf(astr, "%d", &nminus); astr = SkipLine(astr); - ta = malloc(sizeof(struct Acl)); + ta = calloc(sizeof(*ta), 1); ta->nplus = nplus; ta->nminus = nminus; @@ -414,7 +414,7 @@ ParseAcl(astr) for (i = 0; i < nplus; i++) { sscanf(astr, "%100s %d", tname, &trights); astr = SkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); @@ -431,7 +431,7 @@ ParseAcl(astr) for (i = 0; i < nminus; i++) { sscanf(astr, "%100s %d", tname, &trights); astr = SkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); diff --git a/src/libadmin/client/afs_clientAdmin.c b/src/libadmin/client/afs_clientAdmin.c index 44c1382e0c..2d46cf0751 100644 --- a/src/libadmin/client/afs_clientAdmin.c +++ b/src/libadmin/client/afs_clientAdmin.c @@ -1552,6 +1552,9 @@ afsclient_ACLEntryAdd(const char *directory, const char *user, char tmp[64 + 35]; int is_dfs; + memset(&cur_acl, 0, sizeof(cur_acl)); + memset(cur_user, 0, sizeof(cur_user)); + if (client_init == 0) { tst = ADMCLIENTNOINIT; goto fail_afsclient_ACLEntryAdd; diff --git a/src/sys/rmtsysnet.c b/src/sys/rmtsysnet.c index bdb5c0d72a..1e6911f59a 100644 --- a/src/sys/rmtsysnet.c +++ b/src/sys/rmtsysnet.c @@ -66,7 +66,7 @@ RSkipLine(char *astr) struct Acl * RParseAcl(char *astr) { - int nplus, nminus, i, trights; + int nplus = 0, nminus = 0, i, trights = 0; char tname[MAXNAME + 1] = ""; struct AclEntry *first, *last, *tl; struct Acl *ta; @@ -75,7 +75,7 @@ RParseAcl(char *astr) sscanf(astr, "%d", &nminus); astr = RSkipLine(astr); - ta = malloc(sizeof(struct Acl)); + ta = calloc(sizeof(*ta), 1); ta->nplus = nplus; ta->nminus = nminus; @@ -84,7 +84,7 @@ RParseAcl(char *astr) for (i = 0; i < nplus; i++) { sscanf(astr, "%" opr_stringize(MAXNAME) "s %d", tname, &trights); astr = RSkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); @@ -101,7 +101,7 @@ RParseAcl(char *astr) for (i = 0; i < nminus; i++) { sscanf(astr, "%" opr_stringize(MAXNAME) "s %d", tname, &trights); astr = RSkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); diff --git a/src/uss/uss_acl.c b/src/uss/uss_acl.c index b3450ba2b2..0eb4b53bca 100644 --- a/src/uss/uss_acl.c +++ b/src/uss/uss_acl.c @@ -405,7 +405,7 @@ static struct Acl * ParseAcl(char *a_str) { /*ParseAcl */ - int nplus, nminus, i, trights; + int nplus = 0, nminus = 0, i, trights = 0; char tname[MAXNAME + 1] = ""; struct AclEntry *first, *last, *tl; struct Acl *ta; @@ -422,7 +422,7 @@ ParseAcl(char *a_str) /* * Allocate and initialize the first entry. */ - ta = malloc(sizeof(struct Acl)); + ta = calloc(sizeof(*ta), 1); ta->nplus = nplus; ta->nminus = nminus; @@ -434,7 +434,7 @@ ParseAcl(char *a_str) for (i = 0; i < nplus; i++) { sscanf(a_str, "%" opr_stringize(MAXNAME) "s %d", tname, &trights); a_str = SkipLine(a_str); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); @@ -454,7 +454,7 @@ ParseAcl(char *a_str) for (i = 0; i < nminus; i++) { sscanf(a_str, "%" opr_stringize(MAXNAME) "s %d", tname, &trights); a_str = SkipLine(a_str); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); if (!first) first = tl; strcpy(tl->name, tname); diff --git a/src/venus/fs.c b/src/venus/fs.c index e0e5342d42..62bc2f5a00 100644 --- a/src/venus/fs.c +++ b/src/venus/fs.c @@ -573,7 +573,7 @@ EmptyAcl(char *astr) struct Acl *tp; int junk; - tp = malloc(sizeof(struct Acl)); + tp = calloc(sizeof(*tp), 1); assert(tp); tp->nplus = tp->nminus = 0; tp->pluslist = tp->minuslist = 0; @@ -585,12 +585,12 @@ EmptyAcl(char *astr) static struct Acl * ParseAcl(char *astr) { - int nplus, nminus, i, trights; - char tname[MAXNAME]; + int nplus = 0, nminus = 0, i, trights = 0; + char tname[MAXNAME + 1] = ""; struct AclEntry *first, *last, *tl; struct Acl *ta; - ta = malloc(sizeof(struct Acl)); + ta = calloc(sizeof(*ta), 1); assert(ta); ta->dfs = 0; sscanf(astr, "%d dfs:%d %1024s", &ta->nplus, &ta->dfs, ta->cell); @@ -606,7 +606,7 @@ ParseAcl(char *astr) for (i = 0; i < nplus; i++) { sscanf(astr, "%99s %d", tname, &trights); astr = SkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); assert(tl); if (!first) first = tl; @@ -624,7 +624,7 @@ ParseAcl(char *astr) for (i = 0; i < nminus; i++) { sscanf(astr, "%99s %d", tname, &trights); astr = SkipLine(astr); - tl = malloc(sizeof(struct AclEntry)); + tl = calloc(sizeof(*tl), 1); assert(tl); if (!first) first = tl; -- 2.45.2