Bug 277349 - The net.inet.ip.source_address_validation should ignore CARP IP in backup state
Summary: The net.inet.ip.source_address_validation should ignore CARP IP in backup state
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 14.0-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-net (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-27 11:11 UTC by Alexis Savin
Modified: 2024-03-28 19:36 UTC (History)
7 users (show)

See Also:


Attachments
patch to not treat BACKUP address as a local (794 bytes, patch)
2024-02-28 15:41 UTC, Gleb Smirnoff
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Savin 2024-02-27 11:11:45 UTC
The net source validation mechanism introduced in FreeBSD 14 (net.inet.ip.source_address_validation) which is enabled by default
is a good security enhancement, however, it should ignore CARP backup addresses.

The VIP address in a 'backup' state is not used for any traffic (on the backup carp node).
However, it's common to see such a backup node pull information from the active one,
using the VIP as a target and therefore receiving traffic from this VIP in the answer packets.

I have noticed two open tickets/discussions about this behavior:
* https://redmine.pfsense.org/issues/14026
* https://forum.netgate.com/topic/181163/strange-carp-behavioral-change-bug-in-ha-setup-after-upgrade-from-2-6-0-to-2-7-0

STR:

 Deploy two FreeBSD 14.0 Stable, configure carp on one interface of each node.
 Node A (Active) - 10.0.0.2/24
 Node B (Backup) - 10.0.0.3/24
 VIP - 10.0.0.1/24
 
 Ensure net.inet.ip.source_address_validation is set to 1.
 
 From Node B, ping the VIP 10.0.0.1. Observe you do not get answers.

 Disable net.inet.ip.source_address_validation, set it to 0.

 From Node B, ping the VIP 10.0.0.1. Observe you do now get answers.

Kindly appreciate feedback about this.
Comment 1 Gleb Smirnoff freebsd_committer freebsd_triage 2024-02-28 15:41:14 UTC
I think we had discussion on this some time ago. I suggested to make the
BACKUP IP address to be treated as non-local for all purposes except bind(2).

Here is a patch to try. I did not test it, but it gives an idea on what
am I talking about. It should change more that just source IP address
validation. The main question is whether there are any scenario that the
patch would break.
Comment 2 Gleb Smirnoff freebsd_committer freebsd_triage 2024-02-28 15:41:52 UTC
Created attachment 248813 [details]
patch to not treat BACKUP address as a local

Patch for review.
Comment 3 Alexis Savin 2024-02-29 15:38:07 UTC
Just tested the proposed patch, it seems to work pretty fine.

With this proposal, the CARP backup is able to ping the active
VIP on the active node. I have ran a few tests and did not see
specific regression or unexpected impact on network stack.

I look forward seeing this merged into the official kernel.

Many thanks for the quick feedback.
Comment 4 mikeg 2024-03-01 02:53:37 UTC
Ran into the same issue this afternoon ("Holy POLA Batman, why can't our redundancy pairs see the active partner after patching?").
Tested the attached patch and looks good: CARP backup can ping the VIP on master.

Didn't do exhaustive tests but the patch is simple enough I don't think there could be any unexpected consequences/regressions.
Comment 5 Gleb Smirnoff freebsd_committer freebsd_triage 2024-03-05 04:16:32 UTC
I'm not 100% sure there would be no unexpected consequences/regressions.
The function modified isn't used by CARP only.  I need to think more on
that.  I'd appreciate more eyes looking at code and evaluating the idea.

For the reference I've found link to the discussion we had on that before,
it is on Telegram, messages from https://t.me/freebsd_ru/482821 down to
https://t.me/freebsd_ru/482859.  The TLDR summary in English would be
"seems like a good idea, but not one is 100% sure".
Comment 6 Zhenlei Huang freebsd_committer freebsd_triage 2024-03-08 03:54:14 UTC
(In reply to Gleb Smirnoff from comment #1)
> I think we had discussion on this some time ago. I suggested to make the
> BACKUP IP address to be treated as non-local for all purposes except bind(2).
Generally good. And, the bind socket should not accept any incoming connections even they are locally originated.

> Here is a patch to try. I did not test it, but it gives an idea on what
> am I talking about. It should change more that just source IP address
> validation. The main question is whether there are any scenario that the
> patch would break.

The change seems good if we treat CARP BACKUP IP addresses as NOT **local**.
Comment 7 commit-hook freebsd_committer freebsd_triage 2024-03-19 18:58:12 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=56f7860087eec14b4a65310b70bd704e79e1b48c

commit 56f7860087eec14b4a65310b70bd704e79e1b48c
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2024-03-19 18:48:59 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-03-19 18:48:59 +0000

    carp: check CARP status in in_localip_fib(), in6_localip_fib()

    Don't report a BACKUP CARP address as local.  These two functions are used
    only by source address validation for input packets, controlled by sysctls
    net.inet.ip.source_address_validation and
    net.inet6.ip6.source_address_validation.  For this purpose we definitely
    want to treat BACKUP addresses as non local.

    This change is conservative and doesn't modify compat in_localip() and
    in6_localip().  They are used more widely than the FIB-aware versions.
    The change would modify the notion of ipfw(4) 'me' keyword.  There might
    be other consequences as in_localip() is used by various tunneling
    protocols.

    PR:                     277349

 sys/netinet/in.c   | 4 +++-
 sys/netinet6/in6.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)
Comment 8 mickael.maillot 2024-03-21 13:07:55 UTC
this fix will be merged in stable/14 ?
Comment 9 Gleb Smirnoff freebsd_committer freebsd_triage 2024-03-21 17:11:17 UTC
On Thu Mar 21 13:07:55  2024 UTC, mickael.maillot@gmail.com wrote:
> this fix will be merged in stable/14 ?

I plan to merge next week.
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-03-28 19:36:51 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d6e1ae659b11a13a9c289424735394173907c1d3

commit d6e1ae659b11a13a9c289424735394173907c1d3
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2024-03-19 18:48:59 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-03-28 19:35:45 +0000

    carp: check CARP status in in_localip_fib(), in6_localip_fib()

    Don't report a BACKUP CARP address as local.  These two functions are used
    only by source address validation for input packets, controlled by sysctls
    net.inet.ip.source_address_validation and
    net.inet6.ip6.source_address_validation.  For this purpose we definitely
    want to treat BACKUP addresses as non local.

    This change is conservative and doesn't modify compat in_localip() and
    in6_localip().  They are used more widely than the FIB-aware versions.
    The change would modify the notion of ipfw(4) 'me' keyword.  There might
    be other consequences as in_localip() is used by various tunneling
    protocols.

    PR:                     277349
    (cherry picked from commit 56f7860087eec14b4a65310b70bd704e79e1b48c)

 sys/netinet/in.c   | 4 +++-
 sys/netinet6/in6.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)