Page MenuHomeFreeBSD

Do not forward datagrams original from 169.254.0.0/16 link-local addresses
ClosedPublic

Authored by zlei.huang_gmail.com on Apr 25 2021, 9:38 AM.

Details

Summary

The current implement of ip_input() reject packets destined for 169.254.0.0/16, but not those original from 169.254.0.0/16 link-local addresses.

Fix to fully respect RFC 3927 section 2.7.

PR: 255388

Test Plan

Set up an instance net.inet.ip.forwarding=1, input packets with source address 169.254.0.0/16 , netstat -s -p ip should show increasing packets not forwardable

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Moving the code down fixes a couple of problems, but it introduces a new one: multicasts from link-locals can be forwarded if we are a multicast router. Maybe add a separate check in that section? We should still receive such multicasts.

Moving the code down fixes a couple of problems, but it introduces a new one: multicasts from link-locals can be forwarded if we are a multicast router. Maybe add a separate check in that section? We should still receive such multicasts.

Good catch.

RFC 3927 2.7: Do not forward IPv4 packets with a Link-Local source address even if they have a multicast destination address.

May you please add context to the diff?

sys/netinet/ip_input.c
750–751

If the source in not link local, then check for mrouter and drop it enventually.
So if the source is link local, forward it in any case?

May you please add context to the diff?

This is my first differential, and I'm not familiar with Phabricator.
I uploaded the diff from git diff main. How to add context to the diff ?
Thanks.

Got it. I'll add full context.

[1] https://wiki.freebsd.org/Phabricator

zlei.huang_gmail.com edited the summary of this revision. (Show Details)

Added full context.

sys/netinet/ip_input.c
769

It seems wrong that ips_forward is increased unconditionally. Should increase only when ip_mforward() succeed?

sys/netinet/ip_input.c
750–751

If the source is link local, it is for ours.

donner requested changes to this revision.Apr 26 2021, 8:35 AM
donner added inline comments.
sys/netinet/ip_input.c
750–751

Then the condition belongs to line 740

if (V_ip_mrouter && !IN_LINKLOCAL{(ntohl(ip->ip_src.s_addr))) {
This revision now requires changes to proceed.Apr 26 2021, 8:35 AM

Then the condition belongs to line 740
if (V_ip_mrouter && !IN_LINKLOCAL{(ntohl(ip->ip_src.s_addr))) {

Update as @donner suggested.

So we are currently handle all multicast packets from LL sources as locally handled streams regardless of the destination.
That might be correct, even if the local machine is processing the multicast destination itself. So it is able to receive such stream over the local link.

This revision is now accepted and ready to land.Apr 26 2021, 1:12 PM
rgrimes added inline comments.
sys/netinet/ip_input.c
743–744

Remove RFC section ref s/datagrams/packets/ add "from":

/* Do not forward packets from IN_LINKLOCAL */

790

Extra blank line sould be removed

790

Three issues use of "datagrams", the other code uses "packets" and this use of the word "for" to describe to and from and RFC's change get/revised I do not see a need for specific number and secton reference here.

I suggest a clearer:

/* Do not forward packets to or from IN_LINKLOCAL */

This revision now requires review to proceed.Apr 26 2021, 4:19 PM

I would still like to see the "169.254.0.0/16" changed to IN_LINKLOCAL, purpose of macro is to locate this value one place and one place only, scattering this string in the code undoes that.

I would still like to see the "169.254.0.0/16" changed to IN_LINKLOCAL, purpose of macro is to locate this value one place and one place only, scattering this string in the code undoes that.

Sounds reasonable. Sorry I misunderstood the IN_LINKLOCAL.

I am accepting this without the IPSTAT_INC(ips_forward) issue being fixed, as it looks to me as if that is an existing and separate problem in the code. Probably a walk through should be done to see that ips_forward and ips_cantforward are all done correctly.

sys/netinet/ip_input.c
769

Looks like this still needs chased down. I tried to view full context in phabricator, but it totally messes up the screen layout and I can not read the code at all. I do agree that from what I can see this IPSTAT_INC looks wrong here.

798

Maybe here is where the IPSTAT_INC(ips_forward) belongs?

This revision is now accepted and ready to land.Apr 27 2021, 11:30 AM
zlei.huang_gmail.com added inline comments.
sys/netinet/ip_input.c
798

No. There's already IPSTAT_INC(ips_forward) in ip_forward() at line 1078 .
The above IPSTAT_INC(ips_forward) at line 763 is solely for ip_mforward() if I understand correctly.

sys/netinet/ip_input.c
783

Q: given it's pretty neat /16, which splits an 32-bit address in two halves, would it be possible to come up with a macro that operates on network address, without doing ntohl()?

Re: avoiding ntohl(): that would be possible, but probably not within the scope of this change.

Friendly ping @donner , there're changes since your first acceptance, do you have any objections?

Hi, since I do not have access to the src repository, could someone check in the diff please ?