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: I1a536b3a53ec4b6721fbd39a915207da4358720c

diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c
index 4efcd51..948809d 100644 (file)
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -55,8 +55,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);
@@ -64,11 +65,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;
@@ -80,7 +83,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);