Skip to contentSkip to conversation

KEYS: DNS: limit the length of option strings

PATCH Superseded Eric Biggers 2 participants 6 messages Feb 28, 2018 – Apr 16, 2018

Subsystems (3)
Networking Network drivers Keys/keyrings
lore
Share & tools
lore

Eric Biggers

author
#0 lore raw

PATCH v2 1/1 KEYS: DNS: limit the length of option strings

From: Eric Biggers <[email protected]>

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 <[email protected]>
Fixes 4a2d789267e0 ("DNS: If the DNS server returns an error, allow that to be cached [ver #2]")
Cc <[email protected]> # v2.6.36+
Signed-off-by Eric Biggers <[email protected]>
---
 net/dns_resolver/dns_key.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
net/dns_resolver/dns_key.c +5 -7
index e1d4d898a007..ed372d550137 100644
@@ -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);
 	}
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
maintainer
#1 lore raw
Quoted 4 lines · Eric Biggers

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

author
#2 lore raw
Quoted 11 lines · David Howells
Quoted 4 lines · Eric Biggers

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

author
#3 lore raw
Quoted 11 lines · Eric Biggers
Quoted 10 lines · David Howells
Quoted 4 lines · Eric Biggers

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.

Quoted 6 lines
Quoted 1 line

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.

author
#4 lore raw
Quoted 12 lines · Eric Biggers
Quoted 11 lines · Eric Biggers
Quoted 10 lines · David Howells
Quoted 4 lines · Eric Biggers

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.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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.

author
#5 lore raw
Quoted 15 lines · Eric Biggers
Quoted 14 lines · Eric Biggers
Quoted 13 lines · Eric Biggers
Quoted 11 lines · David Howells
Quoted 4 lines · Eric Biggers

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

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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