Skip to content

gnrc_icmpv6_error / gnrc_ipv6: fixes for unspecified address#10421

Merged
bergzand merged 3 commits intoRIOT-OS:masterfrom
miri64:gnrc_icmpv6_error/bug/dont-send-to-unspec
Nov 17, 2018
Merged

gnrc_icmpv6_error / gnrc_ipv6: fixes for unspecified address#10421
bergzand merged 3 commits intoRIOT-OS:masterfrom
miri64:gnrc_icmpv6_error/bug/dont-send-to-unspec

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Nov 17, 2018

Contribution description

This fixes the two bugs regarding the unspecified address I described in #10419 (comment)

  1. Don't build and send ICMPv6 error messages destined to the unspecified address (::).
  2. Don't send (and try to resolve) IPv6 messages addressed to the unspecified address.

Testing procedure

Repeat the steps to reproduce in #10419. The expected results (except for probing neighbor solicitations which are legitimate) should come to be.

Issues/PRs references

Fixes #10419.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 17, 2018
@miri64 miri64 added this to the Release 2019.01 milestone Nov 17, 2018
@miri64 miri64 requested a review from bergzand November 17, 2018 10:37
bergzand
bergzand requested changes Nov 17, 2018
View reviewed changes
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Note to self, compile test code from yesterday night, even if you think you did...

@bergzand bergzand mentioned this pull request Nov 17, 2018
Merged
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

Wit this PR, I'm unable to trigger a Destination unreachable from a native instance by sending an UDP frame :/

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

I don't understand your ":/". Isn't this a good thing? Or do you mean something else than described in #10419?

bergzand
bergzand reviewed Nov 17, 2018
View reviewed changes
sys/net/gnrc/network_layer/icmpv6/error/gnrc_icmpv6_error.c Outdated
if (ipv6_addr_is_unspecified(&ipv6_hdr->src)) {
ipv6 = NULL;
}
return NULL;
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ipv6 instead of NULL?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can only quote myself here ...

Note to self, compile test code from yesterday night, even if you think you did...

Copy link
Copy Markdown
Member Author

@miri64 miri64 Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

I don't understand your ":/". Isn't this a good thing? Or do you mean something else than described in #10419?

What I was trying to say is that I don't get a Destination Unreachable response anymore for "valid" cases where I expect to receive a Destination Unreachable from the instance, so for example with:

srp1( Ether(dst=DST_HWADDR) / IPv6(dst=DST_ADDR, src=SRC_ADDR)  / UDP(dport=1337), iface="tapbr0")

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Yepp... Fixed that.

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

I was also wondering if multicast addresses should be filtered out. Is it valid to respond with an ICMPv6Error to a frame with a multicast address as source (and transmit the error response to a multicast address)

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

To expand my previous comment, should I receive an error response from a frame like this? Assuming there is nothing listening on UDP port 1337 on the instance. And should the error then have the multicast address as destination

srp1( Ether(dst=DST_HWADDR) / IPv6(dst=DST_ADDR, src="ff02::1")  / UDP(dport=1337), iface="tapbr0")

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

To expand my previous comment, should I receive an error response from a frame like this? Assuming there is nothing listening on UDP port 1337 on the instance.

Yes of course there should also be an error message.

And should the error then have the multicast address as destination[?]

No why? We send the packet back as always to the source of the IPv6 packet and that's the only interested party.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Ahh... I get what you mean now. You are right! Multicast should be filtered out as well, but let me check if gnrc_ipv6 not already does that ;-)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Fixed and also amended the new behavior to the documentation.

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

Fixed and also amended the new behavior to the documentation.

Thanks!

ACK, please squash!

@miri64 miri64 force-pushed the gnrc_icmpv6_error/bug/dont-send-to-unspec branch from 8837860 to b37d43c Compare November 17, 2018 14:37
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Squashed and rebased

@miri64
gnrc_icmpv6_error: check IPv6 header before starting to build
ef11323 
Check for:

 - if it exists (critical error condition -- non-IPv6 headers should
   not trigger these functions) => assert
 - if it has a multicast source (that shouldn't really happen but
   people might try weird stuff ;-)
 - if it has an unspecified source (can't determine receiver of error
   message => don't send it, don't build it)
@miri64
gnrc_icmpv6_error: fix doc alignment
0aa96a4
@miri64
gnrc_ipv6: drop packets with unspecified destination
0bdbb68 
It just doesn't makes sense to handle them any further
@miri64 miri64 force-pushed the gnrc_icmpv6_error/bug/dont-send-to-unspec branch from b37d43c to 0bdbb68 Compare November 17, 2018 14:45
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018 •
edited
Loading

Fixed and force-pushed squashed error pointed out by cppcheck.

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

👍 on the force push

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 17, 2018

Murdock's happy :-)

@bergzand bergzand merged commit c755071 into RIOT-OS:master Nov 17, 2018
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 17, 2018

And go!

@miri64 miri64 deleted the gnrc_icmpv6_error/bug/dont-send-to-unspec branch November 17, 2018 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand approved these changes

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

gnrc_icmpv6_error: Able to bounce up to 64 ICMPv6 error messages between 2 instances

2 participants

@miri64 @bergzand