bind(2): Lookup local address in current FIB if '*.bind_all_fibs' is active

[bnovkov]

Summary

When a protocol-specific 'bind_all_fibs' tunable is set to 0, a
listening socket will only receive traffic originating from the FIB
it was bound to. However, there are no checks to determine whether
an address exists in the target FIB when binding the socket, which can
lead to a situation where a socket and the address it was bound to
belong to different FIBs.

Prevent this footgun by looking up the requested address in the current
FIB if 'bind_all_fibs' is active and returning an error if the address
does not exist.

Sponsored by: Stormshield
Sponsored by: Klara, Inc.

[markj]

sys/netinet/raw_ip.c

889

Hum, do we need to be in the network epoch in order to look up the nexthop? fib4_lookup() doesn't assert this, but has a comment suggesting that it's needed.

890

Indentation here is wonky.

sys/netinet6/raw_ip6.c

781

Indentation here is wonky.

[glebius]

IMHO, the check shall be located in in_pcbbind_avail() in the block that has the comment * Is the address a local IP address?. Instead of calling fib4_lookup() that has a different purpose, we need ifa_ifwithaddr() version that has fibnum argument. Many other functions in the ifa_if* family already have it. The IPv6 version is similar.

[bnovkov]

In D58281#1336697, @glebius wrote:

Instead of calling fib4_lookup() that has a different purpose, we need ifa_ifwithaddr() version that has fibnum argument.

Looking through net/if.c I noticed that ifa_ifwithnet seems to match what you're suggesting here, do you think we can use this function instead of adding a fibnum-aware version of `ifa_ifwithaddr()`?

[pouria]

In D58281#1336902, @bnovkov wrote:

In D58281#1336697, @glebius wrote:

Instead of calling fib4_lookup() that has a different purpose, we need ifa_ifwithaddr() version that has fibnum argument.

Looking through net/if.c I noticed that ifa_ifwithnet seems to match what you're suggesting here, do you think we can use this function instead of adding a fibnum-aware version of `ifa_ifwithaddr()`?

I'll create a fib-aware version of ifa_ifwithaddr() and will let you know.

[bnovkov]

In D58281#1337090, @pouria wrote:

In D58281#1336902, @bnovkov wrote:

In D58281#1336697, @glebius wrote:

Instead of calling fib4_lookup() that has a different purpose, we need ifa_ifwithaddr() version that has fibnum argument.

Looking through net/if.c I noticed that ifa_ifwithnet seems to match what you're suggesting here, do you think we can use this function instead of adding a fibnum-aware version of `ifa_ifwithaddr()`?

I'll create a fib-aware version of ifa_ifwithaddr() and will let you know.

Thank you for the offer, but I was trying to avoid unnecessary churn if the same thing can be achieved with ifa_ifwithnet. If you have future uses for a fib-aware version of ifa_ifwithaddr() I can implement it as well.

[glebius]

In D58281#1337102, @bnovkov wrote:

Thank you for the offer, but I was trying to avoid unnecessary churn if the same thing can be achieved with ifa_ifwithnet. If you have future uses for a fib-aware version of ifa_ifwithaddr() I can implement it as well.

There will be less churn if you use standard KPI in the right place instead of current version. Pouria also needs this function. Even if he didn't, still creating this KPI on the first need is the right thing to do.

P.S. Also, if I read current version right, it checks that IP address is reachable rather than local and that's not what we want.

[bnovkov]

In D58281#1337131, @glebius wrote:

In D58281#1337102, @bnovkov wrote:

Thank you for the offer, but I was trying to avoid unnecessary churn if the same thing can be achieved with ifa_ifwithnet. If you have future uses for a fib-aware version of ifa_ifwithaddr() I can implement it as well.

There will be less churn if you use standard KPI in the right place instead of current version. Pouria also needs this function. Even if he didn't, still creating this KPI on the first need is the right thing to do.

Thank you for the clarification, I will address the KPI and your other comments on Monday.

[bnovkov]

Address @glebius 's comments

[pouria]

LGTM

[glebius]

The raw and netinet6 versions look correct, but IPv4 is slightly weird. Comments above.

sys/netinet/in_pcb.c

927

We already are in a block that starts with if (!in_nullhost(laddr)) which is correct writing for laddr.s_addr != INADDR_ANY.

928

You just need to use ifa_ifwithaddr_fib_check() here and don't call it twice.

[bnovkov]

Address @glebius 's comments