diff mbox series

[v2] KEYS: DNS: limit the length of option strings

Message ID 20180228190510.215801-1-ebiggers3@gmail.com
State Not Applicable, archived
Delegated to: David Miller
Headers show
Series [v2] KEYS: DNS: limit the length of option strings | expand

Commit Message

Eric Biggers Feb. 28, 2018, 7:05 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Adding a dns_resolver key whose payload contains a very long option name
resulted in that string being printed in full.  This hit the WARN_ONCE()
in set_precision() during the printk(), because printk() only supports a
precision of up to 32767 bytes:

    precision 1000000 too large
    WARNING: CPU: 0 PID: 752 at lib/vsprintf.c:2189 vsnprintf+0x4bc/0x5b0

Fix it by limiting option strings (combined name + value) to a much more
reasonable 128 bytes.  The exact limit is arbitrary, but currently the
only recognized option is formatted as "dnserror=%lu" which fits well
within this limit.

Also ratelimit the printks.

Reproducer:

    perl -e 'print "#", "A" x 1000000, "\x00"' | keyctl padd dns_resolver desc @s

This bug was found using syzkaller.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Fixes: 4a2d789267e0 ("DNS: If the DNS server returns an error, allow that to be cached [ver #2]")
Cc: <stable@vger.kernel.org> # v2.6.36+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 net/dns_resolver/dns_key.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

David Howells March 7, 2018, 3:54 p.m. UTC | #1 
Eric Biggers <ebiggers3@gmail.com> wrote:

> Fix it by limiting option strings (combined name + value) to a much more
> reasonable 128 bytes.  The exact limit is arbitrary, but currently the
> only recognized option is formatted as "dnserror=%lu" which fits well
> within this limit.

There will be more options coming ("ipv4", "ipv6") but they shouldn't overrun
this limit and we can always extend the limit if need be.

David
Eric Biggers March 12, 2018, 5:57 p.m. UTC | #2 
On Wed, Mar 07, 2018 at 03:54:37PM +0000, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > Fix it by limiting option strings (combined name + value) to a much more
> > reasonable 128 bytes.  The exact limit is arbitrary, but currently the
> > only recognized option is formatted as "dnserror=%lu" which fits well
> > within this limit.
> 
> There will be more options coming ("ipv4", "ipv6") but they shouldn't overrun
> this limit and we can always extend the limit if need be.
> 
> David

David (Howells) do you want to take this patch through the keyrings tree or
should I ask David Miller to take it through net-next?

Eric
Eric Biggers March 23, 2018, 8:21 p.m. UTC | #3 
On Mon, Mar 12, 2018 at 10:57:07AM -0700, Eric Biggers wrote:
> On Wed, Mar 07, 2018 at 03:54:37PM +0000, David Howells wrote:
> > Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > > Fix it by limiting option strings (combined name + value) to a much more
> > > reasonable 128 bytes.  The exact limit is arbitrary, but currently the
> > > only recognized option is formatted as "dnserror=%lu" which fits well
> > > within this limit.
> > 
> > There will be more options coming ("ipv4", "ipv6") but they shouldn't overrun
> > this limit and we can always extend the limit if need be.
> > 
> > David
> 
> David (Howells) do you want to take this patch through the keyrings tree or
> should I ask David Miller to take it through net-next?
> 
> Eric

Ping.
Eric Biggers April 2, 2018, 7:20 p.m. UTC | #4 
On Fri, Mar 23, 2018 at 01:21:22PM -0700, Eric Biggers wrote:
> On Mon, Mar 12, 2018 at 10:57:07AM -0700, Eric Biggers wrote:
> > On Wed, Mar 07, 2018 at 03:54:37PM +0000, David Howells wrote:
> > > Eric Biggers <ebiggers3@gmail.com> wrote:
> > > 
> > > > Fix it by limiting option strings (combined name + value) to a much more
> > > > reasonable 128 bytes.  The exact limit is arbitrary, but currently the
> > > > only recognized option is formatted as "dnserror=%lu" which fits well
> > > > within this limit.
> > > 
> > > There will be more options coming ("ipv4", "ipv6") but they shouldn't overrun
> > > this limit and we can always extend the limit if need be.
> > > 
> > > David
> > 
> > David (Howells) do you want to take this patch through the keyrings tree or
> > should I ask David Miller to take it through net-next?
> > 
> > Eric
> 
> Ping.

Ping again.
Eric Biggers April 16, 2018, 9:31 p.m. UTC | #5 
On Mon, Apr 02, 2018 at 12:20:35PM -0700, Eric Biggers wrote:
> On Fri, Mar 23, 2018 at 01:21:22PM -0700, Eric Biggers wrote:
> > On Mon, Mar 12, 2018 at 10:57:07AM -0700, Eric Biggers wrote:
> > > On Wed, Mar 07, 2018 at 03:54:37PM +0000, David Howells wrote:
> > > > Eric Biggers <ebiggers3@gmail.com> wrote:
> > > > 
> > > > > Fix it by limiting option strings (combined name + value) to a much more
> > > > > reasonable 128 bytes.  The exact limit is arbitrary, but currently the
> > > > > only recognized option is formatted as "dnserror=%lu" which fits well
> > > > > within this limit.
> > > > 
> > > > There will be more options coming ("ipv4", "ipv6") but they shouldn't overrun
> > > > this limit and we can always extend the limit if need be.
> > > > 
> > > > David
> > > 
> > > David (Howells) do you want to take this patch through the keyrings tree or
> > > should I ask David Miller to take it through net-next?
> > > 
> > > Eric
> > 
> > Ping.
> 
> Ping again.

Times up.  I'll resend for net-next.

Eric
diff mbox series

Patch

diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index e1d4d898a007..ed372d550137 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -91,9 +91,9 @@  dns_resolver_preparse(struct key_preparsed_payload *prep)
 
 			next_opt = memchr(opt, '#', end - opt) ?: end;
 			opt_len = next_opt - opt;
-			if (!opt_len) {
-				printk(KERN_WARNING
-				       "Empty option to dns_resolver key\n");
+			if (opt_len <= 0 || opt_len > 128) {
+				pr_warn_ratelimited("Invalid option length (%d) for dns_resolver key\n",
+						    opt_len);
 				return -EINVAL;
 			}
 
@@ -127,10 +127,8 @@  dns_resolver_preparse(struct key_preparsed_payload *prep)
 			}
 
 		bad_option_value:
-			printk(KERN_WARNING
-			       "Option '%*.*s' to dns_resolver key:"
-			       " bad/missing value\n",
-			       opt_nlen, opt_nlen, opt);
+			pr_warn_ratelimited("Option '%*.*s' to dns_resolver key: bad/missing value\n",
+					    opt_nlen, opt_nlen, opt);
 			return -EINVAL;
 		} while (opt = next_opt + 1, opt < end);
 	}