Skip to content

Commit a4b72d3

Browse files
jestabrojestabro
authored
Merge pull request #4651 from l0crian1/fw-empty-nodes
T7366: Firewall rules allow empty nodes
2 parents c99f1c0 + dd43110 commit a4b72d3

File tree

5 files changed

+322
-1
lines changed

5 files changed

+322
-1
lines changed

‎interface-definitions/include/version/firewall-version.xml.i‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
<!-- include start from include/version/firewall-version.xml.i -->
2-
<syntaxVersion component='firewall' version='19'></syntaxVersion>
2+
<syntaxVersion component='firewall' version='20'></syntaxVersion>
33
<!-- include end -->

‎smoketest/config-tests/firewall-empty-nodes‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
set firewall bridge forward filter rule 1 action 'accept'
2+
set firewall ipv4 forward filter rule 1 action 'accept'
3+
set firewall ipv4 forward filter rule 2 action 'accept'
4+
set firewall ipv4 forward filter rule 3 action 'accept'
5+
set firewall ipv4 forward filter rule 5 action 'accept'
6+
set firewall ipv4 forward filter rule 6 action 'accept'
7+
set firewall ipv4 forward filter rule 7 action 'accept'
8+
set firewall ipv4 forward filter rule 8 action 'accept'
9+
set firewall ipv4 forward filter rule 9 action 'accept'
10+
set firewall ipv4 forward filter rule 10 action 'accept'
11+
set firewall ipv4 forward filter rule 10 log
12+
set firewall ipv4 forward filter rule 11 action 'accept'
13+
set firewall ipv4 forward filter rule 13 action 'accept'
14+
set firewall ipv4 forward filter rule 14 action 'accept'
15+
set firewall ipv4 forward filter rule 15 action 'accept'
16+
set firewall ipv4 forward filter rule 16 action 'accept'
17+
set firewall ipv4 forward filter rule 17 action 'accept'
18+
set firewall ipv4 forward filter rule 18 action 'accept'
19+
set firewall ipv4 forward filter rule 19 action 'accept'
20+
set firewall ipv4 forward filter rule 20 action 'accept'
21+
set firewall ipv4 forward filter rule 21 action 'accept'
22+
set firewall ipv6 forward filter rule 1 action 'accept'
23+
set firewall ipv6 forward filter rule 2 action 'accept'
24+
set interfaces ethernet eth0
25+
set interfaces ethernet eth1
26+
set interfaces ethernet eth2
27+
set interfaces loopback lo
28+
set system console device ttyS0 speed '115200'
29+
set system host-name 'vyos'
30+
set system login user vyos authentication encrypted-password '$6$O5gJRlDYQpj$MtrCV9lxMnZPMbcxlU7.FI793MImNHznxGoMFgm3Q6QP3vfKJyOSRCt3Ka/GzFQyW1yZS4NS616NLHaIPPFHc0'
31+
set system login user vyos authentication plaintext-password ''

