From: Etienne Dechamps <etienne@edechamps.fr>
Date: Sat, 7 Nov 2015 11:04:13 +0000 (+0000)
Subject: Make sure the packet source MAC address is always set.
X-Git-Tag: release-1.1pre12~88
X-Git-Url: https://git.tinc-vpn.org/git/browse?a=commitdiff_plain;h=bdd84660c756437cf3bc8f64adf612055acc84ea;p=tinc

Make sure the packet source MAC address is always set.

When tinc is used in router mode with a TAP device, Ethernet (MAC)
headers are not present in packets flowing over the VPN; it is the
node's responsibility to fill out this header before handing the
packet over to the TAP interface (which expects such headers).

Currently, tinc fills out the destination MAC address of the packet
(otherwise the host would not recognize the packets, and nothing would
work), but it does not fill out the source MAC address. In practice this
doesn't seem to cause any real issues (the host doesn't care about the
source address), but it does look weird when looking at the packets with
a sniffer, and it also result in the following valgrind warning:

    ==13651== Syscall param write(buf) points to uninitialised byte(s)
    ==13651==    at 0x5C4B620: __write_nocancel (syscall-template.S:81)
    ==13651==    by 0x1445AA: write_packet (device.c:183)
    ==13651==    by 0x118C7C: send_packet (net_packet.c:1259)
    ==13651==    by 0x12B70A: route_ipv4 (route.c:443)
    ==13651==    by 0x12D5F8: route (route.c:971)
    ==13651==    by 0x1152BC: receive_packet (net_packet.c:250)
    ==13651==    by 0x117E1B: receive_sptps_record (net_packet.c:904)
    ==13651==    by 0x1309A8: sptps_receive_data_datagram (sptps.c:488)
    ==13651==    by 0x130A90: sptps_receive_data (sptps.c:508)
    ==13651==    by 0x115569: receive_udppacket (net_packet.c:286)
    ==13651==    by 0x119856: handle_incoming_vpn_data (net_packet.c:1499)
    ==13651==    by 0x10F3DA: event_loop (event.c:287)
    ==13651==  Address 0xffeffea3a is on thread 1's stack
    ==13651==  in frame #6, created by receive_sptps_record (net_packet.c:821)
    ==13651==

This commit fixes the issue by filling out the source MAC address. It is
generated by negating the last byte of the device MAC address, which is
consistent with what route_arp() does.

In addition, this commit stops route_arp() from filling out the Ethernet
header of the packet - this is the responsibility of send_packet(), not
route().
---

diff --git a/src/net_packet.c b/src/net_packet.c
index fc5720a2..6f72e05b 100644
--- a/src/net_packet.c
+++ b/src/net_packet.c
@@ -1252,8 +1252,12 @@ void send_packet(node_t *n, vpn_packet_t *packet) {
 	// If it's for myself, write it to the tun/tap device.
 
 	if(n == myself) {
-		if(overwrite_mac)
+		if(overwrite_mac) {
 			 memcpy(DATA(packet), mymac.x, ETH_ALEN);
+			 // Use an arbitrary fake source address.
+			 memcpy(DATA(packet) + ETH_ALEN, DATA(packet), ETH_ALEN);
+			 DATA(packet)[ETH_ALEN * 2 - 1] ^= 0xFF;
+		}
 		n->out_packets++;
 		n->out_bytes += packet->len;
 		devops.write(packet);
diff --git a/src/route.c b/src/route.c
index 70c5806b..68a66492 100644
--- a/src/route.c
+++ b/src/route.c
@@ -784,15 +784,13 @@ static void route_arp(node_t *source, vpn_packet_t *packet) {
 	if(subnet->owner == myself)
 		return;                                          /* silently ignore */
 
-	memcpy(DATA(packet), DATA(packet) + ETH_ALEN, ETH_ALEN); /* copy destination address */
-	DATA(packet)[ETH_ALEN * 2 - 1] ^= 0xFF;                  /* mangle source address so it looks like it's not from us */
-
 	memcpy(&addr, arp.arp_tpa, sizeof addr);                 /* save protocol addr */
 	memcpy(arp.arp_tpa, arp.arp_spa, sizeof addr);           /* swap destination and source protocol address */
 	memcpy(arp.arp_spa, &addr, sizeof addr);                 /* ... */
 
 	memcpy(arp.arp_tha, arp.arp_sha, ETH_ALEN);              /* set target hard/proto addr */
-	memcpy(arp.arp_sha, DATA(packet) + ETH_ALEN, ETH_ALEN);  /* add fake source hard addr */
+	memcpy(arp.arp_sha, DATA(packet) + ETH_ALEN, ETH_ALEN);  /* set source hard/proto addr */
+	arp.arp_sha[ETH_ALEN - 1] ^= 0xFF;                       /* for consistency with route_packet() */
 	arp.arp_op = htons(ARPOP_REPLY);
 
 	/* Copy structs on stack back to packet */