diff mbox series

Send bad forward_zone packets to verdict_from_zone

Message ID 20220928153126.624032-1-dsmtngoat@gmail.com
State New
Headers show
Series Send bad forward_zone packets to verdict_from_zone | expand

Commit Message

Gordon Maclean Sept. 28, 2022, 3:31 p.m. UTC
From: Gordon Maclean <dsmtngoat@gmail.com>

Received forward packets which fail acceptance tests are finally handled by a <verdict>_to_<zone> chain
where <verdict> is typically "drop" or "reject".  This "_to_" chain only matches packets destined
for the interface, and so does not match packets destined for interfaces other than where they were received.

As a result the final resolution depends on the default policy for the forward chain, which for a
reasonably configured router is "drop" or "reject", so this is unlikely to be a security hole,
This does not match what the user has configured as the resolution of forward packets received
for the zone.  Also, if the user has enabled logging of failed packets, these packets will not be logged.

This patch may also result in failues in firewall4/tests. That has not been investigated.
---
 root/usr/share/firewall4/templates/ruleset.uc | 2 +-
 root/usr/share/ucode/fw4.uc                   | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Jo-Philipp Wich Sept. 28, 2022, 6:16 p.m. UTC | #1
Hi,

the forward policy for zones is supposed to only apply to forwarded traffic
among interfaces of the same zone. If I read it correctly, your patch would
change this long standing behavior to something else.

~ Jo
Gordon Maclean Sept. 28, 2022, 9:48 p.m. UTC | #2
This patch was resubmitted, in a format more closely matching the openwrt
patch guidelines, with the subject
   "[PATCH] fw4: handle bad forward_zone packets with v_from_z" .


Gordon


On Wed, Sep 28, 2022 at 9:31 AM <dsmtngoat@gmail.com> wrote:
>
> From: Gordon Maclean <dsmtngoat@gmail.com>
>
> Received forward packets which fail acceptance tests are finally handled by a <verdict>_to_<zone> chain
> where <verdict> is typically "drop" or "reject".  This "_to_" chain only matches packets destined
> for the interface, and so does not match packets destined for interfaces other than where they were received.
>
> As a result the final resolution depends on the default policy for the forward chain, which for a
> reasonably configured router is "drop" or "reject", so this is unlikely to be a security hole,
> This does not match what the user has configured as the resolution of forward packets received
> for the zone.  Also, if the user has enabled logging of failed packets, these packets will not be logged.
>
> This patch may also result in failues in firewall4/tests. That has not been investigated.
> ---
>  root/usr/share/firewall4/templates/ruleset.uc | 2 +-
>  root/usr/share/ucode/fw4.uc                   | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/root/usr/share/firewall4/templates/ruleset.uc b/root/usr/share/firewall4/templates/ruleset.uc
> index eaa1f04..daef252 100644
> --- a/root/usr/share/firewall4/templates/ruleset.uc
> +++ b/root/usr/share/firewall4/templates/ruleset.uc
> @@ -239,7 +239,7 @@ table inet fw4 {
>                 ct status dnat accept comment "!fw4: Accept port forwards"
>  {%  endif %}
>  {%  fw4.includes('chain-append', `forward_${zone.name}`) %}
> -               jump {{ zone.forward }}_to_{{ zone.name }}
> +               jump {{ zone.forward }}_from_{{ zone.name }}
>         }
>
>  {%  if (zone.dflags.helper): %}
> diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc
> index 29ae053..a6c1ae5 100644
> --- a/root/usr/share/ucode/fw4.uc
> +++ b/root/usr/share/ucode/fw4.uc
> @@ -2113,6 +2113,7 @@ return {
>
>                 zone.sflags = {};
>                 zone.sflags[zone.input] = true;
> +               zone.sflags[zone.forward] = true;
>
>                 zone.dflags = {};
>                 zone.dflags[zone.output] = true;
> --
> 2.37.3
>
diff mbox series

Patch

diff --git a/root/usr/share/firewall4/templates/ruleset.uc b/root/usr/share/firewall4/templates/ruleset.uc
index eaa1f04..daef252 100644
--- a/root/usr/share/firewall4/templates/ruleset.uc
+++ b/root/usr/share/firewall4/templates/ruleset.uc
@@ -239,7 +239,7 @@  table inet fw4 {
 		ct status dnat accept comment "!fw4: Accept port forwards"
 {%  endif %}
 {%  fw4.includes('chain-append', `forward_${zone.name}`) %}
-		jump {{ zone.forward }}_to_{{ zone.name }}
+		jump {{ zone.forward }}_from_{{ zone.name }}
 	}
 
 {%  if (zone.dflags.helper): %}
diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc
index 29ae053..a6c1ae5 100644
--- a/root/usr/share/ucode/fw4.uc
+++ b/root/usr/share/ucode/fw4.uc
@@ -2113,6 +2113,7 @@  return {
 
 		zone.sflags = {};
 		zone.sflags[zone.input] = true;
+		zone.sflags[zone.forward] = true;
 
 		zone.dflags = {};
 		zone.dflags[zone.output] = true;