[PATCH net] KEYS: DNS: fix parsing multiple options

[PATCH net] KEYS: DNS: fix parsing multiple options

[Eric Biggers]

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++;
 			opt_vlen = next_opt - eq; /* will be -1 if no value */
-- 
2.18.0.rc1.242.g61856ae69a-goog

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

[Simon Horman]

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
> 

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

[Eric Biggers]

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

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

[Simon Horman]

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>

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

[David Howells]

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

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

[David Howells]

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

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

[Eric Biggers]

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

[PATCH net] KEYS: DNS: fix parsing multiple options

[David Howells]

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>
Signed-off-by: David Howells <dhowells@redhat.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 40c851693f77..d448823d4d2e 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 */