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

[Pedro Giffuni]

> On Jan 17, 2018, at 18:37, Gleb Smirnoff <glebius at FreeBSD.org> 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.
> 

FWIW, the associated NULL checks just became no-ops since overflows in mallocarray(9) will now cause panics.

Either the flags should be changed to M_NOWAIT or the NULL checks should be removed. No idea which makes more sense.

Pedro.