‎smoketest/configs/firewall-empty-nodes‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
firewall {
2+
bridge {
3+
forward {
4+
filter {
5+
rule 1 {
6+
action "accept"
7+
vlan
8+
}
9+
}
10+
}
11+
}
12+
ipv4 {
13+
forward {
14+
filter {
15+
rule 1 {
16+
action "accept"
17+
add-address-to-group
18+
}
19+
rule 2 {
20+
action "accept"
21+
connection-status
22+
}
23+
rule 3 {
24+
action "accept"
25+
destination
26+
}
27+
rule 5 {
28+
action "accept"
29+
fragment
30+
}
31+
rule 6 {
32+
action "accept"
33+
icmp
34+
}
35+
rule 7 {
36+
action "accept"
37+
inbound-interface
38+
}
39+
rule 8 {
40+
action "accept"
41+
ipsec
42+
}
43+
rule 9 {
44+
action "accept"
45+
limit
46+
}
47+
rule 10 {
48+
action "accept"
49+
log
50+
log-options
51+
}
52+
rule 11 {
53+
action "accept"
54+
outbound-interface
55+
}
56+
rule 13 {
57+
action "accept"
58+
source
59+
}
60+
rule 14 {
61+
action "accept"
62+
tcp
63+
}
64+
rule 15 {
65+
action "accept"
66+
time
67+
}
68+
rule 16 {
69+
action "accept"
70+
ttl
71+
}
72+
rule 17 {
73+
action "accept"
74+
destination {
75+
group
76+
}
77+
}
78+
rule 18 {
79+
action "accept"
80+
destination {
81+
geoip
82+
}
83+
}
84+
rule 19 {
85+
action "accept"
86+
source {
87+
group
88+
}
89+
}
90+
rule 20 {
91+
action "accept"
92+
source {
93+
geoip
94+
}
95+
}
96+
rule 21 {
97+
action "accept"
98+
tcp {
99+
flags
100+
}
101+
}
102+
}
103+
}
104+
}
105+
ipv6 {
106+
forward {
107+
filter {
108+
rule 1 {
109+
action "accept"
110+
hop-limit
111+
}
112+
rule 2 {
113+
action "accept"
114+
icmpv6
115+
}
116+
}
117+
}
118+
}
119+
}
120+
interfaces {
121+
ethernet eth0 {
122+
}
123+
ethernet eth1 {
124+
}
125+
ethernet eth2 {
126+
}
127+
loopback lo {
128+
}
129+
}
130+
system {
131+
console {
132+
device ttyS0 {
133+
speed "115200"
134+
}
135+
}
136+
host-name "vyos"
137+
login {
138+
user vyos {
139+
authentication {
140+
encrypted-password "$6$O5gJRlDYQpj$MtrCV9lxMnZPMbcxlU7.FI793MImNHznxGoMFgm3Q6QP3vfKJyOSRCt3Ka/GzFQyW1yZS4NS616NLHaIPPFHc0"
141+
plaintext-password ""
142+
}
143+
}
144+
}
145+
}
146+
147+
// Warning: Do not remove the following line.
148+
// vyos-config-version: "bgp@6:broadcast-relay@1:cluster@2:config-management@1:conntrack@6:conntrack-sync@2:container@2:dhcp-relay@2:dhcp-server@8:dhcpv6-server@1:dns-dynamic@4:dns-forwarding@4:firewall@15:flow-accounting@1:https@6:ids@1:interfaces@32:ipoe-server@3:ipsec@13:isis@3:l2tp@9:lldp@2:mdns@1:monitoring@1:nat@8:nat66@3:ntp@3:openconnect@3:ospf@2:pim@1:policy@8:pppoe-server@10:pptp@5:qos@2:quagga@11:reverse-proxy@1:rip@1:rpki@2:salt@1:snmp@3:ssh@2:sstp@6:system@27:vrf@3:vrrp@4:vyos-accel-ppp@2:wanloadbalance@3:webproxy@2"
149+
// Release version: 1.4.3

‎src/conf_mode/firewall.py‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,42 @@ def verify_jump_target(firewall, hook, jump_target, family, recursive=False):
196196

197197
targets_seen.append(target)
198198

199+
def is_node_empty(rule_conf):
200+
is_empty_list = []
201+
is_empty_list.append([
202+
['add_address_to_group'],
203+
['connection_status'],
204+
['destination'],
205+
['destination', 'group'],
206+
['destination', 'geoip'],
207+
['fragment'],
208+
['gre'],
209+
['gre', 'flags'],
210+
['hop_limit'],
211+
['icmp'],
212+
['icmpv6'],
213+
['inbound_interface'],
214+
['ipsec'],
215+
['limit'],
216+
['log_options'],
217+
['outbound_interface'],
218+
['set'],
219+
['source'],
220+
['source', 'group'],
221+
['source', 'geoip'],
222+
['tcp'],
223+
['tcp', 'flags'],
224+
['time'],
225+
['ttl'],
226+
['vlan']
227+
])
228+
229+
for node in is_empty_list[0]:
230+
if dict_search_args(rule_conf, *node) == {}:
231+
return True, node
232+
233+
return False, None
234+
199235
def verify_rule(firewall, family, hook, priority, rule_id, rule_conf):
200236
if 'action' not in rule_conf:
201237
raise ConfigError('Rule action must be defined')
@@ -244,6 +280,11 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf):
244280
if {'match_frag', 'match_non_frag'} <= set(rule_conf['fragment']):
245281
raise ConfigError('Cannot specify both "match-frag" and "match-non-frag"')
246282

