Date: Thu, 18 Jan 2018 08:20:04 +0100 From: "Kristof Provost" <kp@FreeBSD.org> To: "Gleb Smirnoff" <glebius@FreeBSD.org> Cc: "Konstantin Belousov" <kostikbel@gmail.com>, 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: <B263CD6E-FEC6-4A82-A7A0-58FCB9046109@FreeBSD.org> In-Reply-To: <20180117233702.GR8113@FreeBSD.org> References: <201801071335.w07DZFWh069854@repo.freebsd.org> <20180107144423.GD1684@kib.kiev.ua> <20180117233702.GR8113@FreeBSD.org>
index | next in thread | previous in thread | raw e-mail
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, Kristofhome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B263CD6E-FEC6-4A82-A7A0-58FCB9046109>