From 19f8f35949cf8fc7e6b91afc0d29deb0d4db4d99 Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Mon, 18 Sep 2023 16:13:57 -0500
Subject: [PATCH 01/10] OPENAFS-SA-2024-002: viced: Refuse ACLs without '\0' in
 SRXAFS_StoreACL

CVE-2024-10396

Currently, the fileserver treats the ACL given in RXAFS_StoreACL as a
string, even though it is technically an AFSOpaque and could be not
NUL-terminated.

We give the ACL opaque/string to acl_Internalize_pr() to parse, which
will run off the end of the allocated buffer if the given ACL does not
contain a '\0' character. Usually this will result in a parse error
since we'll encounter garbage, but if the partially-garbage ACL
happens to parse successfully, some uninitialized data could make it
into the stored ACL.

In addition, if the given ACL is an opaque of length 0, we'll still
give the opaque pointer to acl_Internalize_pr(). In this case, the
pointer will point to &memZero, which happens to contain a NUL byte,
and so is treated like an empty string (which is not a valid ACL). But
the fact that this causes no problems is somewhat a coincidence, and
so should also be avoided.

To avoid both of these situations, just check if the given ACL string
contains a NUL byte. If it doesn't, or if it has length 0, refuse to
look at it and abort the call with EINVAL.

FIXES 135445

Reviewed-on: https://gerrit.openafs.org/15908
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit e15decb318797f1d471588dc669c3e3b26f1b8b3)

Reviewed-on: https://gerrit.openafs.org/15929
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit f74f960a18f559e683d6a1f5104e43c3ca93ecb8)

Change-Id: I451b0c7b1e983c464dc05ed6bf7ef603bd7ad3ac
---
 src/viced/afsfileprocs.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c
index e145d67be7..eacf631490 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -3462,6 +3462,36 @@ SRXAFS_StoreData64(struct rx_call * acall, struct AFSFid * Fid,
     return code;
 }
 
+/**
+ * Check if the given ACL blob is okay to use.
+ *
+ * If the given ACL is 0-length, or doesn't contain a NUL byte, return an error
+ * and refuse the process the given ACL. The ACL must contain a NUL byte,
+ * otherwise ACL-processing code may run off the end of the buffer and process
+ * uninitialized memory.
+ *
+ * If there is any data beyond the NUL byte, just ignore it and return success.
+ * We won't look at any post-NUL data, but theoretically clients could send
+ * such an ACL to us, since historically it's been allowed.
+ *
+ * @param[in] AccessList    The ACL blob to check
+ *
+ * @returns errno error code to abort the call with
+ * @retval 0 ACL is okay to use
+ */
+static afs_int32
+check_acl(struct AFSOpaque *AccessList)
+{
+    if (AccessList->AFSOpaque_len == 0) {
+	return EINVAL;
+    }
+    if (memchr(AccessList->AFSOpaque_val, '\0',
+	       AccessList->AFSOpaque_len) == NULL) {
+	return EINVAL;
+    }
+    return 0;
+}
+
 afs_int32
 SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
 		struct AFSOpaque * AccessList,
@@ -3496,6 +3526,11 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     if ((errorCode = CallPreamble(acall, ACTIVECALL, Fid, &tcon, &thost)))
 	goto Bad_StoreACL;
 
