-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not send ICMPv4 responses to broadcasts #67
Do not send ICMPv4 responses to broadcasts #67
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Previous comment was invalid. Misread the RFC. Updated to 8 bytes and 64 bits... 😨
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use core::cmp
and then cmp::min(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree
- 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.
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