[FIX NETLINK] properly check arguments to netlink_bind()

[FIX NETLINK] properly check arguments to netlink_bind()

[Holger Eitzenberger]

Hi,

while going through going netlink code I found out that netlink_bind()
does not properly check bind parameters.  I checked both 2.6.23-rc1 as
well as 2.6.16.53, both are affected.

With a small test prog I wasn't able to crash my maschine though, but
data was accessed out of bounds.

Please apply the attached patch.

Thanks.

  /holger

Re: [FIX NETLINK] properly check arguments to netlink_bind()

[Holger Eitzenberger]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

Holger Eitzenberger <holger@my-eitzenberger.de> writes:

> while going through going netlink code I found out that netlink_bind()
> does not properly check bind parameters.  I checked both 2.6.23-rc1 as
> well as 2.6.16.53, both are affected.
>
> With a small test prog I wasn't able to crash my maschine though, but
> data was accessed out of bounds.

See my attached patch, thanks.

  /holger


[-- Attachment #2: netlink fix --]
[-- Type: text/plain, Size: 1108 bytes --]

[NETLINK] Check for correct bind parameters

Before this change it was possible to pass less than sockaddr_nl
bytes, which might lead to arbitrary data being accessed in
netlink_bind().

Signed-off-by: Holger Eitzenberger <holger@my-eitzenberger.de>

---
commit 53ba94ab22cc3338d915d684ba1012fa0419ff14
tree 3a1b1dc6cb5dacac99722b9f96fe3ba4b2d29bde
parent f695baf2df9e0413d3521661070103711545207a
author Holger Eitzenberger <holger@my-eitzenberger.de> Mon, 17 Sep 2007 22:15:37 +0200
committer Holger Eitzenberger <holger@elmo.(none)> Mon, 17 Sep 2007 22:15:37 +0200

 net/netlink/af_netlink.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5681ce3..425543d 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -598,6 +598,9 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, int addr_len
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
 	int err;
 
+	if (addr < sizeof(struct sockaddr_nl))
+		return -EINVAL;
+
 	if (nladdr->nl_family != AF_NETLINK)
 		return -EINVAL;
 

Re: [FIX NETLINK] properly check arguments to netlink_bind()

[Holger Eitzenberger]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]

The previous patchlet wasn't correct, please apply this one.

Sorry for the noise.

  /holger


[-- Attachment #2: netlink bind fix --]
[-- Type: text/plain, Size: 1123 bytes --]

[NETLINK] Check for correct bind parameters

Before this change it was possible to pass less than sockaddr_nl
bytes, which might lead to arbitrary data being accessed in
netlink_bind().

Take two.

Signed-off-by: Holger Eitzenberger <holger@my-eitzenberger.de>

---
commit 3155c34167184c31afeac2a061c0e0b9cd401d56
tree b5efe4234a5835e823b6b024f8d96e56f4abfd18
parent f695baf2df9e0413d3521661070103711545207a
author Holger Eitzenberger <holger@my-eitzenberger.de> Tue, 18 Sep 2007 23:10:11 +0200
committer Holger Eitzenberger <holger@elmo.(none)> Tue, 18 Sep 2007 23:10:11 +0200

 net/netlink/af_netlink.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5681ce3..5aaa9a7 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -598,6 +598,9 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, int addr_len
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
 	int err;
 
+	if (addr_len < sizeof(struct sockaddr_nl))
+		return -EINVAL;
+
 	if (nladdr->nl_family != AF_NETLINK)
 		return -EINVAL;
 

Re: [FIX NETLINK] properly check arguments to netlink_bind()

[David Miller]

From: Holger Eitzenberger <holger@my-eitzenberger.de>
Date: Tue, 18 Sep 2007 23:05:52 +0200

> Holger Eitzenberger <holger@my-eitzenberger.de> writes:
> 
> > while going through going netlink code I found out that netlink_bind()
> > does not properly check bind parameters.  I checked both 2.6.23-rc1 as
> > well as 2.6.16.53, both are affected.
> >
> > With a small test prog I wasn't able to crash my maschine though, but
> > data was accessed out of bounds.
> 
> See my attached patch, thanks.

Your patch is incorrect and also unnecessary.

Firstly, you patch compares the address _pointer_ against
the minimum length.  That's obviously wrong.

And if you check the call sites of the protocol ->bind() methods, they
all use on-stack buffer for the address object which is at least
MAX_SOCK_ADDR bytes in length so that the bind methods don't have to
check the size if they don't want to.

Re: [FIX NETLINK] properly check arguments to netlink_bind()

[Holger Eitzenberger]

David Miller <davem@davemloft.net> writes:

>> > while going through going netlink code I found out that netlink_bind()
>> > does not properly check bind parameters.  I checked both 2.6.23-rc1 as
>> > well as 2.6.16.53, both are affected.

> Firstly, you patch compares the address _pointer_ against
> the minimum length.  That's obviously wrong.

True, the message send later fixed that.

> And if you check the call sites of the protocol ->bind() methods, they
> all use on-stack buffer for the address object which is at least
> MAX_SOCK_ADDR bytes in length so that the bind methods don't have to
> check the size if they don't want to.

Also true, but in that case you still end up accessing uninitialized
data.  Also note that e. g. inet_bind() checks explicitely for that and
it's not clear to me why netlink_bind() is different.  Maybe you just
help me figuring out.

Another point is that simply calling

 bind(nl_fd, (struct sockaddr *)&an_int, sizeof(int));

will not return EINVAL but depends on the randomn data after an_int.

   /holger