From: Etienne Dechamps 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 */