ICMP Extension Header wrong padding placement

[ventaquil]

Brief description

While attempting to convert a ICMP Extension Header packet into a bytes object using the raw(icmp) function, the padding is placed in the incorrect location. As a result, the value returned by raw(icmp) differs from the original pkt packet, leading to functionality errors.

Scapy version

2.5.0.dev272

Python version

3.10.12

Operating system

Linux 5.15.133.1

Additional environment information

Ubuntu 22.04 WSL2

How to reproduce

1. Take prepared packet with ICMP Extension Header.

   >>> pkt = b'\x00\x15]\x94AY\x00\x15]\x07\xcb\x04\x08\x00E\x00\x00\xb0?2\x00\x00\xe6\x01\x1b\xabh,\x1f\x1d\xac\x1cF\n\x0b\x00Ll\x00\x11\x00\x00E \x00<\x96\xdf\x00\x00\x02\x11\xa7\xc6\xac\x1cF\n(Q_t\xb8-\x82\xb3\x00(xt@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00 \x00\x02\xff\x00\x10\x01\x01\tp2\x01\x05\xde\xd2\x01\x05\x9c\xc3\x02'
   

2. Add binding to ICMP Extension Header layer.

   >>> from scapy.contrib.icmp_extensions import ICMPExtensionHeader
   >>> bind_layers(ICMP, ICMPExtensionHeader, type=11, code=0)
   

3. Forge Ether packet.

   >>> ether = Ether(pkt)
   >>> ether
   <Ether  dst=00:15:5d:94:41:59 src=00:15:5d:07:cb:04 type=IPv4 |<IP  version=4 ihl=5 tos=0x0 len=176 id=16178 flags= frag=0 ttl=230 proto=icmp chksum=0x1bab src=104.44.31.29 dst=172.28.70.10 |<ICMP  type=time-exceeded code=ttl-zero-during-transit chksum=0x4c6c reserved=0 length=17 unused=0 |<IPerror  version=4 ihl=5 tos=0x20 len=60 id=38623 flags= frag=0 ttl=2 proto=udp chksum=0xa7c6 src=172.28.70.10 dst=40.81.95.116 |<UDPerror  sport=47149 dport=33459 len=40 chksum=0x7874 |<Raw  load=b'@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_' |<Padding  load=b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' |<ICMPExtensionHeader  version=2 reserved=0 chksum=767 |<ICMPExtensionMPLS  len=16 classnum=1 classtype=1 stack=[<MPLS  label=38659 cos=1 s=0 ttl=1 |<MPLS  label=24045 cos=1 s=0 ttl=1 |<MPLS  label=22988 cos=1 s=1 ttl=2 |>>>] |>>>>>>>>>
   

4. Turn into bytes and compare.

   >>> raw(ether)
   b'\x00\x15]\x94AY\x00\x15]\x07\xcb\x04\x08\x00E\x00\x00\xb0?2\x00\x00\xe6\x01\x1b\xabh,\x1f\x1d\xac\x1cF\n\x0b\x00Ll\x00\x11\x00\x00E \x00<\x96\xdf\x00\x00\x02\x11\xa7\xc6\xac\x1cF\n(Q_t\xb8-\x82\xb3\x00(xt@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_ \x00\x02\xff\x00\x10\x01\x01\tp2\x01\x05\xde\xd2\x01\x05\x9c\xc3\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
   >>> pkt
   b'\x00\x15]\x94AY\x00\x15]\x07\xcb\x04\x08\x00E\x00\x00\xb0?2\x00\x00\xe6\x01\x1b\xabh,\x1f\x1d\xac\x1cF\n\x0b\x00Ll\x00\x11\x00\x00E \x00<\x96\xdf\x00\x00\x02\x11\xa7\xc6\xac\x1cF\n(Q_t\xb8-\x82\xb3\x00(xt@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00 \x00\x02\xff\x00\x10\x01\x01\tp2\x01\x05\xde\xd2\x01\x05\x9c\xc3\x02'
   

Actual result

b'\x00\x15]\x94AY\x00\x15]\x07\xcb\x04\x08\x00E\x00\x00\xb0?2\x00\x00\xe6\x01\x1b\xabh,\x1f\x1d\xac\x1cF\n\x0b\x00Ll\x00\x11\x00\x00E \x00<\x96\xdf\x00\x00\x02\x11\xa7\xc6\xac\x1cF\n(Q_t\xb8-\x82\xb3\x00(xt@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_ \x00\x02\xff\x00\x10\x01\x01\tp2\x01\x05\xde\xd2\x01\x05\x9c\xc3\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

Expected result

b'\x00\x15]\x94AY\x00\x15]\x07\xcb\x04\x08\x00E\x00\x00\xb0?2\x00\x00\xe6\x01\x1b\xabh,\x1f\x1d\xac\x1cF\n\x0b\x00Ll\x00\x11\x00\x00E \x00<\x96\xdf\x00\x00\x02\x11\xa7\xc6\xac\x1cF\n(Q_t\xb8-\x82\xb3\x00(xt@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00 \x00\x02\xff\x00\x10\x01\x01\tp2\x01\x05\xde\xd2\x01\x05\x9c\xc3\x02'

Related resources

No response

[ventaquil]

Essentially, the problem is that Padding is treated as... well, padding.

https://github.com/secdev/scapy/blob/0708e6743cb4ef119c74725bb99047edfb66f66d/scapy/packet.py#L751-L761

The ICMPExtensionHeader.do_build method returns data before Padding.build_padding does.

https://github.com/secdev/scapy/blob/0708e6743cb4ef119c74725bb99047edfb66f66d/scapy/packet.py#L1941-L1953

Take a look at this simple example.

>>> (Padding(16) / ICMPExtensionHeader()).do_build()
b' \x00\xdf\xff'
>>> (Padding(16) / ICMPExtensionHeader()).build_padding()
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>> (Padding(16) / ICMPExtensionHeader()).build()
b' \x00\xdf\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

As a result we may observe moving Padding behind the ICMP Extension Header struct.

The fix is simple and obvious - treat Padding as space between without moving it to the end of the datagram. However I cannot guess negative impact of such change.

[gpotter2]

This should be fixed in https://github.com/secdev/scapy/pull/4332

[ventaquil]

Excellent, thank you @gpotter2! I am waiting for merge.