Don't trust # of entries from ListAttributes
authorSimon Wilkinson <sxw@your-file-system.com>
Sun, 26 Dec 2010 14:54:43 +0000 (14:54 +0000)
committerDerrick Brashear <shadow@dementia.org>
Mon, 27 Dec 2010 20:21:54 +0000 (12:21 -0800)
ListAttributes returns the number of entries in its array as an RPC
argument. But, we can't trust this, as it could be manipulated and
end up pointing past the end of the returned array (which is counted,
so the entries argument is actually pointless).

Add bounds checking to the functions which use this value to prevent
this problem.

Change-Id: I62398d8f6b5c54318c1a42f1bad67a21c90ef944
Reviewed-on: http://gerrit.openafs.org/3597
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

src/bucoord/commands.c
src/libadmin/vos/vosutils.c
src/volser/vsutils.c

index 324510d..5e4ba53 100644 (file)
@@ -259,6 +259,11 @@ EvalVolumeSet2(struct bc_config *aconfig,
            if (nentries == 0)
                ERROR(RXGEN_OPCODE);    /* Use EvalVolumeSet1 */
 
+           if (nentries < 0)
+               nentries = 0;
+           if (nentries > bulkentries.nbulkentries_len)
+               nentries = bulkentries.nbulkentries_len;
+
            /* Step through each entry and add it to the list of volumes */
            entries = bulkentries.nbulkentries_val;
            for (e = 0; e < nentries; e++) {
index c2325bc..9f18ab6 100644 (file)
@@ -259,6 +259,10 @@ VLDB_ListAttributes(afs_cell_handle_p cellHandle,
                    cellHandle->vos_new = 0;
                }
            } else {
+               if (*entriesp < 0)
+                   *entriesp = 0;
+               if (*entriesp > blkentriesp->nbulkentries_len)
+                   *entriesp = blkentriesp->nbulkentries_len;
                rc = 1;
            }
        } else {
@@ -267,6 +271,12 @@ VLDB_ListAttributes(afs_cell_handle_p cellHandle,
                ubik_VL_ListAttributes(cellHandle->vos, 0, attrp,
                          entriesp, &arrayEntries);
            if (tst == 0) {
+
+               if (*entriesp < 0)
+                   *entriesp = 0;
+               if (*entriesp > arrayEntries.bulkentries_len)
+                   *entriesp = arrayEntries.bulkentries_len;
+
                blkentriesp->nbulkentries_val =
                    (nvldbentry *) malloc(*entriesp * sizeof(*blkentriesp));
                if (blkentriesp->nbulkentries_val != NULL) {
@@ -282,6 +292,7 @@ VLDB_ListAttributes(afs_cell_handle_p cellHandle,
                if (arrayEntries.bulkentries_val) {
                    free(arrayEntries.bulkentries_val);
                }
+
                rc = 1;
            }
        }
index 447d39a..33ecefc 100644 (file)
@@ -484,6 +484,12 @@ VLDB_ListAttributes(VldbListByAttributes *attrp,
        if (code)
            return code;
 
+       /* Ensure the number of entries claimed matches the no. returned */
+       if (*entriesp < 0)
+           *entriesp = 0;
+       if (*entriesp > arrayEntries.bulkentries_len)
+           *entriesp = arrayEntries.bulkentries_len;
+
        convertBulkToNBulk(&arrayEntries, blkentriesp);
 
        xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
@@ -496,6 +502,16 @@ VLDB_ListAttributes(VldbListByAttributes *attrp,
         newvlserver = vltype_old;      /* Doesn't support new interface */
         goto tryold;
     }
+
+    if (code)
+       return code;
+
+    /* Ensure the number of entries claimed matches the no. returned */
+    if (*entriesp < 0)
+       *entriesp = 0;
+    if (*entriesp > blkentriesp->nbulkentries_len)
+       *entriesp = blkentriesp->nbulkentries_len;
+
     return code;
 }
 
@@ -517,6 +533,12 @@ VLDB_ListAttributesU(VldbListByAttributes *attrp,
        if (code)
            return code;
 
+       /* Ensure the number of entries claimed matches the no. returned */
+       if (*entriesp < 0)
+           *entriesp = 0;
+       if (*entriesp > arrayEntries.bulkentries_len)
+           *entriesp = arrayEntries.bulkentries_len;
+
        convertBulkToUBulk(&arrayEntries, blkentriesp);
 
        xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
@@ -530,10 +552,15 @@ VLDB_ListAttributesU(VldbListByAttributes *attrp,
         newvlserver = vltype_old;      /* Doesn't support new interface */
         goto tryold;
     }
-
     if (code)
        return code;
 
+    /* Ensure the number of entries claimed matches the no. returned */
+    if (*entriesp < 0)
+       *entriesp = 0;
+    if (*entriesp > narrayEntries.nbulkentries_len)
+       *entriesp = narrayEntries.nbulkentries_len;
+
     convertNBulkToUBulk(&narrayEntries, blkentriesp);
 
     xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);
@@ -554,6 +581,14 @@ VLDB_ListAttributesN2(VldbListByAttributes *attrp,
         code =
             ubik_VL_ListAttributesN2(cstruct, 0, attrp, (name ? name : ""),
                                      thisindex, nentriesp, blkentriesp, nextindexp);
+       if (code)
+           return code;
+
+       /* Ensure the number of entries claimed matches the no. returned */
+       if (*nentriesp < 0)
+           *nentriesp = 0;
+       if (*nentriesp > blkentriesp->nbulkentries_len)
+           *nentriesp = blkentriesp->nbulkentries_len;
     }
     return code;
 }
@@ -578,6 +613,12 @@ VLDB_ListAttributesN2U(VldbListByAttributes *attrp,
         if (code)
            return code;
 
+       /* Ensure the number of entries claimed matches the no. returned */
+       if (*nentriesp < 0)
+           *nentriesp = 0;
+       if (*nentriesp > narrayEntries.nbulkentries_len)
+           *nentriesp = narrayEntries.nbulkentries_len;
+
        convertNBulkToUBulk(&narrayEntries, blkentriesp);
 
        xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);