| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZZ4fyY4r3rqgZL+4@xpf.sh.intel.com>
Date: Wed, 10 Jan 2024 12:40:41 +0800
From: Pengfei Xu <pengfei.xu@...el.com>
To: David Howells <dhowells@...hat.com>, <eadavis@...com>
CC: Linus Torvalds <torvalds@...ux-foundation.org>, Edward Adam Davis
<eadavis@...com>, Simon Horman <horms@...nel.org>, Markus Suvanto
<markus.suvanto@...il.com>, Jeffrey E Altman <jaltman@...istor.com>, "Marc
Dionne" <marc.dionne@...istor.com>, Wang Lei <wang840925@...il.com>, "Jeff
Layton" <jlayton@...hat.com>, Steve French <smfrench@...il.com>, "Jarkko
Sakkinen" <jarkko@...nel.org>, "David S. Miller" <davem@...emloft.net>, "Eric
Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, <linux-afs@...ts.infradead.org>,
<keyrings@...r.kernel.org>, <linux-cifs@...r.kernel.org>,
<linux-nfs@...r.kernel.org>, <ceph-devel@...r.kernel.org>,
<netdev@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <heng.su@...el.com>, <pengfei.xu@...el.com>
Subject: Re: [PATCH] keys, dns: Fix missing size check of V1 server-list
header
On 2023-12-24 at 00:02:49 +0000, David Howells wrote:
> Hi Linus, Edward,
>
> Here's Linus's patch dressed up with a commit message. I would marginally
> prefer just to insert the missing size check, but I'm also fine with Linus's
> approach for now until we have different content types or newer versions.
>
> Note that I'm not sure whether I should require Linus's S-o-b since he made
> modifications or whether I should use a Codeveloped-by line for him.
>
> David
> ---
> From: Edward Adam Davis <eadavis@...com>
>
> keys, dns: Fix missing size check of V1 server-list header
>
> The dns_resolver_preparse() function has a check on the size of the payload
> for the basic header of the binary-style payload, but is missing a check
> for the size of the V1 server-list payload header after determining that's
> what we've been given.
>
> Fix this by getting rid of the the pointer to the basic header and just
> assuming that we have a V1 server-list payload and moving the V1 server
> list pointer inside the if-statement. Dealing with other types and
> versions can be left for when such have been defined.
>
> This can be tested by doing the following with KASAN enabled:
>
> echo -n -e '\x0\x0\x1\x2' | keyctl padd dns_resolver foo @p
>
> and produces an oops like the following:
>
> BUG: KASAN: slab-out-of-bounds in dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
> Read of size 1 at addr ffff888028894084 by task syz-executor265/5069
> ...
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
> print_address_description mm/kasan/report.c:377 [inline]
> print_report+0xc3/0x620 mm/kasan/report.c:488
> kasan_report+0xd9/0x110 mm/kasan/report.c:601
> dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
> __key_create_or_update+0x453/0xdf0 security/keys/key.c:842
> key_create_or_update+0x42/0x50 security/keys/key.c:1007
> __do_sys_add_key+0x29c/0x450 security/keys/keyctl.c:134
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x62/0x6a
>
> This patch was originally by Edward Adam Davis, but was modified by Linus.
>
> Fixes: b946001d3bb1 ("keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry")
> Reported-and-tested-by: syzbot+94bbb75204a05da3d89f@...kaller.appspotmail.com
> Link: https://lore.kernel.org/r/0000000000009b39bc060c73e209@google.com/
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> Signed-off-by: David Howells <dhowells@...hat.com>
> Tested-by: David Howells <dhowells@...hat.com>
> cc: Edward Adam Davis <eadavis@...com>
> cc: Simon Horman <horms@...nel.org>
> cc: Linus Torvalds <torvalds@...ux-foundation.org>
> cc: Jarkko Sakkinen <jarkko@...nel.org>
> cc: Jeffrey E Altman <jaltman@...istor.com>
> cc: Wang Lei <wang840925@...il.com>
> cc: Jeff Layton <jlayton@...hat.com>
> cc: Steve French <sfrench@...ibm.com>
> cc: Marc Dionne <marc.dionne@...istor.com>
> cc: "David S. Miller" <davem@...emloft.net>
> cc: Eric Dumazet <edumazet@...gle.com>
> cc: Jakub Kicinski <kuba@...nel.org>
> cc: Paolo Abeni <pabeni@...hat.com>
> cc: linux-afs@...ts.infradead.org
> cc: linux-cifs@...r.kernel.org
> cc: linux-nfs@...r.kernel.org
> cc: ceph-devel@...r.kernel.org
> cc: keyrings@...r.kernel.org
> cc: netdev@...r.kernel.org
> ---
> net/dns_resolver/dns_key.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
> index 2a6d363763a2..f18ca02aa95a 100644
> --- a/net/dns_resolver/dns_key.c
> +++ b/net/dns_resolver/dns_key.c
> @@ -91,8 +91,6 @@ const struct cred *dns_resolver_cache;
> static int
> dns_resolver_preparse(struct key_preparsed_payload *prep)
> {
> - const struct dns_server_list_v1_header *v1;
> - const struct dns_payload_header *bin;
> struct user_key_payload *upayload;
> unsigned long derrno;
> int ret;
> @@ -103,27 +101,28 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
> return -EINVAL;
>
> if (data[0] == 0) {
> + const struct dns_server_list_v1_header *v1;
> +
> /* It may be a server list. */
> - if (datalen <= sizeof(*bin))
> + if (datalen <= sizeof(*v1))
> return -EINVAL;
>
> - bin = (const struct dns_payload_header *)data;
> - kenter("[%u,%u],%u", bin->content, bin->version, datalen);
> - if (bin->content != DNS_PAYLOAD_IS_SERVER_LIST) {
> + v1 = (const struct dns_server_list_v1_header *)data;
> + kenter("[%u,%u],%u", v1->hdr.content, v1->hdr.version, datalen);
> + if (v1->hdr.content != DNS_PAYLOAD_IS_SERVER_LIST) {
> pr_warn_ratelimited(
> "dns_resolver: Unsupported content type (%u)\n",
> - bin->content);
> + v1->hdr.content);
> return -EINVAL;
> }
>
> - if (bin->version != 1) {
> + if (v1->hdr.version != 1) {
> pr_warn_ratelimited(
> "dns_resolver: Unsupported server list version (%u)\n",
> - bin->version);
> + v1->hdr.version);
> return -EINVAL;
> }
>
> - v1 = (const struct dns_server_list_v1_header *)bin;
> if ((v1->status != DNS_LOOKUP_GOOD &&
> v1->status != DNS_LOOKUP_GOOD_WITH_BAD)) {
> if (prep->expiry == TIME64_MAX)
>
Hi Edward and kernel experts,
Above patch(upstream commit: 1997b3cb4217b09) seems causing a keyctl05 case
to fail in LTP:
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/keyctl/keyctl05.c
It could be reproduced on a bare metal platform.
Kconfig: https://raw.githubusercontent.com/xupengfe/kconfig_diff/main/config_v6.7-rc8
Seems general kconfig could reproduce this issue.
Bisected info between v6.7-rc7(keyctl05 passed) and v6.7-rc8(keyctl05 failed)
is in attached.
keyctl05 failed in add_key with type "dns_resolver" syscall step tracked
by strace:
"
[pid 863107] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
[pid 863106] <... alarm resumed>) = 30
[pid 863107] <... add_key resumed>) = -1 EINVAL (Invalid argument)
"
Passed behavior in v6.7-rc7 kernel:
"
[pid 6726] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
[pid 6725] rt_sigreturn({mask=[]}) = 61
[pid 6726] <... add_key resumed>) = 1029222644
"
Do you mind to take a look for above issue?
Best Regards,
Thanks!
View attachment "bisect.log" of type "text/plain" (1878 bytes)
Powered by blists - more mailing lists