From owner-svn-src-all@freebsd.org Thu Jan 18 07:20:12 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6586CEB95B7; Thu, 18 Jan 2018 07:20:12 +0000 (UTC) (envelope-from srs0=sjjv=en=freebsd.org=kp@codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 310F67D051; Thu, 18 Jan 2018 07:20:12 +0000 (UTC) (envelope-from srs0=sjjv=en=freebsd.org=kp@codepro.be) Received: from [172.16.2.10] (vega.codepro.be [IPv6:2a01:4f8:162:1127::3]) (Authenticated sender: kp) by venus.codepro.be (Postfix) with ESMTPSA id 98CA44664E; Thu, 18 Jan 2018 08:20:08 +0100 (CET) From: "Kristof Provost" To: "Gleb Smirnoff" Cc: "Konstantin Belousov" , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r327675 - head/sys/netpfil/pf Date: Thu, 18 Jan 2018 08:20:04 +0100 X-Mailer: MailMate (2.0BETAr6103) Message-ID: In-Reply-To: <20180117233702.GR8113@FreeBSD.org> References: <201801071335.w07DZFWh069854@repo.freebsd.org> <20180107144423.GD1684@kib.kiev.ua> <20180117233702.GR8113@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed; markup=markdown Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Jan 2018 07:20:12 -0000 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