| Message ID | 20180608162037.129802-1-ebiggers3@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] KEYS: DNS: fix parsing multiple options | expand |
On Fri, Jun 08, 2018 at 09:20:37AM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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 <ebiggers@google.com> > --- > 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 >
Hi Simon, On Mon, Jun 11, 2018 at 11:40:23AM +0200, Simon Horman wrote: > On Fri, Jun 08, 2018 at 09:20:37AM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > 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 <ebiggers@google.com> > > --- > > 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. > Sure, that's a separate existing issue though, and it must be checked that the value is present before using it anyway, which the code already does, so it's not a "real" bug. I think I'll keep this patch simple and leave that part as-is for now. Eric
On Mon, Jun 11, 2018 at 10:57:42AM -0700, Eric Biggers wrote: > Hi Simon, > > On Mon, Jun 11, 2018 at 11:40:23AM +0200, Simon Horman wrote: > > On Fri, Jun 08, 2018 at 09:20:37AM -0700, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > 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 <ebiggers@google.com> > > > --- > > > 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. > > > > Sure, that's a separate existing issue though, and it must be checked that the > value is present before using it anyway, which the code already does, so it's > not a "real" bug. I think I'll keep this patch simple and leave that part as-is > for now. Thanks Eric, I was reflecting on that too. I agree that your patch resolves a problem without introducing a new one. Reviewed-by: Simon Horman <simon.horman@netronome.com>
The fix seems to work, but the use of kstrtoul():
ret = kstrtoul(eq, 10, &derrno);
is incorrect since the buffer can't been modified to block out the next
argument if there is one, so the following fails:
perl -e 'print "#dnserror=1#", "\x00" x 1' |
keyctl padd dns_resolver desc @s
(Note this is preexisting and nothing to do with your patch).
I'm not sure how best to handle this.
Anyway, Dave, can you take Eric's patch into the net tree with:
Acked-by: David Howells <dhowells@redhat.com>
David
Simon Horman <simon.horman@netronome.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
On Thu, Jun 14, 2018 at 05:14:30PM +0100, David Howells wrote: > The fix seems to work, but the use of kstrtoul(): > > ret = kstrtoul(eq, 10, &derrno); > > is incorrect since the buffer can't been modified to block out the next > argument if there is one, so the following fails: > > perl -e 'print "#dnserror=1#", "\x00" x 1' | > keyctl padd dns_resolver desc @s > > (Note this is preexisting and nothing to do with your patch). > > I'm not sure how best to handle this. > > Anyway, Dave, can you take Eric's patch into the net tree with: > > Acked-by: David Howells <dhowells@redhat.com> > > David It could be handled by copying the option value to a temporary buffer. Anyway, that can be a separate fix... David (Miller), are you planning to take this through -net? Thanks! - Eric
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++; opt_vlen = next_opt - eq; /* will be -1 if no value */