freebsd stable - Mar 2016 - [Bug 207463] [patch] stable/10/sys/netpfil/pf/pf_ioctl.c:pfioctl(DIOCRSETADDRS) buffer overflow

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

            Bug ID: 207463
           Summary: [patch]
                    stable/10/sys/netpfil/pf/pf_ioctl.c:pfioctl(DIOCRSETAD
                    DRS) buffer overflow
           Product: Base System
           Version: 10.3-BETA2
          Hardware: Any
                OS: Any
            Status: New
          Keywords: patch
          Severity: Affects Many People
          Priority: ---
         Component: kern
          Assignee: freebsd-bugs at FreeBSD.org
          Reporter: paul at inetstat.net
                CC: freebsd-stable at FreeBSD.org
          Keywords: patch

Created attachment 167367
  -->
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=167367&action=edit
stable/10/sys/netpfil/pf/pf_ioctl.c patch

While investigating bug #192677 (pfctl iotcl buffer to small for bigger spamd
blacklists) on releng/10.2, I believe I have spotted a kernel buffer overflow
in stable/10/sys/netpfil/pf/pf_ioctl.c / stable/10/sys/netpfil/pf/pf_table.c,
introduced by base r286862 / base r286961.

stable/10/sys/netpfil/pf/pf_ioctl.c:pfioctl(DIOCRSETADDRS):

        totlen = io->pfrio_size * sizeof(struct pfr_addr);
        pfras = malloc(totlen, M_TEMP, M_WAITOK);

stable/10/sys/netpfil/pf/pf_table.c:pfr_set_addrs():

        bcopy(&ad, addr + size + i, sizeof(ad));

Inside pfr_set_addrs(), pfioctl()'s "pfras" becomes
"addr", "io->pfrio_size"
becomes "size", and "io->pfrio_size2" becomes
"size2".  pfr_set_addrs() uses
size2 to protect the buffer just above that bcopy.  Looking carefully at
stable/10/sys/sbin/pfctl/pfctl_table.c:pfctl_table("replace") and
stable/10/sys/sbin/pfctl/pfctl_radix.c:pfr_buf_grow(), io->pfrio_buffer
passed
into the ioctl is size2.

This is theoretical, based on simulating the code mentally.  I'm fairly
certain
that my analysis is correct, but I've not verified it via compiled stable/10
code.  The bcopy seems to fairly obviously run off the end of the buffer when
it is only "size".  The fix should be quite simple, by just changing
the buffer
to be "size2" in
stable/10/sys/netpfil/pf/pf_ioctl.c:pfioctl(DIOCRSETADDRS):

        totlen = io->pfrio_size2 * sizeof(struct pfr_addr);

Untested patch attached.  I believe this applies to both stable/10 and head.  I
have tagged it as 10.3-BETA, as that seems to be the places where the more
urgent attention is needed, as it would be quite unfortunate for 10.3 to be
released with this bug (if my analysis is correct).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

Kristof Provost <kp at freebsd.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kp at freebsd.org
           Assignee|freebsd-bugs at FreeBSD.org    |kp at freebsd.org

--- Comment #1 from Kristof Provost <kp at freebsd.org> ---
I think your analysis is correct.

The intention of the bcopy() appears to be to copy additional addresses behind
the original list (hence the adds + size + i construction).

You're correct that the buffer allocated by 'totlen = io->pfrio_size
*
sizeof(struct pfr_addr);' is too small for that.
It's possible to panic a box that way.

I don't think your fix is sufficient though. If user space provides a
smaller
pfrio_size2 than pfrio_size (remember that all user space programmers are out
to get us!) then we'd still end up running outsize the allocated buffer.

I think we need to allocate the largest of pfrio_size and pfrio_size2:
https://reviews.freebsd.org/D5426

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

--- Comment #2 from Paul J Murphy <paul at inetstat.net> ---
Yes, you are correct.  My patch was sufficient only for the default usage by
/sbin/pfctl, but left scope for other usage to cause problems.  I've looked
over your patch, and it looks good to me.  The existing buffer protection code
in pfr_set_addrs() also looks like it will handle a smaller size2 cleanly.  I
have just updated my releng/10.2 system to stable/10's sys/netpfil/pf and
sbin/pfctl, with your patch applied to it, and it seems to both pass a quick
and basic functionality test, and fix bug #192677 (it is now successfully
replacing a pf table with over 130,000 addrs, where 10.2-p12 fails for anything
over around 65,000).

Thanks.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

--- Comment #3 from commit-hook at freebsd.org ---
A commit references this bug:

Author: kp
Date: Thu Feb 25 07:33:59 UTC 2016
New revision: 296025
URL: https://svnweb.freebsd.org/changeset/base/296025

Log:
  pf: Fix possible out-of-bounds write

  In the DIOCRSETADDRS ioctl() handler we allocate a table for struct
pfr_addrs,
  which is processed in pfr_set_addrs(). At the users request we also provide
  feedback on the deleted addresses, by storing them after the new list
  ('bcopy(&ad, addr + size + i, sizeof(ad));' in pfr_set_addrs()).

  This means we write outside the bounds of the buffer we've just allocated.
  We need to look at pfrio_size2 instead (i.e. the size the user reserved for
our
  feedback). That'd allow a malicious user to specify a smaller pfrio_size2
than
  pfrio_size though, in which case we'd still read outside of the allocated
  buffer. Instead we allocate the largest of the two values.

  Reported By:  Paul J Murphy <paul at inetstat.net>
  PR:           207463
  MFC after:    5 days
  Differential Revision:        https://reviews.freebsd.org/D5426

Changes:
  head/sys/netpfil/pf/pf_ioctl.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

--- Comment #4 from Kristof Provost <kp at freebsd.org> ---
I'll talk to re@ about MFCing this after the BETA3 builds are done (so in a
couple of days).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

--- Comment #5 from commit-hook at freebsd.org ---
A commit references this bug:

Author: kp
Date: Thu Mar  3 07:16:36 UTC 2016
New revision: 296340
URL: https://svnweb.freebsd.org/changeset/base/296340

Log:
  MFC: r296025:

  pf: Fix possible out-of-bounds write

  In the DIOCRSETADDRS ioctl() handler we allocate a table for struct
pfr_addrs,
  which is processed in pfr_set_addrs(). At the users request we also provide
  feedback on the deleted addresses, by storing them after the new list
  ('bcopy(&ad, addr + size + i, sizeof(ad));' in pfr_set_addrs()).

  This means we write outside the bounds of the buffer we've just allocated.
  We need to look at pfrio_size2 instead (i.e. the size the user reserved for
our
  feedback). That'd allow a malicious user to specify a smaller pfrio_size2
than
  pfrio_size though, in which case we'd still read outside of the allocated
  buffer. Instead we allocate the largest of the two values.

  Reported By:        Paul J Murphy <paul at inetstat.net>
  PR:         207463
  Approved by:  re (marius)

Changes:
_U  stable/10/
  stable/10/sys/netpfil/pf/pf_ioctl.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207463

Kristof Provost <kp at freebsd.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|New                         |Closed

-- 
You are receiving this mail because:
You are on the CC list for the bug.