From ac602a0a5624b0f0ab04df86f618d09f2a4ad063 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Tue, 5 Nov 2024 23:40:24 -0600 Subject: [PATCH] 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;