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

Do not drop invalid packets by default #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bastelfreak
Copy link
Member

This could be considered a breaking change. I don't mind keeping it open until the next major release.

@bastelfreak bastelfreak self-assigned this Dec 20, 2023
@duritong
Copy link
Collaborator

Can you elaborate, why you want to change the default? I think it's pretty standard to drop invalid packets, not?

@bastelfreak
Copy link
Member Author

I doesn't make sense to have that rule when the default policy is already drop. It's redundant. In addition to that there were bug reports in the past where ceph traffic was marked as invalid for whatever reasons and firewalls dropped it by accident. Same applies for Qemu hypervisors with bridges.

@duritong
Copy link
Collaborator

It is not redundant, since the invalid drop comes before the jump into the default chain.

Which essentially means, that invalid packets of (later) allowed connections are dropped (as you observed). If you are relaying on the default deny for that, then it won't work since the invalid packets matched the (now) previous allowed rule and thus the firewall allows invalid packets.

But that is the whole idea: A firewall should also filter out packets that are not part of an established or related connection (stateful filtering) and secondly it should also filter out any other malicious traffic like invalid packets.

I am fine with making this configurable, but changing the default would definitely weaken the default configuration of the module and thus making it less secure out of the box.

As I said I think it is pretty standard for every firewall to filter out invalid packets by default (https://wiki.nftables.org/wiki-nftables/index.php/Simple_ruleset_for_a_server) and therefor I think it should be in there by default.

What you must be observing is that applications do send invalid new packets, since any other kind of invalid packet would still be passed by the established&related rule (which is before dropping invalid packets), which is quite weird to be honest, but there might be reasons for it, but this does cannot be an argumentation for an overall weakening of the default configuration of the configuration generated by this module.

Correct me if I got any of my understanding wrong, but I think your assumption of the default policy is wrong and thus it works differently than you currently assume and thus argue for the change.

@bastelfreak bastelfreak force-pushed the nftables branch 7 times, most recently from 301bc22 to 29137ea Compare December 27, 2023 11:02
@JohnHolmesII
Copy link

I would also ask that the default not be changed. Configurable should be good enough.

@amuckart
Copy link

amuckart commented Apr 17, 2024

I don't think this is a good idea for a default setting, and I think @duritong's comments are spot on.

Dropping invalid packets is a pretty standard expected behaviour for most firewalls. Changing the default behaviour to fix a handful of misbehaving protocols seems like it will open more dangerous cases than some protocol not working.

The best case is the invalid packet hits the destination host and the destination host drops it. The worst case is that the invalid packet triggers an explot on the destination host and now you've got a compromised network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants