networkd: RFC compliant autonomous prefix handling by hendrikw01 · Pull Request #5636 · systemd/systemd

hendrikw01 · 2017-03-25T12:11:05Z

Previously, lifetime_valid of a Router Advertisement was not handled the way RFC4862 has specified.
In particular: Sections 5.5.3.d and 5.5.3.e

This pull request fixes: #3879 and #5638

heftig · 2017-03-25T18:05:01Z

No, I think this is still wrong. If I understand section 5.5.3.e correctly, an incoming advertisement with preferred lifetime and valid lifetime as zero for a prefix that is already known should:

Immediately deprecate the address
Keep the address for 2 more hours, unless the advertisement has been authenticated

hendrikw01 · 2017-03-25T20:03:18Z

Yes, the router sends the first RA after reconnect with:
ValidLifetime 7199 and referredLifetime 0 for the old prefix.
networkd/kernel then flags the address as deprecated and sets a lifetime of 2 hours. This behaviour is exactly right but then the router starts to send ValidLifetime 0 for that already deprecated prefix.

hendrikw01 · 2017-03-25T20:39:38Z

This bug can be used as a denial-of-service attack.
I have tested to send an RA with radvd:

prefix 2001:db8:1:0::/64
        {
                AdvOnLink on;
                AdvAutonomous on;
                AdvRouterAddr off;
                AdvPreferredLifetime 0;
                AdvValidLifetime 0;
         };

Only this RA is needed to force the network interface of all systemd-networkd nodes in the local network to go into the fail state and then looses all ipv4/6 addresses after they expire.

Mär 25 21:31:04 bpi systemd-networkd[210]: NDISC: Received Router Advertisement: flags none preference low lifetime 30 sec
Mär 25 21:31:04 bpi systemd-networkd[210]: NDISC: Invoking callback for 'r'.
Mär 25 21:31:04 bpi systemd-networkd[210]: eth0: Could not set NDisc route or address: Invalid argument
Mär 25 21:31:04 bpi systemd-networkd[210]: eth0: Failed

hendrikw01 · 2017-03-27T06:56:10Z

I have created a simple test suite to validate the correct behavior using radvd:
https://github.com/hendrikw01/ra_lifetime_test

My implementation does work as expected.

keszybz

So this looks OK, as long as the two patches are squashed together, and the stylistic issues are fixed.

@pfl can you take a look?

keszybz · 2017-03-27T00:19:56Z

src/network/networkd-ndisc.c

+
+        /* see RFC4862 section 5.5.3.e */
+        r = address_get(link, address->family, &address->in_addr, address->prefixlen, &existing_address);
+        if(r > 0){


See CODING_STYLE: space before ( and after ).

This patch uses 4ch indentation, instead of 8ch, but it is updated in the second patch. It seems that they should be squashed together.

keszybz · 2017-03-30T01:58:31Z

src/network/networkd-ndisc.c


 static void ndisc_router_process_autonomous_prefix(Link *link, sd_ndisc_router *rt) {
        _cleanup_address_free_ Address *address = NULL;
+        Address *existing_address = NULL;


Initialization is not needed.

keszybz · 2017-03-30T02:27:40Z

src/network/networkd-ndisc.c

+            else if(existing_address->cinfo.ifa_valid <= NDISC_PREFIX_LFT_MIN)
+                address->cinfo.ifa_valid = existing_address->cinfo.ifa_valid;
+            else
+                address->cinfo.ifa_valid = NDISC_PREFIX_LFT_MIN;


So this first branch of the outer if follows the RFC word for word. Looks good.

keszybz · 2017-03-30T02:37:38Z

src/network/networkd-ndisc.c


 #define NDISC_DNSSL_MAX 64U
 #define NDISC_RDNSS_MAX 64U
+#define NDISC_PREFIX_LFT_MIN 7200


Please add U like in other constants.

keszybz · 2017-03-30T02:49:25Z

src/network/networkd-ndisc.c

+                else if(lifetime_remaining <= NDISC_PREFIX_LFT_MIN)
+                        address->cinfo.ifa_valid = lifetime_remaining;
+                else
+                        address->cinfo.ifa_valid = NDISC_PREFIX_LFT_MIN;


I made a comment here, but it seems github lost it: this follows the RFC very closely, should be correct.

pfl · 2017-03-30T05:57:43Z

src/network/networkd-ndisc.c

+        r = address_get(link, address->family, &address->in_addr, address->prefixlen, &existing_address);
+        if(r > 0){
+            if(lifetime_valid > NDISC_PREFIX_LFT_MIN || lifetime_valid > existing_address->cinfo.ifa_valid)
+                address->cinfo.ifa_valid = lifetime_valid;


This one is clear and from the RFC. ACK.

pfl · 2017-03-30T06:00:04Z

src/network/networkd-ndisc.c

+            if(lifetime_valid > NDISC_PREFIX_LFT_MIN || lifetime_valid > existing_address->cinfo.ifa_valid)
+                address->cinfo.ifa_valid = lifetime_valid;
+            else if(existing_address->cinfo.ifa_valid <= NDISC_PREFIX_LFT_MIN)
+                address->cinfo.ifa_valid = existing_address->cinfo.ifa_valid;


ACK. We don't have secured Router Advertisments so the bottom half of section 5.5.3, part e), 2nd bullet does not apply.

pfl · 2017-03-30T06:03:26Z

src/network/networkd-ndisc.c

+            else if(existing_address->cinfo.ifa_valid <= NDISC_PREFIX_LFT_MIN)
+                address->cinfo.ifa_valid = existing_address->cinfo.ifa_valid;
+            else
+                address->cinfo.ifa_valid = NDISC_PREFIX_LFT_MIN;


pfl · 2017-03-30T06:14:07Z

src/network/networkd-ndisc.c

+            else
+                address->cinfo.ifa_valid = NDISC_PREFIX_LFT_MIN;
+        }else if(lifetime_valid > 0)
+            address->cinfo.ifa_valid = lifetime_valid;


This happens when r = address_get() < 0, right? Shouldn't this also check if lifetime_valid > NDISC_PREFIX_LFT_MIN like above?

I thought about this too, and in the end that seems to be the same as bullet 1, i.e. having the address invalidated because remaining lifetime is <= 0. In that case this assignment would be correct. Let's see what @hendrikw01 says.

Thanks for the review!

lifetime_valid doesn´t need to be 2 hours or longer. That means the router can add a prefix/address with a lifetime of 30 secs and renew it every 10 secs. This behavior is totally fine but a bogus RA can´t force it to invalidate faster. If we got new prefix (address_get() < 0) we can´t decide if this is a legit Prefix or not, so the only option is to assume it is, and if it is the router updates the prefix before it gets invalid.

hendrikw01 · 2017-03-30T15:17:53Z

Now, I think I have all issues covered. From my side, it can be merged.

heftig · 2017-03-30T15:27:01Z

I think you're supposed to squash your fixups, not just push more commits.

Previously, `lifetime_valid` of a Router Advertisement was not handled the way RFC4862 has specified. In particular: Sections 5.5.3.d and 5.5.3.e

hendrikw01 · 2017-03-31T02:11:52Z

Sorry, first time for me to squash something in git.

hendrikw01 mentioned this pull request Mar 25, 2017

networkd ndisc: Bug can be used for a denial-of-service attack #5638

Closed

2 tasks

hendrikw01 closed this Mar 26, 2017

hendrikw01 force-pushed the master branch from dcbdaac to 1539a65 Compare March 26, 2017 21:37

hendrikw01 reopened this Mar 26, 2017

hendrikw01 changed the title ~~networkd: remove ipv6 address if needed~~ networkd: RFC compliant autonomous prefix handling Mar 26, 2017

hendrikw01 mentioned this pull request Mar 27, 2017

systemd-networkd won't renew DHCPv4 address after NDisc error #5625

Closed

2 tasks

keszybz reviewed Mar 30, 2017

View reviewed changes

keszybz requested a review from pfl March 30, 2017 02:49

pfl reviewed Mar 30, 2017

View reviewed changes

networkd: RFC compliant autonomous prefix handling

a1f52a8

Previously, `lifetime_valid` of a Router Advertisement was not handled the way RFC4862 has specified. In particular: Sections 5.5.3.d and 5.5.3.e

hendrikw01 force-pushed the master branch from acc7c5a to a1f52a8 Compare March 31, 2017 02:07

poettering added ndisc/ra network labels Mar 31, 2017

poettering merged commit 6554550 into systemd:master Mar 31, 2017

Uh oh!

Conversation

hendrikw01 commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heftig commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hendrikw01 commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hendrikw01 commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hendrikw01 commented Mar 27, 2017

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikw01 Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikw01 commented Mar 30, 2017

Uh oh!

heftig commented Mar 30, 2017

Uh oh!

hendrikw01 commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

hendrikw01 commented Mar 25, 2017 •

edited

Loading

heftig commented Mar 25, 2017 •

edited

Loading

hendrikw01 commented Mar 25, 2017 •

edited

Loading

hendrikw01 commented Mar 25, 2017 •

edited

Loading

hendrikw01 Mar 30, 2017 •

edited

Loading