+    errorCode = check_acl(AccessList);
+    if (errorCode != 0) {
+	goto Bad_StoreACL;
+    }
+
     /* 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);
-- 
2.45.2


From cbc56aaa8af0b422c9545e66b1823e73244d2535 Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Mon, 18 Sep 2023 16:14:07 -0500
Subject: [PATCH 02/10] OPENAFS-SA-2024-002: viced: Free ACL on
 acl_Internalize_pr error

CVE-2024-10396

Currently, we don't free 'newACL' if acl_Internalize_pr() fails. If
acl_Internalize_pr() has already allocated 'newACL', then the memory
associated with newACL will be leaked. This can happen if parsing the
given ACL fails at any point after successfully parsing the first
couple of lines in the ACL.

Change acl_FreeACL() to make freeing a NULL acl a no-op, to make it
easier to make sure the acl has been freed.

FIXES 135445

Reviewed-on: https://gerrit.openafs.org/15909
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit f4dfc2d7183f126bc4a45b5cabc78c3de020925f)

Reviewed-on: https://gerrit.openafs.org/15930
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit a07e50726df09c49dfe7b953c3e49eb98f310c09)

Change-Id: I24fc132a91ba9867f7c5ba0fdbd5ae150bc358a7
---
 src/libacl/aclprocs.c    |  4 ++++
 src/viced/afsfileprocs.c | 20 ++++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/libacl/aclprocs.c b/src/libacl/aclprocs.c
index 72bf8aebc9..b6d8c6bb90 100644
--- a/src/libacl/aclprocs.c
+++ b/src/libacl/aclprocs.c
@@ -122,6 +122,10 @@ acl_FreeACL(struct acl_accessList **acl)
     /* Releases the access list defined by acl.  Returns 0 always. */
     struct freeListEntry *x;
 
+    if (*acl == NULL) {
+	return 0;
+    }
+
     x = (struct freeListEntry *)
 	((char *)*acl - sizeof(struct freeListEntry *) - sizeof(int));
     *acl = NULL;
diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c
index eacf631490..5d62d00546 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -1277,16 +1277,24 @@ RXFetch_AccessList(Vnode * targetptr, Vnode * parentwhentargetnotdir,
 static afs_int32
 RXStore_AccessList(Vnode * targetptr, struct AFSOpaque *AccessList)
 {
-    struct acl_accessList *newACL;	/* PlaceHolder for new access list */
+    int code;
+    struct acl_accessList *newACL = NULL;
 
     if (acl_Internalize_pr(hpr_NameToId, AccessList->AFSOpaque_val, &newACL)
-	!= 0)
-	return (EINVAL);
-    if ((newACL->size + 4) > VAclSize(targetptr))
-	return (E2BIG);
+	!= 0) {
+	code = EINVAL;
+	goto done;
+    }
+    if ((newACL->size + 4) > VAclSize(targetptr)) {
+	code = E2BIG;
+	goto done;
+    }
     memcpy((char *)VVnodeACL(targetptr), (char *)newACL, (int)(newACL->size));
+    code = 0;
+
+ done:
     acl_FreeACL(&newACL);
-    return (0);
+    return code;
 
 }				/*RXStore_AccessList */
 
-- 
2.45.2


From a1a1c258c9c9825e038eec0df923b77c4728f6a9 Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Tue, 19 Sep 2023 15:44:08 -0500
Subject: [PATCH 03/10] 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

Reviewed-on: https://gerrit.openafs.org/15910
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 35d218c1d17973c1412ea5dff1e23d9aae50c4c7)

Reviewed-on: https://gerrit.openafs.org/15931
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 1e6e813188ecce62eb7af19385d911f63469bdb6)

Change-Id: Id70301ae16ab0d2aa0b7183cd9b469d7ff0b889a
---
 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 b6d8c6bb90..dca7c18c1d 100644
--- a/src/libacl/aclprocs.c
+++ b/src/libacl/aclprocs.c
@@ -274,10 +274,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 32ef2dcedb78832e7d40e5e1997ccdb61a4d0e2d Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Tue, 19 Sep 2023 15:55:42 -0500
Subject: [PATCH 04/10] 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

Reviewed-on: https://gerrit.openafs.org/15911
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 96ab2c6f8a614d597a523b45871c5f64a50a7040)

Reviewed-on: https://gerrit.openafs.org/15932
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit d66caf8c04878724001839317637445708edef2c)

Change-Id: Ie904de0abdb72d812abeca92b3bd9cda667c63a4
---
 src/libacl/aclprocs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/libacl/aclprocs.c b/src/libacl/aclprocs.c
