[RFC bluetooth-next 0/2] ieee802154: socket: fix buffer overflow

[RFC bluetooth-next 0/2] ieee802154: socket: fix buffer overflow

[Alexander Aring]

Hi,

this is some critical bug fix here and I don't know how do deal with that now.
I don't know if you can "get root" by this security issue. But the socket interface
can be simple loaded by user via module-autoloading while using the address family.
Maybe there is no security issue and the buffers are cutted off. I don't know, but
there is definitely something wrong here.

In my opinion af_ieee802154 should go to stable (bluetooth), but this will break
the complete userspace interface for every application. I think there are no many
users so I will simple send

Patch 1/2 "af_ieee802154: fix struct ieee802154_addr_sa size"

to bluetooth. This is a RFC to talk about this issue and if somebody knows a better
way please tell that here. Note also all userspace applications need to be updated
after this patch. I really don't know how to deal with such issue and CC here a lot
of well known linux hackers and would be glad if I get any suggestions about that.

Or maybe I should go to the netdev mailinglist with this issue.

- Alex

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Werner Almesberger <werner@almesberger.net>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>

Alexander Aring (2):
  af_ieee802154: fix struct ieee802154_addr_sa size
  ieee802154: socket: add BUILD_BUG_ON for cast check

 include/net/af_ieee802154.h | 2 +-
 net/ieee802154/socket.c     | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.2.1

[RFC bluetooth-next 1/2] af_ieee802154: fix struct ieee802154_addr_sa size

[Alexander Aring]

The structure "ieee802154_addr_sa" need to fit into the u8 sa_data[14]
from struct sockaddr, because there is a casting of "struct sockaddr" and
"struct ieee802154_sockaddr".

I tested a compiling with a 32 bit system and detected that the "struct
ieee802154_sockaddr", which contains the ieee802154_addr_sa structure, has a
size of 20 bytes. The "struct sockaddr" has a size of 16 bytes. This doesn't
fit together and some buffers are overflows. This patch changes the
"addr_type" type definition from "int" to "u8". After this change it will be
fits together.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/af_ieee802154.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/af_ieee802154.h b/include/net/af_ieee802154.h
index 7d38e2f..3652269 100644
--- a/include/net/af_ieee802154.h
+++ b/include/net/af_ieee802154.h
@@ -33,7 +33,7 @@ enum {
 #define IEEE802154_ADDR_LEN	8
 
 struct ieee802154_addr_sa {
-	int addr_type;
+	u8 addr_type;
 	u16 pan_id;
 	union {
 		u8 hwaddr[IEEE802154_ADDR_LEN];
-- 
2.2.1

[RFC bluetooth-next 2/2] ieee802154: socket: add BUILD_BUG_ON for cast check

[Alexander Aring]

This patch adds an overdue BUILD_BUG_ON size check that the
"struct sockaddr_ieee802154" is never greater than the
"struct sockaddr". The IEEE 802.15.4 will cast the "struct sockaddr" to
struct sockaddr_ieee802154" at several places.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/ieee802154/socket.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 2878d8c..9c09291 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -1085,6 +1085,9 @@ static int __init af_ieee802154_init(void)
 {
 	int rc = -EINVAL;
 
+	BUILD_BUG_ON(sizeof(struct sockaddr_ieee802154) >
+		     sizeof(struct sockaddr));
+
 	rc = proto_register(&ieee802154_raw_prot, 1);
 	if (rc)
 		goto out;
-- 
2.2.1

Re: [RFC bluetooth-next 1/2] af_ieee802154: fix struct ieee802154_addr_sa size

[Phoebe Buckheister]

Hi,

On Sat, 10 Jan 2015 23:33:25 +0100
Alexander Aring <alex.aring@gmail.com> wrote:

> The structure "ieee802154_addr_sa" need to fit into the u8 sa_data[14]
> from struct sockaddr, because there is a casting of "struct sockaddr"
> and "struct ieee802154_sockaddr".
> 
> I tested a compiling with a 32 bit system and detected that the
> "struct ieee802154_sockaddr", which contains the ieee802154_addr_sa
> structure, has a size of 20 bytes. The "struct sockaddr" has a size
> of 16 bytes. This doesn't fit together and some buffers are
> overflows. This patch changes the "addr_type" type definition from
> "int" to "u8". After this change it will be fits together.

Do look at how Unix domain sockets handle the problem. Also, IPv6
addresses exceed sizeof(struct sockaddr) quite significantly. Casting
pointers isn't a problem, only if we *ever* store our addrs to a struct
sockaddr will we have a problem.

Perhaps I am missing something, but from what I can tell, i think the
code is safe at least in that regard.

> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  include/net/af_ieee802154.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/af_ieee802154.h b/include/net/af_ieee802154.h
> index 7d38e2f..3652269 100644
> --- a/include/net/af_ieee802154.h
> +++ b/include/net/af_ieee802154.h
> @@ -33,7 +33,7 @@ enum {
>  #define IEEE802154_ADDR_LEN	8
>  
>  struct ieee802154_addr_sa {
> -	int addr_type;
> +	u8 addr_type;
>  	u16 pan_id;
>  	union {
>  		u8 hwaddr[IEEE802154_ADDR_LEN];