Do not send ICMPv4 responses to broadcasts #67
Conversation
src/iface/ethernet.rs
Outdated
| @@ -19,6 +20,8 @@ use socket::{Socket, SocketSet, AnySocket}; | |||
| #[cfg(feature = "proto-tcp")] use socket::TcpSocket; | |||
| use super::ArpCache; | |||
|
|
|||
| const SIZEOF_ICMP_ERR_PAYLOAD: usize = 44; | |||
There was a problem hiding this comment.
Does something like this belong in wire::icmpv4?
Edit: Still same question, but it isn't 44 any more. Again, not sure what I was thinking.
src/iface/ethernet.rs
Outdated
| // If the datagram does not have a payload greater than or equal | ||
| // to 64 bytes minus the size of the ipv4 header in length, only | ||
| // send back the size of given payload. | ||
| let payload_len = min(ip_payload.len(), SIZEOF_ICMP_ERR_PAYLOAD); |
There was a problem hiding this comment.
According to RFC 792 up to 64 bytes of the original payload should be included. Since the header is separate, that is
64 - sizeof(ip header).
Edit: this is completely wrong. Misread the RFC. I clearly need more coffee.
Also, this may not be the correct way to handle this. It may be best to just ignore packets that have payloads less than 8 bytes of data, or it may be best to zero extend the data? Technically the packet generated by this algorithm could be shorter than expected, which IMO is really not good.
dlrobertson
force-pushed
the
improve_dst_unreachable
branch
from
8dd35b3
to
3a28298
Compare
src/iface/ethernet.rs
Outdated
| @@ -1,6 +1,7 @@ | |||
| // Heads up! Before working on this file you should read the parts | |||
| // of RFC 1122 that discuss Ethernet, ARP and IP. | |||
|
|
|||
| use core::cmp::min; | |||
There was a problem hiding this comment.
use core::cmp and then cmp::min(...).
src/iface/ethernet.rs
Outdated
| _ if eth_frame.dst_addr().is_unicast() && ipv4_repr.dst_addr.is_unicast() => { | ||
| // If the datagram does not have a payload greater than or equal | ||
| // to 64 bits, only send back the size of given payload. | ||
| let payload_len = min(ip_payload.len(), SIZEOF_ICMP_ERR_PAYLOAD); |
There was a problem hiding this comment.
Just push the entire payload there, up to MTU. It's more useful when debugging networking issues anyway, and it's free for us because we don't allocate.
src/iface/ethernet.rs
Outdated
| ttl: 64 | ||
| }; | ||
| Ok(Packet::Icmpv4(ipv4_reply_repr, icmp_reply_repr)) | ||
| if ipv4_repr.dst_addr.is_unicast() { |
There was a problem hiding this comment.
This code asks to be refactored into something like icmpv4_reply that would make the Ipv4Repr with the given Icmpv4Repr, and also have the condition for unicast addresses.
- Do not send ICMPv4 responses for packets with a broadcast destination
address.
- Do not send DstUnreachable with ProtoUnreachable on receipt of a
packet with an unknown protocol with a non-unicast destination
address.
- Do not send DstUnreachable with PortUnreachable on receipt of a
UDP packet when no sockets are listening on the destination port
and the destination address is a non-unicast address.
- Send the correct amount of the original datagram when sending Destination
Unreachable error responses.
- Do not assume that a ip datagram has a payload when sending a proto
unreachable ICMPv4 error response.
- Add tests to iface tests.
- Ensure ICMP error responses are correctly formed when the
datagram has no payload.
- Ensure ICMP error responses are correctly handled for UDP packets
when no socket is listening on the destination port.
- Ensure the correct amount of the original payload is returned in
Destination Unreachable responses.
dlrobertson
force-pushed
the
improve_dst_unreachable
branch
from
3a28298
to
af41a2d
Compare
|
Thanks! |
address.
packet with an unknown protocol with a non-unicast destination
address.
UDP packet when no sockets are listening on the destination port
and the destination address is a non-unicast address.
Unreachable error responses.
unreachable ICMPv4 error response.
datagram has no payload.
when no socket is listening on the destination port.
Destination Unreachable responses.
Related to: #66