index dca7c18c1d..2b5c38e9ca 100644
--- a/src/libacl/aclprocs.c
+++ b/src/libacl/aclprocs.c
@@ -288,6 +288,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;
@@ -300,6 +304,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 4ffe7b60f1510affaad43cd152098e54e8831580 Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Wed, 21 Aug 2024 00:29:34 -0500
Subject: [PATCH 05/10] 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

Reviewed-on: https://gerrit.openafs.org/15912
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit eb8b93a971c6293cdfbf8cd3d9a6351a8cb76f81)

[1.8: printableACL() does not exist in this branch.]

Reviewed-on: https://gerrit.openafs.org/15933
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit ee020f7cba7d82bc3d4b468210b5052af53c5db5)

Change-Id: Icf18d600168b7ba5e2356e86479597ae650f67fa
---
 src/viced/afsfileprocs.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c
index 5d62d00546..f766de09c4 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -1275,12 +1275,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;
@@ -3502,7 +3502,7 @@ check_acl(struct AFSOpaque *AccessList)
 
 afs_int32
 SRXAFS_StoreACL(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 */
@@ -3516,6 +3516,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     struct host *thost;
     struct client *t_client = NULL;	/* tmp ptr to client data */
     struct in_addr logHostAddr;	/* host ip holder for inet_ntoa */
+    char *rawACL = uncheckedACL->AFSOpaque_val;
 #if FS_STATS_DETAILED
     struct fs_stats_opTimingData *opP;	/* Ptr to this op's timing struct */
     struct timeval opStartTime, opStopTime;	/* Start/stop times for RPC op */
@@ -3534,7 +3535,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     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;
     }
@@ -3544,7 +3545,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     logHostAddr.s_addr = rxr_HostOf(tcon);
     ViceLog(1,
 	    ("SAFS_StoreACL, Fid = %u.%u.%u, ACL=%s, Host %s:%d, Id %d\n",
-	     Fid->Volume, Fid->Vnode, Fid->Unique, AccessList->AFSOpaque_val,
+	     Fid->Volume, Fid->Vnode, Fid->Unique, rawACL,
 	     inet_ntoa(logHostAddr), ntohs(rxr_PortOf(tcon)), t_client->ViceId));
     FS_LOCK;
     AFSCallStats.StoreACL++, AFSCallStats.TotalCalls++;
@@ -3573,7 +3574,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     }
 
     /* 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;
     }
 
@@ -3618,7 +3619,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
 
     osi_auditU(acall, StoreACLEvent, errorCode,
                AUD_ID, t_client ? t_client->ViceId : 0,
-               AUD_FID, Fid, AUD_ACL, AccessList->AFSOpaque_val, AUD_END);
+               AUD_FID, Fid, AUD_ACL, rawACL, AUD_END);
     return errorCode;
 
 }				/*SRXAFS_StoreACL */
-- 
2.45.2


From 1db8e2f9c720ee2dda6eb65d4460137f3d92cbb2 Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Wed, 21 Aug 2024 00:41:49 -0500
Subject: [PATCH 06/10] 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

Reviewed-on: https://gerrit.openafs.org/15913
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit c9eae1e8b26144063e5d1db23d47ee82c4b9ef3a)

Reviewed-on: https://gerrit.openafs.org/15934
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit bb01d76a2095baa65880bdc5d504e7a198958265)

Change-Id: I2e078a2642b4b8d9c66bcebaee4f1977eef05ca5
---
 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 f766de09c4..2634e5fc35 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -3516,7 +3516,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     struct host *thost;
     struct client *t_client = NULL;	/* tmp ptr to client data */
     struct in_addr logHostAddr;	/* host ip holder for inet_ntoa */
-    char *rawACL = uncheckedACL->AFSOpaque_val;
+    char *rawACL = NULL;
 #if FS_STATS_DETAILED
     struct fs_stats_opTimingData *opP;	/* Ptr to this op's timing struct */
     struct timeval opStartTime, opStopTime;	/* Start/stop times for RPC op */