283+
node_empty, node_name = is_node_empty(rule_conf)
284+
if node_empty:
285+
tmp = ' '.join(node_name).replace('_', '-')
286+
raise ConfigError(f'Configuration node {tmp} may not be empty')
287+
247288
if 'limit' in rule_conf:
248289
if 'rate' in rule_conf['limit']:
249290
rate_int = re.sub(r'\D', '', rule_conf['limit']['rate'])

‎src/migration-scripts/firewall/19-to-20‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
# Copyright VyOS maintainers and contributors <[email protected]>
2+
#
3+
# This library is free software; you can redistribute it and/or
4+
# modify it under the terms of the GNU Lesser General Public
5+
# License as published by the Free Software Foundation; either
6+
# version 2.1 of the License, or (at your option) any later version.
7+
#
8+
# This library is distributed in the hope that it will be useful,
9+
# but WITHOUT ANY WARRANTY; without even the implied warranty of
10+
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
11+
# Lesser General Public License for more details.
12+
#
13+
# You should have received a copy of the GNU Lesser General Public License
14+
# along with this library. If not, see <http://www.gnu.org/licenses/>.
15+
16+
# T7366: Firewall rules allow empty nodes
17+
# When configuring the firewall, many nodes are accepted with empty values,
18+
# which are not parsed into rules.
19+
20+
# Very few of these have subsequent error handling. Some should be obvious
21+
# to the user that a value is required, like 'inbound-interface' (though
22+
# an error should still be thrown if they're configured without children).
23+
24+
# But some could be misunderstood and lead to an outage or wide open firewall.
25+
# For instance, let's say someone wanted to block all icmp. They may incorrectly
26+
# configure:
27+
28+
# set firewall ipv4 input filter rule 10 action drop
29+
# set firewall ipv4 input filter rule 10 icmp
30+
31+
# And this would create this rule in nftables, dropping all traffic in subsequent rules:
32+
33+
# counter packets 0 bytes 0 drop comment "ipv4-INP-filter-10"
34+
35+
# They could also unintentionally allow all traffic by attempting to only allow icmp in a rule.
36+
37+
import json
38+
39+
from vyos.configtree import ConfigTree
40+
from vyos.utils.dict import dict_search_args
41+
42+
firewall_base = ['firewall']
43+
44+
def migrate(config: ConfigTree) -> None:
45+
if not config.exists(firewall_base):
46+
# Nothing to do
47+
return
48+
49+
firewall_dict = json.loads(config.to_json()).get('firewall')
50+
51+
is_empty_list = []
52+
is_empty_list.append([
53+
['add-address-to-group'],
54+
['connection-status'],
55+
['destination', 'group'],
56+
['destination', 'geoip'],
57+
['destination'],
58+
['fragment'],
59+
['gre', 'flags'],
60+
['gre'],
61+
['hop-limit'],
62+
['icmp'],
63+
['icmpv6'],
64+
['inbound-interface'],
65+
['ipsec'],
66+
['limit'],
67+
['log-options'],
68+
['outbound-interface'],
69+
['set'],
70+
['source', 'group'],
71+
['source', 'geoip'],
72+
['source'],
73+
['tcp', 'flags'],
74+
['tcp'],
75+
['time'],
76+
['ttl'],
77+
['vlan']
78+
])
79+
80+
for family in ['ipv4', 'ipv6', 'bridge']:
81+
if family in firewall_dict:
82+
for chain in ['name','forward','input','output', 'prerouting']:
83+
if chain in firewall_dict[family]:
84+
for priority, priority_conf in firewall_dict[family][chain].items():
85+
if 'rule' in priority_conf:
86+
for rule_id, rule_conf in priority_conf['rule'].items():
87+
node_deleted_list = []
88+
for node in is_empty_list[0]:
89+
if dict_search_args(rule_conf, *node) == {}:
90+
if len(node) == 1:
91+
if node not in node_deleted_list:
92+
config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0]])
93+
else:
94+
del firewall_dict[family][chain][priority]['rule'][rule_id][node[0]][node[1]]
95+
96+
if dict_search_args(rule_conf, node[0]) == {}:
97+
config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0]])
98+
node_deleted_list.append([node[0]])
99+
else:
100+
config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0], node[1]])

0 commit comments

Comments
 (0)