From 8992210f27671673a89a541776aa105238ad14cf Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Sun, 26 Dec 2010 14:54:43 +0000 Subject: [PATCH] Don't trust # of entries from ListAttributes 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 Tested-by: BuildBot --- src/bucoord/commands.c | 5 +++++ src/libadmin/vos/vosutils.c | 11 +++++++++++ src/volser/vsutils.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 58 insertions(+), 1 deletions(-) diff --git a/src/bucoord/commands.c b/src/bucoord/commands.c index 324510d..5e4ba53 100644 --- a/src/bucoord/commands.c +++ b/src/bucoord/commands.c @@ -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++) { diff --git a/src/libadmin/vos/vosutils.c b/src/libadmin/vos/vosutils.c index c2325bc..9f18ab6 100644 --- a/src/libadmin/vos/vosutils.c +++ b/src/libadmin/vos/vosutils.c @@ -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; } } diff --git a/src/volser/vsutils.c b/src/volser/vsutils.c index 447d39a..33ecefc 100644 --- a/src/volser/vsutils.c +++ b/src/volser/vsutils.c @@ -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); -- 1.7.1