From 0bf9fba458b39035a09f45c1b63f1e65672d4c00 Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Fri, 30 Jan 2015 12:12:03 -0500 Subject: [PATCH] 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. Change-Id: Ic7d7f615a37491835af8d92f3c5f1b6a667d9d01 Reviewed-on: http://gerrit.openafs.org/11702 Tested-by: BuildBot Reviewed-by: Mark Vitale Reviewed-by: Benjamin Kaduk --- src/libacl/netprocs.c | 68 ++++++++++++++++++++++++++++++++++++++++++++------ src/volser/dumpstuff.c | 12 +++++++-- src/volser/vol-dump.c | 6 ++++- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/src/libacl/netprocs.c b/src/libacl/netprocs.c index f41ad55..7aaae0c 100644 --- a/src/libacl/netprocs.c +++ b/src/libacl/netprocs.c @@ -24,14 +24,53 @@ #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); diff --git a/src/volser/dumpstuff.c b/src/volser/dumpstuff.c index 8c01aa0..e1754d1 100644 --- a/src/volser/dumpstuff.c +++ b/src/volser/dumpstuff.c @@ -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':{ diff --git a/src/volser/vol-dump.c b/src/volser/vol-dump.c index 554ecf0..1efbbaf 100644 --- a/src/volser/vol-dump.c +++ b/src/volser/vol-dump.c @@ -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), -- 1.9.4