[pve-devel] [PATCH firewall] fix #967: source: dest: limit length

[pve-devel] [PATCH firewall] fix #967: source: dest: limit length

[Aaron Lauterer]

iptables-restore has a buffer limit of 1024 for paramters [0].

If users end up adding a long list of IPs in the source or dest field
they might reach this limit. The result is that the rule will not be
applied and pve-firewall will show some error in the syslog which will
be "hidden" for most users.

Enforcing a smaller limit ourselves should help to avoid any such
situation. 512 characters should help to not run into any problems that
stem from differences in what counts as character. If people need longer
lists, using IP sets are the better approach anyway.

[0] http://git.netfilter.org/iptables/tree/iptables/xshared.c?h=v1.8.7#n469

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 src/PVE/Firewall.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 92ea33d..50be187 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1449,11 +1449,13 @@ my $rule_properties = {
 	description => "Restrict packet source address. $addr_list_descr",
 	type => 'string', format => 'pve-fw-addr-spec',
 	optional => 1,
+	maxLength => 512,
     },
     dest => {
 	description => "Restrict packet destination address. $addr_list_descr",
 	type => 'string', format => 'pve-fw-addr-spec',
 	optional => 1,
+	maxLength => 512,
     },
     proto => {
 	description => "IP protocol. You can use protocol names ('tcp'/'udp') or simple numbers, as defined in '/etc/protocols'.",
-- 
2.20.1

[pve-devel] [PATCH manager] ui: firewall: rule: maxlength for source and dest

[Aaron Lauterer]

Limiting the length of the source and dest paramters helps to avoid
problems with iptables-restore which would not apply a rule if a
parameter is larger than the parameter buffer (1024)[0]. As the API is
already limiting this, we should also reflect that in the GUI and give
people a hint that IP sets are most likely the better approach.

[0] http://git.netfilter.org/iptables/tree/iptables/xshared.c?h=v1.8.7#n469

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
This one "needs" the firewall patch 'fix #967: source: dest: limit length'

As always when it comes to messages, someone might have a better idea
how to phrase the maxLengthText.

 www/manager6/grid/FirewallRules.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/www/manager6/grid/FirewallRules.js b/www/manager6/grid/FirewallRules.js
index 424bdfcb..f32a1ea1 100644
--- a/www/manager6/grid/FirewallRules.js
+++ b/www/manager6/grid/FirewallRules.js
@@ -135,7 +135,8 @@ Ext.define('PVE.FirewallRulePanel', {
 		base_url: me.list_refs_url,
 		value: '',
 		fieldLabel: gettext('Source'),
-
+		maxLength: 512,
+		maxLengthText: gettext('Too long, consider using IP sets.'),
 	    },
 	    {
 		xtype: 'pveIPRefSelector',
@@ -145,6 +146,8 @@ Ext.define('PVE.FirewallRulePanel', {
 		base_url: me.list_refs_url,
 		value: '',
 		fieldLabel: gettext('Destination'),
+		maxLength: 512,
+		maxLengthText: gettext('Too long, consider using IP sets.'),
 	    },
 	);
 
-- 
2.20.1

[pve-devel] applied: [PATCH firewall] fix #967: source: dest: limit length

[Thomas Lamprecht]

On 22.04.21 14:30, Aaron Lauterer wrote:
> iptables-restore has a buffer limit of 1024 for paramters [0].
> 
> If users end up adding a long list of IPs in the source or dest field
> they might reach this limit. The result is that the rule will not be
> applied and pve-firewall will show some error in the syslog which will
> be "hidden" for most users.
> 
> Enforcing a smaller limit ourselves should help to avoid any such
> situation. 512 characters should help to not run into any problems that
> stem from differences in what counts as character. If people need longer
> lists, using IP sets are the better approach anyway.
> 
> [0] http://git.netfilter.org/iptables/tree/iptables/xshared.c?h=v1.8.7#n469
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
>

applied, thanks! Even in the worst case IP-address length, namely Ipv4-mapped
IPv6 (which we do not really support anyway, so only as theoretical worst-case),
for example: "0000:0000:0000:0000:0000:ffff:192.168.100.228", there we would
need 45 + 1 characters per entry plus separator, so even then one could add 11
IPs in there, which is IMO more than enough for direct apply - IPsets should
be preferred, like you hint in the gui patch anyway.

[pve-devel] applied: [PATCH manager] ui: firewall: rule: maxlength for source and dest

[Thomas Lamprecht]

On 22.04.21 14:30, Aaron Lauterer wrote:
> Limiting the length of the source and dest paramters helps to avoid
> problems with iptables-restore which would not apply a rule if a
> parameter is larger than the parameter buffer (1024)[0]. As the API is
> already limiting this, we should also reflect that in the GUI and give
> people a hint that IP sets are most likely the better approach.
> 
> [0] http://git.netfilter.org/iptables/tree/iptables/xshared.c?h=v1.8.7#n469
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> This one "needs" the firewall patch 'fix #967: source: dest: limit length'
> 
> As always when it comes to messages, someone might have a better idea
> how to phrase the maxLengthText.
> 

good enough for me  :)

>  www/manager6/grid/FirewallRules.js | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>

applied, thanks!