netdev - Re: [PATCH] rfcomm: fix information leak in rfcomm_sock

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 5 Jan 2016 22:48:18 -0800
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Insu Yun <wuninsu@...il.com>
Cc:	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Peter Hurley <peter@...leysoftware.com>,
	JAGANATH KANAKKASSERY <jaganath.k@...sung.com>,
	ying.xue@...driver.com,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	BlueZ development <linux-bluetooth@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, taesoo@...ech.edu,
	yeongjin.jang@...ech.edu, insu@...ech.edu, changwoo@...ech.edu
Subject: Re: [PATCH] rfcomm: fix information leak in rfcomm_sock_bind

Hi Insu,

> if addr_len < sizeof(sa), sa.rc_bdaddr(4bytes) can be leaked 
> by using rfcomm_sock_getname()
> 
> Signed-off-by: Insu Yun <wuninsu@...il.com>
> ---
> net/bluetooth/rfcomm/sock.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 7511df7..d61dfdc 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -336,14 +336,15 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
> {
> 	struct sockaddr_rc sa;
> 	struct sock *sk = sock->sk;
> -	int len, err = 0;
> +	int err = 0;
> 
> 	if (!addr || addr->sa_family != AF_BLUETOOTH)
> 		return -EINVAL;
> 
> -	memset(&sa, 0, sizeof(sa));
> -	len = min_t(unsigned int, sizeof(sa), addr_len);
> -	memcpy(&sa, addr, len);

what are we leaking here?

If you make addr_len (controlled by userspace) smaller than struct sockaddr_rc, the kernel will only copy addr_len of sockaddr_rc to userspace. What is the kernel leaking again? We are happy to hand out the full sockaddr_rc in the first place.

> +	if (addr_len != sizeof(sa))
> +		return -EINVAL;
> +
> +	memcpy(&sa, addr, addr_len);
> 
> 	BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);

While I give you the fact that we should require at minimum to provide bdaddr_t and channel just as a good safe guard, but even if you don't, then it just means you try to bind to a truncated address, which will fail when calling connect().

So the sockaddr_rc sa will be memset to zero. Which means if you provide less than sockaddr_rc you get a truncated rc_bdaddr and rc_channel set to zero. So what is getname() leaking now. It will leak zeros. It will not leak random kernel memory.

Please help me to understand where this is an information leak. Since leaking memory that is explicitly set to zero is not an information leak.

And just FYI you can bind a RFCOMM socket to a truncated address and channel 0 in a valid way with the full size of sockaddr_rc. That is still valid. We do not discriminate any addresses. If some company manages to get the IEEE to assign that address to them, than that is a valid address.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html