[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[Sven Eckelmann]

The routing checks are validating the sender mac address. They reject every
sender mac address which is a broadcast. But they also have to reject
zero-mac address and multicast mac addresses.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/routing.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 610f2c4..b648caf 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -196,8 +196,8 @@ bool batadv_check_management_packet(struct sk_buff *skb,
 	if (!is_broadcast_ether_addr(ethhdr->h_dest))
 		return false;
 
-	/* packet with broadcast sender address */
-	if (is_broadcast_ether_addr(ethhdr->h_source))
+	/* packet with invalid sender address */
+	if (!is_valid_ether_addr(ethhdr->h_source))
 		return false;
 
 	/* create a copy of the skb, if needed, to modify it. */
@@ -357,8 +357,8 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
 	if (is_broadcast_ether_addr(ethhdr->h_dest))
 		goto out;
 
-	/* packet with broadcast sender address */
-	if (is_broadcast_ether_addr(ethhdr->h_source))
+	/* packet with invalid sender address */
+	if (!is_valid_ether_addr(ethhdr->h_source))
 		goto out;
 
 	/* not for me */
@@ -449,8 +449,8 @@ static int batadv_check_unicast_packet(struct batadv_priv *bat_priv,
 	if (is_broadcast_ether_addr(ethhdr->h_dest))
 		return -EBADR;
 
-	/* packet with broadcast sender address */
-	if (is_broadcast_ether_addr(ethhdr->h_source))
+	/* packet with invalid sender address */
+	if (!is_valid_ether_addr(ethhdr->h_source))
 		return -EBADR;
 
 	/* not for me */
@@ -1091,8 +1091,8 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
 	if (!is_broadcast_ether_addr(ethhdr->h_dest))
 		goto out;
 
-	/* packet with broadcast sender address */
-	if (is_broadcast_ether_addr(ethhdr->h_source))
+	/* packet with invalid sender address */
+	if (!is_valid_ether_addr(ethhdr->h_source))
 		goto out;
 
 	/* ignore broadcasts sent by myself */
-- 
2.8.1

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Reject unicast packet for zero/mcast recepient

[Sven Eckelmann]

An unicast batman-adv packet cannot be transmitted to a multicast or zero
address. So reject incoming packets which still have these classes of
addresses as destination mac address.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/routing.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index b648caf..44e1e80 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -353,8 +353,8 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
 
 	ethhdr = eth_hdr(skb);
 
-	/* packet with unicast indication but broadcast recipient */
-	if (is_broadcast_ether_addr(ethhdr->h_dest))
+	/* packet with unicast indication but non-unicast recipient */
+	if (!is_valid_ether_addr(ethhdr->h_dest))
 		goto out;
 
 	/* packet with invalid sender address */
@@ -445,8 +445,8 @@ static int batadv_check_unicast_packet(struct batadv_priv *bat_priv,
 
 	ethhdr = eth_hdr(skb);
 
-	/* packet with unicast indication but broadcast recipient */
-	if (is_broadcast_ether_addr(ethhdr->h_dest))
+	/* packet with unicast indication but non-unicast recipient */
+	if (!is_valid_ether_addr(ethhdr->h_dest))
 		return -EBADR;
 
 	/* packet with invalid sender address */
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[Linus Lüssing]

On Mon, Jul 18, 2016 at 12:15:40AM +0200, Sven Eckelmann wrote:
> The routing checks are validating the sender mac address. They reject every
> sender mac address which is a broadcast. But they also have to reject
> zero-mac address and multicast mac addresses.

Initially I was a little shocked because there are legitimate
cases for zero-source MAC addresesses. But then I saw in the code
that you are talking about source MAC address of the outter
batman-adv frame :). Maybe that could be clarified in the commit
message?

For batadv_check_management_packet(), agreed, I guess much of the
protocol does rely on valid source addresses.

For data packets, I'm not quite sure, though. Could be interesting
to not restrict that now to still allow enhancements regarding
privacy, I think. And zero-source MAC addresses shouldn't harm
anything in the case of data packets, should they?

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Reject unicast packet for zero/mcast recepient

[Linus Lüssing]

On Mon, Jul 18, 2016 at 12:15:41AM +0200, Sven Eckelmann wrote:
> An unicast batman-adv packet cannot be transmitted to a multicast or zero
> address. So reject incoming packets which still have these classes of
> addresses as destination mac address.

Same here, see previous mail.

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[Sven Eckelmann]

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

On Samstag, 6. August 2016 06:42:44 CEST Linus Lüssing wrote:
> On Mon, Jul 18, 2016 at 12:15:40AM +0200, Sven Eckelmann wrote:
> > The routing checks are validating the sender mac address. They reject every
> > sender mac address which is a broadcast. But they also have to reject
> > zero-mac address and multicast mac addresses.
> 
> Initially I was a little shocked because there are legitimate
> cases for zero-source MAC addresesses. But then I saw in the code
> that you are talking about source MAC address of the outter
> batman-adv frame :). Maybe that could be clarified in the commit
> message?

Ah yes, you are right. This should be described better in the commit
message.

> For batadv_check_management_packet(), agreed, I guess much of the
> protocol does rely on valid source addresses.

Yes, think so too.

> For data packets, I'm not quite sure, though. Could be interesting
> to not restrict that now to still allow enhancements regarding
> privacy, I think. And zero-source MAC addresses shouldn't harm
> anything in the case of data packets, should they?

So you would prefer here that is_broadcast_ether_addr is replaced
for bcast and ucast packets with is_multicast_ether_addr? Same for
patch 2, right?

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[Sven Eckelmann]

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

On Samstag, 6. August 2016 10:27:08 CEST Sven Eckelmann wrote:
[...]
> > For data packets, I'm not quite sure, though. Could be interesting
> > to not restrict that now to still allow enhancements regarding
> > privacy, I think. And zero-source MAC addresses shouldn't harm
> > anything in the case of data packets, should they?
> 
> So you would prefer here that is_broadcast_ether_addr is replaced
> for bcast and ucast packets with is_multicast_ether_addr? Same for
> patch 2, right?

Hm, no. This doesn't make any sense for patch 2 because patch 2 is about the 
destination and a destination with zero mac address isn't valid. Or do you see 
any reason to accept zero mac addresses as destination in the outer ethernet 
header?

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Don't allow zero and multicast sender address

[Sven Eckelmann]

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

On Samstag, 6. August 2016 06:42:44 CEST Linus Lüssing wrote:
[...]
> For data packets, I'm not quite sure, though. Could be interesting
> to not restrict that now to still allow enhancements regarding
> privacy, I think. And zero-source MAC addresses shouldn't harm
> anything in the case of data packets, should they?

I have now changed it but I personally think that this will not work. Many 
filters/firewalls go crazy when they see zero mac addresses, the first bcast 
batadv_is_my_mac check is not working anymore and it should not be possible to 
send over IBSS or Sta-to-AP with a zero mac source address. From AP-to-Sta is 
not a problem because it can use 4 addresses in its header. But the other way 
around is problematic because only 3 address are in the wifi header. A 
transfer with zero mac address as source address from Sta-to-AP with ath9k/
mac80211 should end up in an Ack back to 00:00:00:00:00:00 and a 
deauthentication frame to 00:00:00:00:00:00.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]