svn commit: r327675 - head/sys/netpfil/pf

[Kristof Provost]

On 7 Jan 2018, at 15:44, Konstantin Belousov wrote:
> On Sun, Jan 07, 2018 at 01:35:15PM +0000, Kristof Provost wrote:
>> Author: kp
>> Date: Sun Jan  7 13:35:15 2018
>> New Revision: 327675
>> URL: https://svnweb.freebsd.org/changeset/base/327675
>>
>> Log:
>>   pf: Avoid integer overflow issues by using mallocarray() iso. 
>> malloc()
>>
>>   pfioctl() handles several ioctl that takes variable length input, 
>> these
>>   include:
>>   - DIOCRADDTABLES
>>   - DIOCRDELTABLES
>>   - DIOCRGETTABLES
>>   - DIOCRGETTSTATS
>>   - DIOCRCLRTSTATS
>>   - DIOCRSETTFLAGS
>>
>>   All of them take a pfioc_table struct as input from userland. One 
>> of
>>   its elements (pfrio_size) is used in a buffer length calculation.
>>   The calculation contains an integer overflow which if triggered can 
>> lead
>>   to out of bound reads and writes later on.
> So the size of the allocation is controlled directly from the 
> userspace ?
> This is an easy DoS, and by itself is perhaps bigger issue than the 
> overflow.

Yes, although only as root.

I’m not sure what the best way of handling that would be. It’s not 
easy to determine limits for these sizes. Any arbitrary value might 
break someone’s use case.
OpenBSD tend to copy in individual entries one at a time. This avoids 
having to allocate memory for all of them in one go, but I don’t like 
mixing user pointers and kernel pointers. It’s far too easy to lose 
track of what’s been copied in and what’s still in user space.

Regards,
Kristof