netdev - Re: [PATCH net] KEYS: DNS: fix parsing multiple options

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Jun 2018 17:18:18 +0100
From:   David Howells <dhowells@...hat.com>
To:     Simon Horman <simon.horman@...ronome.com>
Cc:     dhowells@...hat.com, Eric Biggers <ebiggers3@...il.com>,
        netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        keyrings@...r.kernel.org, Wang Lei <wang840925@...il.com>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH net] KEYS: DNS: fix parsing multiple options

Simon Horman <simon.horman@...ronome.com> wrote:

> > -			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 */

Yes, but note the next line ^^^ and the comment thereon.

This is followed later by a check:

				if (opt_vlen <= 0)
					goto bad_option_value;

in the dnserror option handler.

Note, also, there is guaranteed to be a NUL char included at the end of the
payload data, and that this is checked:

	if (datalen <= 1 || !data || data[datalen - 1] != '\0') {

David