volser: range check acl header fields during dumps and restores
authorMichael Meffie <mmeffie@sinenomine.net>
Fri, 30 Jan 2015 17:12:03 +0000 (12:12 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Thu, 3 Dec 2015 05:20:54 +0000 (00:20 -0500)
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.

Change-Id: Ic7d7f615a37491835af8d92f3c5f1b6a667d9d01
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>

src/libacl/netprocs.c
src/volser/dumpstuff.c
src/volser/vol-dump.c

index f41ad55..7aaae0c 100644 (file)
 
 #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);
@@ -48,19 +87,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);
index 8c01aa0..e1754d1 100644 (file)
@@ -1093,7 +1093,11 @@ DumpVnode(struct iod *iodp, struct VnodeDiskObject *v, VolumeId 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 %"AFS_VOLID_FMT")\n",
+                vnodeNumber, afs_printable_VolumeId_lu(volid));
+       }
        if (!code)
            code =
                DumpByteString(iodp, 'A', (byte *) VVnodeDiskACL(v),
@@ -1426,7 +1430,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':{
index 554ecf0..1efbbaf 100644 (file)
@@ -713,7 +713,11 @@ DumpVnode(int dumpfd, struct VnodeDiskObject *v, VolumeId 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 %"AFS_VOLID_FMT")\n",
+                       vnodeNumber, afs_printable_VolumeId_lu(volid));
+       }
        if (!code)
            code =
                DumpByteString(dumpfd, 'A', (byte *) VVnodeDiskACL(v),