@@ -3539,6 +3539,7 @@ SRXAFS_StoreACL(struct rx_call * acall, struct AFSFid * Fid,
     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 ab1ac1cfeba4a7a74499407319e8120737d7351c Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk <kaduk@mit.edu>
Date: Mon, 4 Nov 2024 20:33:16 -0800
Subject: [PATCH 07/10] 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.

Reviewed-on: https://gerrit.openafs.org/15914
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 0b1ccb0dbc3b7673558eceff3d672971f5bb0197)

Reviewed-on: https://gerrit.openafs.org/15935
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 64068705b15661a8d4e0b9f9f2ad4aec34ed51a7)

Change-Id: I2a26ac00fd62466c4ebaf1323fc03ff236b15bee
---
 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 172cbc082e..ebf43e0120 100644
--- a/src/WINNT/afsd/cm_ioctl.c
+++ b/src/WINNT/afsd/cm_ioctl.c
@@ -427,6 +427,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 24c0cc7534..07385f7eae 100644
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -1607,10 +1607,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 09b7df000f..929aabd7d8 100644
--- a/src/libafscp/afscp_acl.c
+++ b/src/libafscp/afscp_acl.c
@@ -56,6 +56,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 d559dad5a76ca977e643c870f413890e8dee7e80 Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk <kaduk@mit.edu>
Date: Mon, 4 Nov 2024 20:50:50 -0800
Subject: [PATCH 08/10] 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.

Reviewed-on: https://gerrit.openafs.org/15915
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 7e13414e8ea995d438cde3e60988225f3ab4cbcd)

Reviewed-on: https://gerrit.openafs.org/15936
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit a96a3160f5425125588f39f5ac612df3ef9b9a8a)

Change-Id: I7cb16fa73e4b1ad511cdb3b2c2d55aa7e93a1f7b
---
 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 ebf43e0120..0f524ffe0d 100644
--- a/src/WINNT/afsd/cm_ioctl.c
+++ b/src/WINNT/afsd/cm_ioctl.c
@@ -431,6 +431,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 07385f7eae..f24b03b07e 100644
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -1614,6 +1614,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 929aabd7d8..1bbacd0f9f 100644
--- a/src/libafscp/afscp_acl.c
+++ b/src/libafscp/afscp_acl.c
@@ -62,6 +62,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 8bba6910e1f2feb21782dd92c9a3c60c3f2a360a Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk <kaduk@mit.edu>
Date: Mon, 4 Nov 2024 20:50:50 -0800
Subject: [PATCH 09/10] 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.

Reviewed-on: https://gerrit.openafs.org/15916
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit a4ecb050540528a1bff840ff08d21f99e6ef3fbf)

Reviewed-on: https://gerrit.openafs.org/15937
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit a9ede52673b8c8abbfc2577ac6987a8a5686206f)

Change-Id: I2e22cc3613d9e182418a0a79ab584cc3b317c6a5
---
 src/WINNT/afsd/fs.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.c b/src/WINNT/afsd/fs.c
index 5331b3072f..22b58b2365 100644
--- a/src/WINNT/afsd/fs.c
+++ b/src/WINNT/afsd/fs.c
@@ -547,9 +547,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;
 }
 
diff --git a/src/kauth/kkids.c b/src/kauth/kkids.c
index f54dc5408d..f7291b66b6 100644
--- a/src/kauth/kkids.c
+++ b/src/kauth/kkids.c
@@ -198,7 +198,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 bd3f4cb4a2..aae456ed6e 100644
--- a/src/kauth/test/test_interim_ktc.c
+++ b/src/kauth/test/test_interim_ktc.c
@@ -386,9 +386,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 35dbd6c90a..699a79440b 100644
--- a/src/libadmin/client/afs_clientAdmin.c
+++ b/src/libadmin/client/afs_clientAdmin.c
@@ -1545,9 +1545,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;
@@ -1574,6 +1578,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 df350d21b7..b630e55dfe 100644
--- a/src/sys/rmtsysnet.c
+++ b/src/sys/rmtsysnet.c
@@ -64,9 +64,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 30a696eaa4..2119ef59df 100644
--- a/src/uss/uss_acl.c
+++ b/src/uss/uss_acl.c
@@ -343,9 +343,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 9bdd1fba15..5651162422 100644
--- a/src/venus/fs.c
+++ b/src/venus/fs.c
@@ -537,9 +537,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 11706b3bcacf80862ff454c55f9ace62426d7c2f Mon Sep 17 00:00:00 2001
From: Andrew Deason <adeason@sinenomine.net>
Date: Tue, 5 Nov 2024 23:40:24 -0600
Subject: [PATCH 10/10] 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.

