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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B263CD6E-FEC6-4A82-A7A0-58FCB9046109>