Fix location estimation via ICMP traceroute

netenv/location.go

@@ -21,6 +21,12 @@ import (
 )
 
 var (
+	// locationTestingIPv4 holds the IP address of the server that should be
+	// tracerouted to find the location of the device. The ping will never reach
+	// the destination in most cases.
+	// The selection of this IP requires sensitivity, as the IP address must be
+	// far enough away to produce good results.
+	// At the same time, the IP address should be common and not raise attention.
 	locationTestingIPv4     = "1.1.1.1"
 	locationTestingIPv4Addr *net.IPAddr
 
@@ -162,10 +168,10 @@ func (a sortLocationsByAccuracy) Len() int           { return len(a) }
 func (a sortLocationsByAccuracy) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
 func (a sortLocationsByAccuracy) Less(i, j int) bool { return a[j].IsMoreAccurateThan(a[i]) }
 
-func SetInternetLocation(ip net.IP, source DeviceLocationSource) (ok bool) {
+func SetInternetLocation(ip net.IP, source DeviceLocationSource) (dl *DeviceLocation, ok bool) {
 	// Check if IP is global.
 	if netutils.GetIPScope(ip) != netutils.Global {
-		return false
+		return nil, false
 	}
 
 	// Create new location.
@@ -188,29 +194,32 @@ func SetInternetLocation(ip net.IP, source DeviceLocationSource) (ok bool) {
 		loc.Location = geoLoc
 	}
 
+	addLocation(loc)
+	return loc, true
+}
+
+func addLocation(dl *DeviceLocation) {
 	locationsLock.Lock()
 	defer locationsLock.Unlock()
 
 	// Add to locations, if better.
 	var exists bool
 	for i, existing := range locations.All {
-		if ip.Equal(existing.IP) {
+		if (dl.IP == nil && existing.IP == nil) || dl.IP.Equal(existing.IP) {
 			exists = true
-			if loc.IsMoreAccurateThan(existing) {
+			if dl.IsMoreAccurateThan(existing) {
 				// Replace
-				locations.All[i] = loc
+				locations.All[i] = dl
 				break
 			}
 		}
 	}
 	if !exists {
-		locations.All = append(locations.All, loc)
+		locations.All = append(locations.All, dl)
 	}
 
 	// Sort locations.
 	sort.Sort(sortLocationsByAccuracy(locations.All))
-
-	return true
 }
 
 // DEPRECATED: Please use GetInternetLocation instead.
@@ -292,20 +301,18 @@ func getLocationFromUPnP() (ok bool) {
 }
 */
 
-func getLocationFromTraceroute() (v4ok bool) {
+func getLocationFromTraceroute() (dl *DeviceLocation, err error) {
 	// Create connection.
 	conn, err := net.ListenPacket("ip4:icmp", "")
 	if err != nil {
-		log.Warningf("netenv: location: failed to open icmp conn: %s", err)
-		return false
+		return nil, fmt.Errorf("failed to open icmp conn: %s", err)
 	}
 	v4Conn := ipv4.NewPacketConn(conn)
 
 	// Generate a random ID for the ICMP packets.
 	generatedID, err := rng.Number(0xFFFF) // uint16
 	if err != nil {
-		log.Warningf("netenv: location: failed to generate icmp msg ID: %s", err)
-		return false
+		return nil, fmt.Errorf("failed to generate icmp msg ID: %s", err)
 	}
 	msgID := int(generatedID)
 	var msgSeq int
@@ -323,7 +330,7 @@ func getLocationFromTraceroute() (v4ok bool) {
 	maxHops := 4 // add one for every reply that is not global
 
 	// Get additional listener for ICMP messages via the firewall.
-	icmpPacketsViaFirewall, doneWithListeningToICMP := ListenToICMP()
+	icmpPacketsViaFirewall, doneWithListeningToICMP := ListenToICMP(locationTestingIPv4Addr.IP)
 	defer doneWithListeningToICMP()
 
 nextHop:
@@ -339,15 +346,13 @@ nextHop:
 			// Make packet data.
 			pingPacket, err := pingMessage.Marshal(nil)
 			if err != nil {
-				log.Warningf("netenv: location: failed to build icmp packet: %s", err)
-				return false
+				return nil, fmt.Errorf("failed to build icmp packet: %s", err)
 			}
 
 			// Set TTL on IP packet.
 			err = v4Conn.SetTTL(i)
 			if err != nil {
-				log.Warningf("netenv: location: failed to set icmp packet TTL: %s", err)
-				return false
+				return nil, fmt.Errorf("failed to set icmp packet TTL: %s", err)
 			}
 
 			// Send ICMP packet.
@@ -357,8 +362,7 @@ nextHop:
 						continue
 					}
 				}
-				log.Warningf("netenv: location: failed to send icmp packet: %s", err)
-				return false
+				return nil, fmt.Errorf("failed to send icmp packet: %s", err)
 			}
 
 			// Listen for replies of the ICMP packet.
@@ -381,7 +385,7 @@ nextHop:
 					}
 					// We received a reply, so we did not trigger a time exceeded response on the way.
 					// This means we were not able to find the nearest router to us.
-					return false
+					return nil, errors.New("received final echo reply without time exceeded messages")
 				case layers.ICMPv4TypeDestinationUnreachable,
 					layers.ICMPv4TypeTimeExceeded:
 					// Continue processing.
@@ -413,13 +417,17 @@ nextHop:
 				switch icmpPacket.TypeCode.Type() {
 				case layers.ICMPv4TypeDestinationUnreachable:
 					// We have received a valid destination unreachable response, abort.
-					return false
+					return nil, errors.New("destination unreachable")
 
 				case layers.ICMPv4TypeTimeExceeded:
 					// We have received a valid time exceeded error.
 					// If message came from a global unicast, us it!
 					if netutils.GetIPScope(remoteIP) == netutils.Global {
-						return SetInternetLocation(remoteIP, SourceTraceroute)
+						dl, ok := SetInternetLocation(remoteIP, SourceTraceroute)
+						if !ok {
+							return nil, errors.New("invalid IP address")
+						}
+						return dl, nil
 					}
 
 					// Otherwise, continue.
@@ -430,7 +438,7 @@ nextHop:
 	}
 
 	// We did not receive anything actionable.
-	return false
+	return nil, errors.New("did not receive any actionable ICMP reply")
 }
 
 func recvICMP(currentHop int, icmpPacketsViaFirewall chan packet.Packet) (
@@ -455,7 +463,7 @@ func recvICMP(currentHop int, icmpPacketsViaFirewall chan packet.Packet) (
 			}
 			return pkt.Info().RemoteIP(), icmp4, true
 
-		case <-time.After(time.Duration(currentHop*10+50) * time.Millisecond):
+		case <-time.After(time.Duration(currentHop*20+100) * time.Millisecond):
 			return nil, nil, false
 		}
 	}