Date: Wed, 17 Jan 2018 20:42:52 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, Kristof Provost <kp@FreeBSD.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r327675 - head/sys/netpfil/pf Message-ID: <6872E06D-738E-42C4-8D1A-C4381C21A269@FreeBSD.org> In-Reply-To: <20180117233702.GR8113@FreeBSD.org> References: <201801071335.w07DZFWh069854@repo.freebsd.org> <20180107144423.GD1684@kib.kiev.ua> <20180117233702.GR8113@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jan 17, 2018, at 18:37, Gleb Smirnoff <glebius@FreeBSD.org> wrote: >=20 > 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> >=20 > K> > Log: > K> > pf: Avoid integer overflow issues by using mallocarray() iso. = malloc() > K> > =20 > 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> > =20 > 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. >=20 > 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. >=20 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6872E06D-738E-42C4-8D1A-C4381C21A269>