All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-02-28 19:05 ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-02-28 19:05 UTC (permalink / raw)
  To: keyrings, David Howells; +Cc: netdev, Mark Rutland, Eric Biggers

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(-)

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);
 	}
-- 
2.16.2.395.g2e18187dfd-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-02-28 19:05 ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-02-28 19:05 UTC (permalink / raw)
  To: keyrings, David Howells; +Cc: netdev, Mark Rutland, Eric Biggers

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(-)

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);
 	}
-- 
2.16.2.395.g2e18187dfd-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
  2018-02-28 19:05 ` Eric Biggers
  (?)
@ 2018-03-07 15:54 ` David Howells
  2018-03-12 17:57     ` Eric Biggers
  -1 siblings, 1 reply; 11+ messages in thread
From: David Howells @ 2018-03-07 15:54 UTC (permalink / raw)
  To: Eric Biggers; +Cc: dhowells, keyrings, netdev, Mark Rutland, Eric Biggers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
  2018-03-07 15:54 ` David Howells
@ 2018-03-12 17:57     ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-03-12 17:57 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-03-12 17:57     ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-03-12 17:57 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
  2018-03-12 17:57     ` Eric Biggers
@ 2018-03-23 20:21       ` Eric Biggers
  -1 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-03-23 20:21 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-03-23 20:21       ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-03-23 20:21 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
  2018-03-23 20:21       ` Eric Biggers
@ 2018-04-02 19:20         ` Eric Biggers
  -1 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-04-02 19:20 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-04-02 19:20         ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-04-02 19:20 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
  2018-04-02 19:20         ` Eric Biggers
@ 2018-04-16 21:31           ` Eric Biggers
  -1 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-04-16 21:31 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] KEYS: DNS: limit the length of option strings
@ 2018-04-16 21:31           ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-04-16 21:31 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, netdev, Mark Rutland, Eric Biggers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-04-16 21:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 19:05 [PATCH v2] KEYS: DNS: limit the length of option strings Eric Biggers
2018-02-28 19:05 ` Eric Biggers
2018-03-07 15:54 ` David Howells
2018-03-12 17:57   ` Eric Biggers
2018-03-12 17:57     ` Eric Biggers
2018-03-23 20:21     ` Eric Biggers
2018-03-23 20:21       ` Eric Biggers
2018-04-02 19:20       ` Eric Biggers
2018-04-02 19:20         ` Eric Biggers
2018-04-16 21:31         ` Eric Biggers
2018-04-16 21:31           ` Eric Biggers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.