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).
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
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.
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
I'll talk to re@ about MFCing this after the BETA3 builds are done (so in a couple of days).
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