Page MenuHomeVyOS Platform

Firewall incorrect handler for recent count and time
Closed, ResolvedPublicBUG

Description

To reproduce:

set firewall name FOO default-action 'reject'
set firewall name FOO rule 10 action 'accept'
set firewall name FOO rule 10 state established 'enable'
set firewall name FOO rule 10 state related 'enable'
set firewall name FOO rule 20 action 'accept'
set firewall name FOO rule 20 protocol 'icmp'
set firewall name FOO rule 20 state new 'enable'
set firewall name FOO rule 30 action 'reject'
set firewall name FOO rule 30 destination port '22'
set firewall name FOO rule 30 protocol 'tcp'
set firewall name FOO rule 30 recent count '4'
set firewall name FOO rule 30 recent time '60'
set firewall name FOO rule 40 action 'accept'
set firewall name FOO rule 40 destination port '22'
set firewall name FOO rule 40 protocol 'tcp'
set firewall name FOO rule 40 state new 'enable'
set interfaces ethernet eth0 firewall local name 'FOO'

Generated rules:

	chain FOO {
		ct state { 0x2, 0x4 } counter packets 580 bytes 45488 return comment "FOO-10"
		ct state { 0x8 } meta l4proto 1 counter packets 0 bytes 0 return comment "FOO-20"
		tcp dport { 22 } counter packets 9 bytes 540 reject comment "FOO-30"
		ct state { 0x8 } tcp dport { 22 } counter packets 0 bytes 0 return comment "FOO-40"
		counter packets 5 bytes 300 reject comment "FOO default-action reject"
	}

Unable to ssh, rule 30 reject:

$ telnet 192.168.122.11 22
Trying 192.168.122.11...
telnet: Unable to connect to remote host: Connection refused

Expected rules:

	chain FOO {
		ct state related,established counter packets 464 bytes 30601 return comment "FOO-10"
		meta l4proto icmp ct state new counter packets 0 bytes 0 return comment "FOO-20"
		meta l4proto tcp tcp dport 22 # recent: UPDATE seconds: 60 hit_count: 4 name: FOO-30 side: source mask: 255.255.255.255 counter packets 0 bytes 0 reject comment "FOO-30"
		meta l4proto tcp tcp dport 22 # recent: SET name: FOO-30 side: source mask: 255.255.255.255 counter packets 1 bytes 60 comment "FOO-30"
		meta l4proto tcp ct state new tcp dport 22 counter packets 1 bytes 60 return comment "FOO-40"
		counter packets 0 bytes 0 reject comment "FOO-10000 default-action reject"
	}

Show firewall:

vyos@r11-roll:~$ show firewall 
Rulesets Information

---------------------------------
IPv4 Firewall "FOO"

Active on: (eth0,local) (eth2,in) (eth2,local)

Rule     Action    Protocol      Packets    Bytes  Conditions
-------  --------  ----------  ---------  -------  ----------------------------------
10       accept    all               621    48244  ct state { established, related }
20       accept    icmp                0        0  ct state { new } meta l4proto icmp
30       reject    tcp                 9      540  tcp dport { 22 }
40       accept    tcp                 0        0  ct state { new } tcp dport { 22 }
default  reject    all                 5      300

Report from the forum https://forum.vyos.io/t/firewall-recent-seems-to-block-all-requests

Details

Difficulty level
Unknown (require assessment)
Version
VyOS 1.4-rolling-202201240317
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

I had forgotten about the recent syntax and it was merged in a broken state (https://github.com/vyos/vyos-1x/blob/current/python/vyos/firewall.py#L164). We should try and find a remedy, or remove it from CLI.

I believe Sets was meant to be used as the equivalent but it is missing the ability to match on recent count N. The commented out meter line (in link above) was another attempt to implement it, however that line caused the nft tool to segfault when the file was executed.

Would changing the guide to use limit rate 4/minute achieve the same target functionality?

I've come up with a working idea how to implement but would like feedback before submitting a PR.

Diff: https://github.com/vyos/vyos-1x/compare/current...sarthurdev:T4209

How my branch outputs the recent rule (with a required change/migration to recent time):

tcp dport { 22 } add @FOO_30 { ip saddr limit rate 4/minute burst 4 packets } counter packets 0 bytes 0 return comment "FOO-30" # This line matches (and accepts) packets that do *not* exceed the limit
tcp dport { 22 } counter packets 0 bytes 0 reject comment "FOO-30" # This line catches packets that did exceed the limit and uses the action (reject) from the rule.

The problem with this is traffic that doesn't exceed the recent count/time will not pass further down the chain like before (accepted by FOO-40 in the OP).

sarthurdev changed the task status from Open to In progress.Jan 27 2022, 8:30 PM
In T4209#117429, @sdev wrote:

Would changing the guide to use limit rate 4/minute achieve the same target functionality?

What is the practical difference between limit rate and recent? Is it just two different ways of accomplishing the same?

In T4209#117429, @sdev wrote:

Would changing the guide to use limit rate 4/minute achieve the same target functionality?

What is the practical difference between limit rate and recent? Is it just two different ways of accomplishing the same?

After a proper look limit rate by itself can't be used as it is not limiting per source address.

Iβ€˜m no expert here nor extremely strong opiniated. My thoughts though: if theres no exact equivalent, why try to re-implement the recent functionality with nftables at β€žallβ€œ cost?

After all, recent was an option in VyOS because iptables had this functionality with rules. People naturally used it because it allowed (among other things) to throttle attacks. Now we change tool and have different ways of achieving a similar if not the same objective.

Therefore why not move away from recent entirely, starting with a cleaner slate, and obviously with a potentially breaking change?

I agree with @johannrichard, having recent "change" behavior is probably going to cause some confusion.

The logic explained above, where the connection will be accepted if not exceeding the limit, even though the rule action is drop or reject confused me at least πŸ˜„

I've actually found a way to define this properly, resulting rule now looks like below:

tcp dport { 22 } add @FOO_30 { ip saddr limit rate over 4/minute burst 4 packets } counter packets 3 bytes 156 reject comment "FOO-30"
ct state { new } tcp dport { 22 } counter packets 5 bytes 260 return comment "FOO-40"

We can match on a source address being over the limit and use the action correctly, like in previous versions.

I will PR this after a few more tests.

Viacheslav moved this task from Need Triage to Finished on the VyOS 1.4 Sagitta board.