if addr_len < sizeof(sa), sa.rc_bdaddr(4bytes) can be leaked
by using rfcomm_sock_getname()
Signed-off-by: Insu Yun <[email protected]>
---
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);
+ if (addr_len != sizeof(sa))
+ return -EINVAL;
+
+ memcpy(&sa, addr, addr_len);
BT_DBG("sk %p %pMR", sk, &sa.rc_bdaddr);
--
1.9.1
On Wed, Jan 6, 2016 at 1:48 AM, Marcel Holtmann <[email protected]> wrote:
> Hi Insu,
>
> > if addr_len < sizeof(sa), sa.rc_bdaddr(4bytes) can be leaked
> > by using rfcomm_sock_getname()
> >
> > Signed-off-by: Insu Yun <[email protected]>
> > ---
> > 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.
>
Yes. sockaddr_rc is in the kernel stack initialized only amount of
addr_len. Remaining part is uninitialized kernel stack which may contain
some address.
>
> > + 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().
>
bind will not be failed.
>
> 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.
>
Yes not random. Sorry for confusing, it only leaks unitiailized memory in
stack.
>
> 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
>
#include <bluetooth/bluetooth.h>
#include <bluetooth/rfcomm.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <stdint.h>
#include <stdlib.h>
#define BUG
int main() {
int s = socket(AF_BLUETOOTH, SOCK_STREAM, BTPROTO_RFCOMM);
struct sockaddr addr;
int len = sizeof(addr);
int i;
if (s == -1) {
perror("socket");
exit(-1);
}
memset(&addr, 0, sizeof(addr));
addr.sa_family = AF_BLUETOOTH;
#ifdef BUG
if (bind(s, &addr, sizeof(addr.sa_family)) == -1) {
#else
if (bind(s, &addr, sizeof(addr)) == -1) {
#endif
perror("bind");
exit(-1);
}
getsockname(s, &addr, &len);
printf("%08X\n", ((struct sockaddr_rc*)&addr)->rc_bdaddr);
}
This is PoC code. When bind, it passes kernel stack address initialized by
addr_len.
But in current rfcomm_sock_bind, it copies uninitialized memory value to
src variable.
After that, it can be accessed in user space using getsockname.
Thanks
--
Regards
Insu Yun
Hi Insu,
> if addr_len < sizeof(sa), sa.rc_bdaddr(4bytes) can be leaked
> by using rfcomm_sock_getname()
>
> Signed-off-by: Insu Yun <[email protected]>
> ---
> 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