From: Michael Meffie Date: Fri, 30 Jan 2015 17:12:03 +0000 (-0500) Subject: volser: range check acl header fields during dumps and restores X-Git-Tag: upstream/1.6.18^2~21 X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=30da3d32533ce225d8d89099b0c3355f01a946a9;p=packages%2Fo%2Fopenafs.git 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 Reviewed-by: Mark Vitale Reviewed-by: Benjamin Kaduk (cherry picked from commit 0bf9fba458b39035a09f45c1b63f1e65672d4c00) Change-Id: Icebeb1d62900a7978f02177627a30e41de49a182 Reviewed-on: https://gerrit.openafs.org/12127 Tested-by: BuildBot Reviewed-by: Michael Meffie Tested-by: Michael Meffie Reviewed-by: Mark Vitale Reviewed-by: Stephan Wiesand --- diff --git a/src/libacl/netprocs.c b/src/libacl/netprocs.c index f9aec80af..19dc6a367 100644 --- a/src/libacl/netprocs.c +++ b/src/libacl/netprocs.c @@ -16,6 +16,7 @@ #include #include +#include #include #ifdef AFS_NT40_ENV @@ -28,14 +29,53 @@ #include #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 915b70a37..b37301d37 100644 --- 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 0820e2544..ac7a76383 100644 --- 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),