Fix L2 dst address computation (very intrusive) (#4145)

* Less intrusive L2 dst computation (especially ARP)

* Apply guedou suggestions

Author: gpotter2

scapy/layers/inet.py

@@ -1099,6 +1099,9 @@ def mysummary(self):
 
 
 def inet_register_l3(l2, l3):
+    """
+    Resolves the default L2 destination address when IP is used.
+    """
     return getmacbyip(l3.dst)
 
 

scapy/layers/inet6.py

@@ -475,6 +475,9 @@ def dispatch_hook(cls, _pkt=None, *_, **kargs):
 
 
 def inet6_register_l3(l2, l3):
+    """
+    Resolves the default L2 destination address when IPv6 is used.
+    """
     return getmacbyip6(l3.dst)
 
 

scapy/layers/l2.py

@@ -166,6 +166,12 @@ def __init__(self, name):
 
     def i2h(self, pkt, x):
         # type: (Optional[Packet], Optional[str]) -> str
+        if x is None and pkt is not None:
+            x = "None (resolved on build)"
+        return super(DestMACField, self).i2h(pkt, x)
+
+    def i2m(self, pkt, x):
+        # type: (Optional[Packet], Optional[str]) -> bytes
         if x is None and pkt is not None:
             try:
                 x = conf.neighbor.resolve(pkt, pkt.payload)
@@ -176,12 +182,10 @@ def i2h(self, pkt, x):
                     raise ScapyNoDstMacException()
                 else:
                     x = "ff:ff:ff:ff:ff:ff"
-                    warning("Mac address to reach destination not found. Using broadcast.")  # noqa: E501
-        return super(DestMACField, self).i2h(pkt, x)
-
-    def i2m(self, pkt, x):
-        # type: (Optional[Packet], Optional[str]) -> bytes
-        return super(DestMACField, self).i2m(pkt, self.i2h(pkt, x))
+                    warning(
+                        "MAC address to reach destination not found. Using broadcast."
+                    )
+        return super(DestMACField, self).i2m(pkt, x)
 
 
 class SourceMACField(MACField):
@@ -311,8 +315,10 @@ class LLC(Packet):
                    ByteField("ctrl", 0)]
 
 
-def l2_register_l3(l2, l3):
-    # type: (Packet, Packet) -> Optional[str]
+def l2_register_l3(l2: Packet, l3: Packet) -> Optional[str]:
+    """
+    Delegates resolving the default L2 destination address to the payload of L3.
+    """
     neighbor = conf.neighbor  # type: Neighbor
     return neighbor.resolve(l2, l3.payload)
 
@@ -554,15 +560,28 @@ def mysummary(self):
         return self.sprintf("ARP %op% %psrc% > %pdst%")
 
 
-def l2_register_l3_arp(l2, l3):
-    # type: (Packet, Packet) -> Optional[str]
-    # TODO: support IPv6?
-    plen = l3.plen if l3.plen is not None else l3.get_field("pdst").i2len(l3, l3.pdst)
+def l2_register_l3_arp(l2: Packet, l3: Packet) -> Optional[str]:
+    """
+    Resolves the default L2 destination address when ARP is used.
+    """
+    if l3.op == 1:  # who-has
+        return "ff:ff:ff:ff:ff:ff"
+    elif l3.op == 2:  # is-at
+        log_runtime.warning(
+            "You should be providing the Ethernet destination MAC address when "
+            "sending an is-at ARP."
+        )
+    # Need ARP request to send ARP request...
+    plen = l3.get_field("pdst").i2len(l3, l3.pdst)
     if plen == 4:
         return getmacbyip(l3.pdst)
+    elif plen == 32:
+        from scapy.layers.inet6 import getmacbyip6
+        return getmacbyip6(l3.pdst)
+    # Can't even do that
     log_runtime.warning(
-        "Unable to guess L2 MAC address from an ARP packet with a "
-        "non-IPv4 pdst. Provide it manually !"
+        "You should be providing the Ethernet destination mac when sending this "
+        "kind of ARP packets."
     )
     return None
 

test/scapy/layers/inet.uts

@@ -572,6 +572,28 @@ assert isinstance(pkt, UDP) and pkt.dport == 5353
 pkt = pkt.payload
 assert isinstance(pkt, DNS) and isinstance(pkt.payload, NoPayload)
 
+= Layer binding with show()
+* getmacbyip must only be called when building
+
+import mock
+
+def _err(*_):
+    raise ValueError
+
+with mock.patch("scapy.layers.l2.getmacbyip", side_effect=_err):
+    with mock.patch("scapy.layers.inet.getmacbyip", side_effect=_err):
+        # ARP who-has should never call getmacbyip
+        pkt1 = Ether() / ARP(pdst="10.0.0.1")
+        pkt1.show()
+        bytes(pkt1)
+        # IP should only call getmacbyip when building
+        pkt2 = Ether() / IP(dst="10.0.0.1")
+        pkt2.show()
+        try:
+            bytes(pkt2)
+            assert False, "Should have called getmacbyip"
+        except ValueError:
+            pass
 
 ############
 ############