volser: range check acl header fields during dumps and restores

Perform range checks on the acl header fields when reading an
acl from a dump stream and when writing an acl to a dump
stream.

Before this change, a bogus value in the total, positive, or
negative acl fields from a dump stream could cause an out of
bounds access of the acl entries table, crashing the volume
server.

Reviewed-on: http://gerrit.openafs.org/11702
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
(cherry picked from commit 0bf9fba458b39035a09f45c1b63f1e65672d4c00)

Change-Id: Icebeb1d62900a7978f02177627a30e41de49a182
Reviewed-on: https://gerrit.openafs.org/12127
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>

diff --git a/src/libacl/netprocs.c b/src/libacl/netprocs.c
index f9aec80afa430c77dbe5ac8b6ff1bb923f611910..19dc6a367f43db38ebe08e09d4182fa04b1f6a4f 100644 (file)
--- a/src/libacl/netprocs.c
+++ b/src/libacl/netprocs.c
@@ -16,6 +16,7 @@
 #include <afsconfig.h>
 #include <afs/param.h>
 
+#include <errno.h>
 
 #include <sys/types.h>
 #ifdef AFS_NT40_ENV
@@ -28,14 +29,53 @@
 #include <afs/ptclient.h>
 #include "acl.h"
 
+/*!
+ * Sanity check the access list.
+ *
+ * \param acl   pointer to the acl structure to be checked
+ *
+ * \returns  0 if acl list is valid
+ *   \retval 0       success
+ *   \retval EINVAL  invalid values in the acl header
+ */
+static int
+CheckAccessList(struct acl_accessList *acl)
+{
+    if (acl->total < 0 || acl->total > ACL_MAXENTRIES) {
+       return EINVAL;
+    }
+    if (acl->positive < 0 || acl->negative < 0
+       || (acl->positive + acl->negative) != acl->total) {
+       return EINVAL;
+    }
+    /* Note: size may exceed sizeof(struct acl_accessList). */
+    if (acl->size < sizeof(struct acl_accessList)
+       + (acl->total - 1) * sizeof(struct acl_accessEntry)) {
+       return EINVAL;
+    }
+    return 0;
+}
+
+/*!
+ * Convert the access list to network byte order.
+ *
+ * \param acl   pointer to the acl structure to be converted
+ *
+ * \returns  zero on success
+ *   \retval 0       success
+ *   \retval EINVAL  invalid values in the acl header
+ */
 int
 acl_HtonACL(struct acl_accessList *acl)
 {
-    /* Converts the access list defined by acl to network order.  Returns 0 always. */
-
     int i;
-    if (htonl(1) == 1)
-       return (0);             /* no swapping needed */
+    int code;
+
+    code = CheckAccessList(acl);
+    if (code) {
+       return code;
+    }
+
     for (i = 0; i < acl->positive; i++) {
        acl->entries[i].id = htonl(acl->entries[i].id);
        acl->entries[i].rights = htonl(acl->entries[i].rights);
@@ -52,19 +92,32 @@ acl_HtonACL(struct acl_accessList *acl)
     return (0);
 }
 
+/*!
+ * Convert the access list to host byte order.
+ *
+ * \param acl   pointer to the acl structure to be converted
+ *
+ * \returns  zero on success
+ *   \retval 0       success
+ *   \retval EINVAL  invalid values in the acl header
+ */
 int
 acl_NtohACL(struct acl_accessList *acl)
 {
-    /* Converts the access list defined by acl to network order. Returns 0 always. */
-
     int i;
-    if (ntohl(1) == 1)
-       return (0);             /* no swapping needed */
+    int code;
+
     acl->size = ntohl(acl->size);
     acl->version = ntohl(acl->version);
     acl->total = ntohl(acl->total);
     acl->positive = ntohl(acl->positive);
     acl->negative = ntohl(acl->negative);
+
+    code = CheckAccessList(acl);
+    if (code) {
+       return code;
+    }
+
     for (i = 0; i < acl->positive; i++) {
        acl->entries[i].id = ntohl(acl->entries[i].id);
        acl->entries[i].rights = ntohl(acl->entries[i].rights);

diff --git a/src/volser/dumpstuff.c b/src/volser/dumpstuff.c
index 915b70a378772f98f6107d1592e17be1f42827aa..b37301d37bdbef05a66c4d7b5c44d4cb57a8cc29 100644 (file)
--- a/src/volser/dumpstuff.c
+++ b/src/volser/dumpstuff.c
@@ -1095,7 +1095,11 @@ DumpVnode(struct iod *iodp, struct VnodeDiskObject *v, int volid,
     if (!code)
        code = DumpInt32(iodp, 's', v->serverModifyTime);
     if (v->type == vDirectory) {
-       acl_HtonACL(VVnodeDiskACL(v));
+       code = acl_HtonACL(VVnodeDiskACL(v));
+       if (code) {
+           Log("DumpVnode: Skipping invalid acl vnode %u (volume %i)\n",
+                vnodeNumber, volid);
+       }
        if (!code)
            code =
                DumpByteString(iodp, 'A', (byte *) VVnodeDiskACL(v),
@@ -1420,7 +1424,11 @@ ReadVnodes(struct iod *iodp, Volume * vp, int incremental,
            case 'A':
                ReadByteString(iodp, (byte *) VVnodeDiskACL(vnode),
                               VAclDiskSize(vnode));
-               acl_NtohACL(VVnodeDiskACL(vnode));
+               if (acl_NtohACL(VVnodeDiskACL(vnode)) != 0) {
+                   Log("ReadVnodes: invalid acl for vnode %lu in dump.\n",
+                        (unsigned long)vnodeNumber);
+                   return VOLSERREAD_DUMPERROR;
+               }
                break;
            case 'h':
            case 'f':{

diff --git a/src/volser/vol-dump.c b/src/volser/vol-dump.c
index 0820e2544395182122d4513315b58d0549651a1e..ac7a76383d4565d6e1ed368c162fa29469a1b6e7 100644 (file)
--- a/src/volser/vol-dump.c
+++ b/src/volser/vol-dump.c
@@ -742,7 +742,11 @@ DumpVnode(int dumpfd, struct VnodeDiskObject *v, int volid, int vnodeNumber,
     if (!code)
        code = DumpInt32(dumpfd, 's', v->serverModifyTime);
     if (v->type == vDirectory) {
-       acl_HtonACL(VVnodeDiskACL(v));
+       code = acl_HtonACL(VVnodeDiskACL(v));
+       if (code) {
+           fprintf(stderr, "Skipping invalid acl in vnode %u (volume %i)\n",
+                   vnodeNumber, volid);
+       }
        if (!code)
            code =
                DumpByteString(dumpfd, 'A', (byte *) VVnodeDiskACL(v),