Bug 207463

Summary: [patch] stable/10/sys/netpfil/pf/pf_ioctl.c:pfioctl(DIOCRSETADDRS) buffer overflow
Product: Base System Reporter: Paul J Murphy <paul>
Component: kernAssignee: Kristof Provost <kp>
Status: Closed FIXED    
Severity: Affects Many People CC: kp, paul, stable
Priority: --- Keywords: patch
Version: 10.3-BETA2   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
stable/10/sys/netpfil/pf/pf_ioctl.c patch none

Description Paul J Murphy 2016-02-24 17:13:42 UTC 
Created attachment 167367 [details]
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).
Comment 1 Kristof Provost freebsd_committer freebsd_triage 2016-02-24 23:19:09 UTC 
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
Comment 2 Paul J Murphy 2016-02-25 01:08:14 UTC 
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.
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-02-25 07:34:17 UTC 
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@inetstat.net>
  PR:		207463
  MFC after:	5 days
  Differential Revision:	https://reviews.freebsd.org/D5426

Changes:
  head/sys/netpfil/pf/pf_ioctl.c
Comment 4 Kristof Provost freebsd_committer freebsd_triage 2016-02-25 07:36:36 UTC 
I'll talk to re@ about MFCing this after the BETA3 builds are done (so in a couple of days).
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-03-03 07:17:24 UTC 
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@inetstat.net>
  PR:         207463
  Approved by:	re (marius)

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