net: ICMPv4 echo reply packets do not use default values in the IP header

[aurel32]

Describe the bug

The ICMPv4 echo reply packets are build by reusing the echo request packet, only changing a few fields of the IP header. This means some fields of the outbound packet are taken from the inboud packet instead of using the default values from the interface, and most notably the TTL, flags and fragment offset. It might be safer to create a new packet using net_ipv4_create() and icmpv4_create() and copying only the payload.

To Reproduce
Send an ICMP echo request packet with a given TTL different than 64 (the default value in Zephyr). See how the echo reply packet comes back with the same TTL (if on the same subnet or lower if it goes through a router). Use a different TTL and observe how the echo reply packet use that different TTL.

Expected behavior
The echo reply packet should use the default IP header values and not the one from the inbound packet.

Impact
Conformance issue. It also means that the IP header sent by the board for ICMPv4 packets can partially be controlled by the inbound packet. It might have security impacts.

[tbursztyka]

It's because clone sets the same attributes to the cloned pkt as the original one.

It's just a matter to reset the ttl of the reply. Note that net_ipv4_create and net_icmpv4_create are used, it can work in both situation overwriting or not.

will send a patch, directly part of #12989

[aurel32]

Thanks this commit indeed fixes the issue. Now I wonder why not just copy the payload to avoid this kind of issue, like it is already done for IPv6?

[tbursztyka]

In IPv6 it made sens to avoid cloning due to the fact option headers should not be present in the reply. In icmpv4 it's all the same (beside the ip src/dst addr, the icmp type and the ttl obviously): using cloning was a little bit simpler. (one clone vs net_pkt_alloc_with_buffer + net_pkt_copy).
Ok a really tiny bit simpler :)