Reviewed-on: https://gerrit.openafs.org/15917
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit ac602a0a5624b0f0ab04df86f618d09f2a4ad063)

Reviewed-on: https://gerrit.openafs.org/15938
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 21941c0ab2d28fa3a074f46e4d448d518a7c1b8a)

Change-Id: I6b5af8544f5d14597b8143fa6e349b30bf604777
---
 src/WINNT/afsd/fs.c                   | 10 +++++-----
 src/kauth/kkids.c                     |  6 +++---
 src/kauth/test/test_interim_ktc.c     | 10 +++++-----
 src/libadmin/client/afs_clientAdmin.c |  3 +++
 src/sys/rmtsysnet.c                   | 10 +++++-----
 src/uss/uss_acl.c                     | 10 +++++-----
 src/venus/fs.c                        | 12 ++++++------
 7 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/src/WINNT/afsd/fs.c b/src/WINNT/afsd/fs.c
index 22b58b2365..704fa5639a 100644
--- a/src/WINNT/afsd/fs.c
+++ b/src/WINNT/afsd/fs.c
@@ -567,7 +567,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;
@@ -589,9 +589,9 @@ EmptyAcl(char *astr)
 static 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[MAXNAME];
+    char tname[MAXNAME + 1] = "";
     struct AclEntry *first, *next, *last, *tl;
     struct Acl *ta;
 
@@ -638,7 +638,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)
@@ -666,7 +666,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 f7291b66b6..2ebc56b7bb 100644
--- a/src/kauth/kkids.c
+++ b/src/kauth/kkids.c
@@ -236,8 +236,8 @@ struct AclEntry {
 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;
     sscanf(astr, "%d", &nplus);
@@ -245,7 +245,7 @@ ParseAcl(char *astr)
     sscanf(astr, "%d", &nminus);
     SkipLine(astr);
 
-    ta = (struct Acl *)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 aae456ed6e..20463bf03e 100644
--- a/src/kauth/test/test_interim_ktc.c
+++ b/src/kauth/test/test_interim_ktc.c
@@ -397,8 +397,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);
@@ -406,7 +406,7 @@ ParseAcl(astr)
     sscanf(astr, "%d", &nminus);
     astr = SkipLine(astr);
 
-    ta = (struct Acl *)malloc(sizeof(struct Acl));
+    ta = calloc(sizeof(*ta), 1);
     ta->nplus = nplus;
     ta->nminus = nminus;
 
