> On Jan 17, 2018, at 18:37, Gleb Smirnoff <gleb...@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. _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
