Disable ICMP (v4) redirects by default

[emaste]

Summary

Relnotes: Yes

[emaste]

Based on discussion on a recent secteam call. After putting this together I discovered D23329, which provides an rc.conf setting defaulting to AUTO which is set to yes (drop) if a routing daemon is enabled, and no if not - so if we do want to make this change we'll want to update rc.d/routing as well.

CC @donner and @melifaro.

[glebius]

I agree that this default is a long due to be changed. Needs to be mentioned in Release Notes, though.

[emaste]

switch userland default as well

[emaste]

libexec/rc/rc.d/routing

341

I don't think there's an issue with just changing the var itself from AUTO to YES (i.e., avoiding the underscore-prefixed dance)

[ceri]

libexec/rc/rc.d/routing

340

Is the |”” still appropriate?
It seems to have the wrong default behaviour if this is somehow unset.

[emaste]

libexec/rc/rc.d/routing

340

Default is now yes, so choosing yes if unset seems appropriate?

[ceri]

libexec/rc/rc.d/routing

340

A very fine point; was reading backwards.

[rgrimes]

libexec/rc/rc.d/routing

341

For systems doing "updates" just switching auto to yes *may* break some installations.

[gordon]

libexec/rc/rc.d/routing

341

That's kind of the point here. At some point we have to break some eggs. We will doing this for 15 and not intending to MFC. It will be a release note item.

346

Should we print this on the yes case given it is now the default?

[emaste]

libexec/rc/rc.d/routing

346

Hrm, good question. This is one of the unfortunate side effects of negative-sense sysctls; we print a message in all of the "= 1" cases so there's some argument for keeping that for consistency. We could instead print ignore ICMP redirect=NO in the no case I suppose.

[zarychtam_plan-b.pwste.edu.pl]

We probably need a similar change for IPv6 redirects.

[emaste]

libexec/rc/rc.d/routing

341

Also note that redirects are a performance optimization, if a system changes to yes after upgrade it won't "break" in the sense of network unreachability.

[emaste]

ICMP6:

VNET_DEFINE_STATIC(int, icmp6_rediraccept) = 1;
#define V_icmp6_rediraccept     VNET(icmp6_rediraccept)
SYSCTL_INT(_net_inet6_icmp6, ICMPV6CTL_REDIRACCEPT, rediraccept,
    CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(icmp6_rediraccept), 0,
    "Accept ICMPv6 redirect messages");

It appears there's no rc.conf machinery to configure this though?

[zlei]

A long long time ago ( I was a student then ), I enabled drop_redirect on one of my VM, but the router ( out of my control ) keep sending ICMP redirects. That confused me for quite a long time until I figured out that is perfect legitimate for routers to do that.

I meant, if the industry encourage disabling sending ICMP redirects on routers, then it is good time to drop ICMP redirects on a host, for security reason.

[markj]

Is there a reason we shouldn't go ahead with this?

[pouria]

In D45102#1366629, @markj wrote:

Is there a reason we shouldn't go ahead with this?

+1

[philip]

I'm surprised this wasn't already done. :)

In practice everyone drops redirects at the edges but it's good hygiene to drop them on the host too.

[emaste]

(Discussed on the mailing list at https://lists.freebsd.org/archives/freebsd-net/2024-May/004920.html)

[bcr]

Ok from manpages.

[markj]

In D45102#1366764, @emaste wrote:

(Discussed on the mailing list at https://lists.freebsd.org/archives/freebsd-net/2024-May/004920.html)

Regarding v6, ICMP6 redirects are validated more than v4 redirects: they are ignored when forwarding is enabled, they need to come from a link-local addr and they need to come from the current next-hop gateway. It's not obvious to me that we want to drop v6 redirects by default.

[markj]

In D45102#1366777, @markj wrote:

In D45102#1366764, @emaste wrote:

(Discussed on the mailing list at https://lists.freebsd.org/archives/freebsd-net/2024-May/004920.html)

Regarding v6, ICMP6 redirects are validated more than v4 redirects: they are ignored when forwarding is enabled, they need to come from a link-local addr and they need to come from the current next-hop gateway. It's not obvious to me that we want to drop v6 redirects by default.

... we should probably make it more clear in the man page that this applies to v4 only.

[glebius]

While here., the variable and sysctl can be changed to bool.