From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Subject: Re: [PATCH net] KEYS: DNS: fix parsing multiple options Date: Mon, 11 Jun 2018 11:40:23 +0200 Message-ID: <20180611094022.gvrefejktxzw44i7@netronome.com> References: <20180608162037.129802-1-ebiggers3@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, "David S . Miller" , keyrings@vger.kernel.org, David Howells , Wang Lei , Eric Biggers To: Eric Biggers Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:39794 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932375AbeFKJkZ (ORCPT ); Mon, 11 Jun 2018 05:40:25 -0400 Received: by mail-wm0-f67.google.com with SMTP id p11-v6so15120822wmc.4 for ; Mon, 11 Jun 2018 02:40:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180608162037.129802-1-ebiggers3@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Jun 08, 2018 at 09:20:37AM -0700, Eric Biggers wrote: > From: Eric Biggers > > My recent fix for dns_resolver_preparse() printing very long strings was > incomplete, as shown by syzbot which still managed to hit the > WARN_ONCE() in set_precision() by adding a crafted "dns_resolver" key: > > precision 50001 too large > WARNING: CPU: 7 PID: 864 at lib/vsprintf.c:2164 vsnprintf+0x48a/0x5a0 > > The bug this time isn't just a printing bug, but also a logical error > when multiple options ("#"-separated strings) are given in the key > payload. Specifically, when separating an option string into name and > value, if there is no value then the name is incorrectly considered to > end at the end of the key payload, rather than the end of the current > option. This bypasses validation of the option length, and also means > that specifying multiple options is broken -- which presumably has gone > unnoticed as there is currently only one valid option anyway. > > Fix it by correctly calculating the length of the option name. > > Reproducer: > > perl -e 'print "#A#", "\x00" x 50000' | keyctl padd dns_resolver desc @s > > Fixes: 4a2d789267e0 ("DNS: If the DNS server returns an error, allow that to be cached [ver #2]") > Signed-off-by: Eric Biggers > --- > net/dns_resolver/dns_key.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c > index 40c851693f77e..d448823d4d2ed 100644 > --- a/net/dns_resolver/dns_key.c > +++ b/net/dns_resolver/dns_key.c > @@ -97,7 +97,7 @@ dns_resolver_preparse(struct key_preparsed_payload *prep) > return -EINVAL; > } > > - eq = memchr(opt, '=', opt_len) ?: end; > + eq = memchr(opt, '=', opt_len) ?: next_opt; > opt_nlen = eq - opt; > eq++; It seems risky to advance eq++ in the case there the value is empty. Its not not pointing to the value but it may be accessed twice further on in this loop. > opt_vlen = next_opt - eq; /* will be -1 if no value */ > -- > 2.18.0.rc1.242.g61856ae69a-goog >