From a4ecb050540528a1bff840ff08d21f99e6ef3fbf Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Mon, 4 Nov 2024 20:50:50 -0800 Subject: [PATCH] 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; }