From bdd84660c756437cf3bc8f64adf612055acc84ea Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 7 Nov 2015 11:04:13 +0000 Subject: [PATCH] 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(). --- src/net_packet.c | 6 +++++- src/route.c | 6 ++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/net_packet.c b/src/net_packet.c index fc5720a..6f72e05 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 70c5806..68a6649 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 */ -- 2.25.1