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

[Kristof Provost]

On 18 Jan 2018, at 0:37, Gleb Smirnoff wrote:
> On Sun, Jan 07, 2018 at 04:44:23PM +0200, Konstantin Belousov wrote:
> K> On Sun, Jan 07, 2018 at 01:35:15PM +0000, Kristof Provost wrote:
> K> > Author: kp
> K> > Date: Sun Jan  7 13:35:15 2018
> K> > New Revision: 327675
> K> > URL: https://svnweb.freebsd.org/changeset/base/327675
> K> >
> K> > Log:
> K> >   pf: Avoid integer overflow issues by using mallocarray() iso. 
> malloc()
> K> >
> K> >   pfioctl() handles several ioctl that takes variable length 
> input, these
> K> >   include:
> K> >   - DIOCRADDTABLES
> K> >   - DIOCRDELTABLES
> K> >   - DIOCRGETTABLES
> K> >   - DIOCRGETTSTATS
> K> >   - DIOCRCLRTSTATS
> K> >   - DIOCRSETTFLAGS
> K> >
> K> >   All of them take a pfioc_table struct as input from userland. 
> One of
> K> >   its elements (pfrio_size) is used in a buffer length 
> calculation.
> K> >   The calculation contains an integer overflow which if triggered 
> can lead
> K> >   to out of bound reads and writes later on.
> K> So the size of the allocation is controlled directly from the 
> userspace ?
> K> This is an easy DoS, and by itself is perhaps bigger issue than the 
> overflow.
>
> Yes, this is one of the dirties parts of pf. The whole API to read and 
> configure
> tables from the userland calls to be rewritten from scratch. 
> Conversion from
> malloc to mallocarray really does nothing. Better just put a maximum 
> value
> cap.
>
I’m working on introducing limits there. Many GET/DELETE calls we can 
just limit to min(user_specified_size, kernel_size) for example.
Some of the others, like ADDTABLES are only used by pfctl, which only 
ever adds one table at a time. An arbitrary limit of 65k should be fine 
there.

ADDADDRS is one of the hard ones, because there’s no obvious limit to 
how many addresses can be added to a table. We may end up with an 
annoying arbitrary limit there.

I’m not done auditing all of them, but hopefully I’ll have something 
soon-ish.

And yes, I’m aware that the NULL checks no longer make sense. I’ll 
remove them as part of this work. They’re useless but not harmful at 
the moment.

Regards,
Kristof