Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firewall: Settings: Normalization - Default behavior and best practices. #7203

Closed
2 tasks done
AdSchellevis opened this issue Feb 5, 2024 · 0 comments
Closed
2 tasks done
Assignees
Labels
cleanup Low impact changes
Milestone

Comments

@AdSchellevis
Copy link
Member

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Is your feature request related to a problem? Please describe.

Currently our default normalization practice is to add scrub to most interfaces, but not all. Which in some cases can lead to unexpected behavior as some virtual types (like enc0) are excluded, in which case fragment reassembly may be omitted (which is enabled by default when using scrub according to https://man.freebsd.org/cgi/man.cgi?pf.conf(5)).

The question is if the current "micro management" approach actually makes sense, as most examples start with scrub in all and the previous openbsd manual states "Other than these somewhat unusual cases, scrubbing all packets is highly recommended practice. "

When specific normalization rules are needed, in OPNsense it is already possible to define these first and optionally disable all automatic scrubbing.

Describe the solution you like

Refactor the current per interface option to always end with scrub in all when default scrubbing is not disabled, add the no-df and random-id when being asked (likely hardly used) and add the interface mss clamping rules before the generic scrub rule.

Point of reference:

core/src/etc/inc/filter.inc

Lines 564 to 589 in c7d6f53

if (empty($config['system']['scrub_interface_disable'])) {
foreach ($FilterIflist as $scrubcfg) {
if (isset($scrubcfg['virtual']) || empty($scrubcfg['descr'])) {
continue;
}
$mssclampv4 = '';
$mssclampv6 = '';
if (
!empty($scrubcfg['mss']) && is_numeric($scrubcfg['mss']) &&
!in_array($scrubcfg['if'], array('pppoe', 'pptp', 'l2tp'))
) {
$mssclampv4 = 'max-mss ' . (intval($scrubcfg['mss'] - 40));
$mssclampv6 = 'max-mss ' . (intval($scrubcfg['mss'] - 60));
}
$scrubnodf = !empty($config['system']['scrubnodf']) ? 'no-df' : '';
$scrubrnid = !empty($config['system']['scrubrnid']) ? 'random-id' : '';
if (!empty($mssclampv4)) {
$scrubrules .= "scrub on {$scrubcfg['if']} inet all {$scrubnodf} {$scrubrnid} {$mssclampv4}\n";
$scrubrules .= "scrub on {$scrubcfg['if']} inet6 all {$scrubnodf} {$scrubrnid} {$mssclampv6}\n";
} else {
$scrubrules .= "scrub on {$scrubcfg['if']} all {$scrubnodf} {$scrubrnid}\n";
}
}
}

At first sight there don't seem to be downsides for not scrubbing traffic on outbound traffic, in which case it's likely a good idea to only add in by default.

Describe alternatives you considered

Leaving as is, knowing some vague reassemble issues will popup at some point for existing or new virtual interface types.

Additional context

@AdSchellevis AdSchellevis added the cleanup Low impact changes label Feb 5, 2024
@AdSchellevis AdSchellevis self-assigned this Feb 5, 2024
@AdSchellevis AdSchellevis added this to the 24.7 milestone Feb 5, 2024
fichtner pushed a commit that referenced this issue Feb 13, 2024
…ion behavior and choose "in" as standard direction for manual rules. closes #7203

(cherry picked from commit 630ab19)
fichtner pushed a commit that referenced this issue Mar 12, 2024
…ion behavior and choose "in" as standard direction for manual rules. closes #7203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Low impact changes
Development

No branches or pull requests

1 participant