@@ -415,7 +415,7 @@ ParseAcl(astr)
     for (i = 0; i < nplus; i++) {
 	sscanf(astr, "%100s %d", tname, &trights);
 	astr = SkipLine(astr);
-	tl = (struct AclEntry *)malloc(sizeof(struct AclEntry));
+	tl = calloc(sizeof(*tl), 1);
 	if (!first)
 	    first = tl;
 	strcpy(tl->name, tname);
@@ -432,7 +432,7 @@ ParseAcl(astr)
     for (i = 0; i < nminus; i++) {
 	sscanf(astr, "%100s %d", tname, &trights);
 	astr = SkipLine(astr);
-	tl = (struct AclEntry *)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 699a79440b..cad0dca45a 100644
--- a/src/libadmin/client/afs_clientAdmin.c
+++ b/src/libadmin/client/afs_clientAdmin.c
@@ -1445,6 +1445,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 b630e55dfe..ce1c9f337f 100644
--- a/src/sys/rmtsysnet.c
+++ b/src/sys/rmtsysnet.c
@@ -75,8 +75,8 @@ RSkipLine(char *astr)
 struct Acl *
 RParseAcl(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);
@@ -84,7 +84,7 @@ RParseAcl(char *astr)
     sscanf(astr, "%d", &nminus);
     astr = RSkipLine(astr);
 
-    ta = (struct Acl *)malloc(sizeof(struct Acl));
+    ta = calloc(sizeof(*ta), 1);
     ta->nplus = nplus;
     ta->nminus = nminus;
 
@@ -93,7 +93,7 @@ RParseAcl(char *astr)
     for (i = 0; i < nplus; i++) {
 	sscanf(astr, "%100s %d", tname, &trights);
 	astr = RSkipLine(astr);
-	tl = (struct AclEntry *)malloc(sizeof(struct AclEntry));
+	tl = calloc(sizeof(*tl), 1);
 	if (!first)
 	    first = tl;
 	strcpy(tl->name, tname);
@@ -110,7 +110,7 @@ RParseAcl(char *astr)
     for (i = 0; i < nminus; i++) {
 	sscanf(astr, "%100s %d", tname, &trights);
 	astr = RSkipLine(astr);
-	tl = (struct AclEntry *)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 2119ef59df..3690208ec0 100644
--- a/src/uss/uss_acl.c
+++ b/src/uss/uss_acl.c
@@ -409,8 +409,8 @@ static struct Acl *
 ParseAcl(char *a_str)
 {				/*ParseAcl */
 
-    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;
 
@@ -426,7 +426,7 @@ ParseAcl(char *a_str)
     /*
      * Allocate and initialize the first entry.
      */
-    ta = (struct Acl *)malloc(sizeof(struct Acl));
+    ta = calloc(sizeof(*ta), 1);
     ta->nplus = nplus;
     ta->nminus = nminus;
 
@@ -438,7 +438,7 @@ ParseAcl(char *a_str)
     for (i = 0; i < nplus; i++) {
 	sscanf(a_str, "%100s %d", tname, &trights);
 	a_str = SkipLine(a_str);
-	tl = (struct AclEntry *)malloc(sizeof(struct AclEntry));
+	tl = calloc(sizeof(*tl), 1);
 	if (!first)
 	    first = tl;
 	strcpy(tl->name, tname);
@@ -458,7 +458,7 @@ ParseAcl(char *a_str)
     for (i = 0; i < nminus; i++) {
 	sscanf(a_str, "%100s %d", tname, &trights);
 	a_str = SkipLine(a_str);
-	tl = (struct AclEntry *)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 5651162422..1e7acaac80 100644
--- a/src/venus/fs.c
+++ b/src/venus/fs.c
@@ -557,7 +557,7 @@ EmptyAcl(char *astr)
     struct Acl *tp;
     int junk;
 
-    tp = (struct Acl *)malloc(sizeof(struct Acl));
+    tp = calloc(sizeof(*tp), 1);
     assert(tp);
     tp->nplus = tp->nminus = 0;
     tp->pluslist = tp->minuslist = 0;
@@ -569,12 +569,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 = (struct Acl *)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);
@@ -590,7 +590,7 @@ ParseAcl(char *astr)
     for (i = 0; i < nplus; i++) {
 	sscanf(astr, "%99s %d", tname, &trights);
 	astr = SkipLine(astr);
-	tl = (struct AclEntry *)malloc(sizeof(struct AclEntry));
+	tl = calloc(sizeof(*tl), 1);
 	assert(tl);
 	if (!first)
 	    first = tl;
@@ -608,7 +608,7 @@ ParseAcl(char *astr)
     for (i = 0; i < nminus; i++) {
 	sscanf(astr, "%99s %d", tname, &trights);
 	astr = SkipLine(astr);
-	tl = (struct AclEntry *)malloc(sizeof(struct AclEntry));
+	tl = calloc(sizeof(*tl), 1);
 	assert(tl);
 	if (!first)
 	    first = tl;
-- 
2.45.2