From d1946ffe9be0031a2daf907f5e96cf0ee7f5e15e Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Sat, 16 Jul 2011 22:57:55 +0100 Subject: [PATCH] libafs/dir: Verify directory pathnames Provide a new routine, afs_dir_GetVerifiedBlob() which will ensure that the pathname contained within a directory blob is correctly terminated before returning it to the caller. For the purposes of this function, correct termination is defined as having a terminating \0 character within the same directory page as the blob itself. Change-Id: I4b3bbb95cb49645a8ac52e6061f9e24f89924831 Reviewed-on: http://gerrit.openafs.org/5241 Tested-by: BuildBot Reviewed-by: Derrick Brashear --- src/afs/LINUX/osi_vnodeops.c | 24 +++---- src/afs/VNOPS/afs_vnop_readdir.c | 8 ++- src/dir/dir.c | 120 +++++++++++++++++++++++-------- src/dir/dir.h | 2 + 4 files changed, 106 insertions(+), 48 deletions(-) diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index b61353ed7..ca1998a28 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -340,25 +340,19 @@ afs_linux_readdir(struct file *fp, void *dirbuf, filldir_t filldir) if (!dirpos) break; - code = afs_dir_GetBlob(tdc, dirpos, &entry); - if (code) - break; - de = (struct DirEntry *)entry.data; - - ino = afs_calc_inum(avc->f.fid.Cell, avc->f.fid.Fid.Volume, - ntohl(de->fid.vnode)); - - if (de->name) - len = strlen(de->name); - else { - printf("afs_linux_readdir: afs_dir_GetBlob failed, null name (inode %lx, dirpos %d)\n", - (unsigned long)&tdc->f.inode, dirpos); - DRelease(&entry, 0); + code = afs_dir_GetVerifiedBlob(tdc, dirpos, &de); + if (code) { + afs_warn("Corrupt directory (inode %lx, dirpos %d)", + (unsigned long)&tdc->f.inode, dirpos); ReleaseSharedLock(&avc->lock); afs_PutDCache(tdc); code = -ENOENT; goto out; - } + } + + ino = afs_calc_inum (avc->f.fid.Cell, avc->f.fid.Fid.Volume, + ntohl(de->fid.vnode)); + len = strlen(de->name); /* filldir returns -EINVAL when the buffer is full. */ { diff --git a/src/afs/VNOPS/afs_vnop_readdir.c b/src/afs/VNOPS/afs_vnop_readdir.c index 5aca35255..f2f0a84ef 100644 --- a/src/afs/VNOPS/afs_vnop_readdir.c +++ b/src/afs/VNOPS/afs_vnop_readdir.c @@ -738,8 +738,12 @@ afs_readdir(OSI_VC_DECL(avc), struct uio *auio, afs_ucred_t *acred) origOffset = AFS_UIO_OFFSET(auio); /* scan for the next interesting entry scan for in-use blob otherwise up point at * this blob note that ode, if non-zero, also represents a held dir page */ - if (!(us = BlobScan(tdc, (origOffset >> 5))) - || (afs_dir_GetBlob(tdc, us, &nextEntry) != 0)) { + us = BlobScan(tdc, (origOffset >> 5)); + + if (us) + afs_dir_GetVerifiedBlob(tdc, us, &nextEntry); + + if (us == 0 || nde == NULL) { /* failed to setup nde, return what we've got, and release ode */ if (len) { /* something to hand over. */ diff --git a/src/dir/dir.c b/src/dir/dir.c index 28647d0e7..191d5bcea 100644 --- a/src/dir/dir.c +++ b/src/dir/dir.c @@ -425,25 +425,24 @@ afs_dir_EnumerateDir(dir_file_t dir, int (*proc) (void *, char *name, num = ntohs(dhp->hashTable[i]); while (num != 0) { /* Walk down the hash table list. */ - DErrno = 0; - if (afs_dir_GetBlob(dir, num, &entrybuf) != 0) { - if (DErrno) { - /* we failed, return why */ - DRelease(&headerbuf, 0); - return DErrno; - } - break; - } + code = afs_dir_GetVerifiedBlob(dir, num, &entrybuf); + if (code) + goto out; + ep = (struct DirEntry *)entrybuf.data; + if (!ep) + break; num = ntohs(ep->next); code = (*proc) (hook, ep->name, ntohl(ep->fid.vnode), ntohl(ep->fid.vunique)); DRelease(&entrybuf, 0); if (code) - break; + goto out; } } + +out: DRelease(&headerbuf, 0); return 0; } @@ -467,9 +466,9 @@ afs_dir_IsEmpty(dir_file_t dir) num = ntohs(dhp->hashTable[i]); while (num != 0) { /* Walk down the hash table list. */ - if (afs_dir_GetBlob(dir, num, &entrybuf) != 0); + if (afs_dir_GetVerifiedBlob(dir, num, &entrybuf) != 0); break; - ep = (struct DirEntry *)entrybuf.data; + if (strcmp(ep->name, "..") && strcmp(ep->name, ".")) { DRelease(&entrybuf, 0); DRelease(&headerbuf, 0); @@ -483,22 +482,78 @@ afs_dir_IsEmpty(dir_file_t dir) return 0; } -int -afs_dir_GetBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *buffer) +/* Return a pointer to an entry, given its number. Also return the maximum + * size of the entry, which is determined by its position within the directory + * page. + */ + +static int +GetBlobWithLimit(dir_file_t dir, afs_int32 blobno, + struct DirBuffer *buffer, afs_size_t *maxlen) { + afs_size_t pos; int code; + *maxlen = 0; memset(buffer, 0, sizeof(struct DirBuffer)); code = DRead(dir, blobno >> LEPP, buffer); if (code) return code; - buffer->data = (void *)(((long)buffer->data) + 32 * (blobno & (EPP - 1))); + pos = 32 * (blobno & (EPP - 1)); + + *maxlen = AFS_PAGESIZE - pos - 1; + + buffer->data = (void *)(((char *)buffer->data) + pos); return 0; } +/* Given an entries number, return a pointer to that entry */ +int +afs_dir_GetBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *buffer) +{ + afs_size_t maxlen = 0; + return GetBlobWithLimit(dir, blobno, buffer, &maxlen); +} + +/* Return an entry, having verified that the name held within the entry + * doesn't overflow off the end of the directory page it is contained + * within + */ + +int +afs_dir_GetVerifiedBlob(dir_file_t file, afs_int32 blobno, + struct DirBuffer *outbuf) +{ + struct DirEntry *dir; + struct DirBuffer buffer; + afs_size_t maxlen; + int code; + char *cp; + + outbuf = NULL; + + code = GetBlobWithLimit(file, blobno, &buffer, &maxlen); + if (code) + return code; + + dir = (struct DirEntry *)buffer.data; + + /* A blob is only valid if the name within it is NULL terminated before + * the end of the blob's containing page */ + for (cp = dir->name; *cp != '\0' && cp < ((char *)dir) + maxlen; cp++); + + if (*cp != '\0') { + DRelease(&buffer, 0); + return EIO; + } + + *outbuf = buffer; + return 0; +} + int afs_dir_DirHash(char *string) { @@ -550,8 +605,9 @@ FindItem(dir_file_t dir, char *ename, struct DirBuffer *prevbuf, return ENOENT; } - code = afs_dir_GetBlob(dir, (u_short) ntohs(dhp->hashTable[i]), - &curr); + code = afs_dir_GetVerifiedBlob(dir, + (u_short) ntohs(dhp->hashTable[i]), + &curr); if (code) { DRelease(&prev, 0); return code; @@ -569,22 +625,22 @@ FindItem(dir_file_t dir, char *ename, struct DirBuffer *prevbuf, return 0; } DRelease(&prev, 0); - if (tp->next == 0) { - /* The end of the line */ - DRelease(&curr, 0); - return ENOENT; - } prev = curr; prev.data = &(tp->next); - code = afs_dir_GetBlob(dir, (u_short) ntohs(tp->next), &curr); - if (code) { - DRelease(&prev, 0); - return code; - } + if (tp->next == 0) + goto out; /* The end of the line */ + + code = afs_dir_GetVerifiedBlob(dir, (u_short) ntohs(tp->next), + &curr); + if (code) + goto out; } - /* Never reached */ + +out: + DRelease(&prev, 0); + return ENOENT; } static int @@ -611,8 +667,9 @@ FindFid (void *dir, afs_uint32 vnode, afs_uint32 unique, for (i=0; ihashTable[i] != 0) { - code = afs_dir_GetBlob(dir, (u_short)ntohs(dhp->hashTable[i]), - &curr); + code = afs_dir_GetVerifiedBlob(dir, + (u_short)ntohs(dhp->hashTable[i]), + &curr); if (code) { DRelease(&header, 0); return code; @@ -634,7 +691,8 @@ FindFid (void *dir, afs_uint32 vnode, afs_uint32 unique, if (next == 0) break; - code = afs_dir_GetBlob(dir, (u_short)ntohs(next), &curr); + code = afs_dir_GetVerifiedBlob(dir, (u_short)ntohs(next), + &curr); if (code) { DRelease(&header, 0); return code; diff --git a/src/dir/dir.h b/src/dir/dir.h index 3d1055a0a..f5c8eef42 100644 --- a/src/dir/dir.h +++ b/src/dir/dir.h @@ -104,6 +104,8 @@ extern int afs_dir_EnumerateDir(dir_file_t dir, extern int afs_dir_IsEmpty(dir_file_t dir); extern int afs_dir_GetBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *); +extern int afs_dir_GetVerifiedBlob(dir_file_t dir, afs_int32 blobno, + struct DirBuffer *); extern int afs_dir_DirHash(char *string); extern int afs_dir_InverseLookup (void *dir, afs_uint32 vnode, -- 2.39.5