From 2ef863720da4d9f368aaca0461c672a3008195ca Mon Sep 17 00:00:00 2001 From: Mark Vitale Date: Fri, 7 Aug 2015 11:56:16 -0400 Subject: [PATCH] afs: pioctl kernel memory overrun CVE-2015-8312: Any pioctl with an input buffer size (ViceIoctl->in_size) exactly equal to AFS_LRALLOCSIZE (4096 bytes) will cause a one-byte overwrite of its kernel memory working buffer. This may crash the operating system or cause other undefined behavior. The attacking pioctl must be a valid AFS pioctl code. However, it need not specify valid arguments (in the ViceIoctl), since only rudimentary checking is done in afs_HandlePioctl. Most argument validation occurs later in the individual pioctl handlers. Nor does the issuer need to be authenticated or authorized in any way, since authorization checks also occur much later, in the individual pioctl handlers. An unauthorized user may therefore trigger the overrun by either crafting his own malicious pioctl, or by issuing a privileged command, e.g. 'fs newalias', with appropriately sized but otherwise arbitrary arguments. In the latter case, the attacker will see the expected error message: "fs: You do not have the required rights to do this operation" but in either case the damage has been done. Pioctls are not logged or audited in any way (except those that cause loggable or auditable events as side effects). root cause: afs_HandlePioctli() calls afs_pd_alloc() to allocate two two afs_pdata structs, one for input and one for output. The memory for these buffers is based on the requested size, plus at least one extra byte for the null terminator to be set later: requested size allocated ================= ================================= > AFS_LRALLOCSIZ osi_Alloc(size+1) <= AFS_LRALLOCSIZ afs_AllocLargeSize(AFS_LRALLOCSIZ) afs_HandlePioctl then adds a null terminator to each buffer, one byte past the requested size. This is safe in all cases except one: if the requested in_size was _exactly_ AFS_LRALLOCSIZ (4096 bytes), this null is one byte beyond the allocated storage, zeroing a byte of kernel memory. Commit 6260cbecd0795c4795341bdcf98671de6b9a43fb introduced the null terminators and they were correct at that time. But the commit message warns: "note that this works because PIGGYSIZE is always less than AFS_LRALLOCSIZ" Commit f8ed1111d76bbf36a466036ff74b44e1425be8bd introduced the bug by increasing the maximum size of the buffers but failing to account correctly for the null terminator in the case of input buffer size == AFS_LRALLOCSIZ. Commit 592a99d6e693bc640e2bdfc2e7e5243fcedc8f93 (master version of one of the fixes in the recent 1.6.13 security release) is the fix that drew my attention to this new bug. Ironically, 592a99 (combined with this commit), will make it possible to eliminate the "offending" null termination line altogether since it will now be performed automatically by afs_pd_alloc(). [kaduk@mit.edu: adjust commit message for CVE number assignment, reduce unneeded churn in the diff.] Change-Id: I0299274c6d879f95c9b40cc85859294c26c410d7 --- src/afs/afs_pioctl.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index e91316a..0c96172 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -53,8 +53,9 @@ struct afs_pdata { static_inline int afs_pd_alloc(struct afs_pdata *apd, size_t size) { - - if (size > AFS_LRALLOCSIZ) + /* Ensure that we give caller at least one trailing guard byte + * for the NUL terminator. */ + if (size >= AFS_LRALLOCSIZ) apd->ptr = osi_Alloc(size + 1); else apd->ptr = osi_AllocLargeSpace(AFS_LRALLOCSIZ); @@ -62,11 +63,13 @@ afs_pd_alloc(struct afs_pdata *apd, size_t size) if (apd->ptr == NULL) return ENOMEM; - if (size > AFS_LRALLOCSIZ) + /* Clear it all now, including the guard byte. */ + if (size >= AFS_LRALLOCSIZ) memset(apd->ptr, 0, size + 1); else memset(apd->ptr, 0, AFS_LRALLOCSIZ); + /* Don't tell the caller about the guard byte. */ apd->remaining = size; return 0; @@ -78,7 +81,7 @@ afs_pd_free(struct afs_pdata *apd) if (apd->ptr == NULL) return; - if (apd->remaining > AFS_LRALLOCSIZ) + if (apd->remaining >= AFS_LRALLOCSIZ) osi_Free(apd->ptr, apd->remaining + 1); else osi_FreeLargeSpace(apd->ptr); -- 1.9.4