| Message ID | 20221103210921.580870-1-dsmtngoat@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | fw4: fix handling of unaccepted forward packets | expand |
Hi, you misunderstand the purpose of the zone forward policy. It is not meant to catch traffic from a zone to another zone, but traffic relayed from one interface to another interface within the same zone. Traffic from one zone to another zone is solely handled by the global forward policy in the defaults section (or individual rules). Your patch would change this long standing behavior, therefor NACK from me. ~ Jo
OK, I finally understand that the forward option in a zone applies to traffic
within the zone. Calling it "intra-zone forward" might be more clear, though
I do see that it is stated explicitly in the "Firewall - Zone Settings" window.
Here's a change to enable logging of unaccepted inter-zone forwards,
in case it is of any use.
diff --git a/root/usr/share/firewall4/templates/ruleset.uc
b/root/usr/share/firewall4/templates/ruleset.uc
index d6eedfd..e848da8 100644
--- a/root/usr/share/firewall4/templates/ruleset.uc
+++ b/root/usr/share/firewall4/templates/ruleset.uc
@@ -243,6 +243,8 @@ table inet fw4 {
{% if (fw4.forward_policy() != "accept" && (zone.log & 1)): %}
log prefix "{{ fw4.forward_policy() }} {{ zone.name }}
forward: "
{% endif %}
+{% fw4.includes('chain-append', `forward_${zone.name}`) %}
+ jump {{ fw4.forward_policy() }}_from_{{ zone.name }}
}
{% if (zone.dflags.helper): %}
diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc
index 47e86cd..fe06f3e 100644
--- a/root/usr/share/ucode/fw4.uc
+++ b/root/usr/share/ucode/fw4.uc
@@ -2112,7 +2112,9 @@ return {
zone.masq6_dest_subnets =
subnets_group_by_masking(masq_dest_subnets[1]);
zone.sflags = {};
- zone.sflags[zone.input] = true;
+ zone.sflags["accept"] = true;
+ zone.sflags["reject"] = true;
+ zone.sflags["drop"] = true;
zone.dflags = {};
zone.dflags[zone.output] = true;
> Hi,
> you misunderstand the purpose of the zone forward policy. It is not meant to
> catch traffic from a zone to another zone, but traffic relayed from one
> interface to another interface within the same zone.
> Traffic from one zone to another zone is solely handled by the global forward
> policy in the defaults section (or individual rules).
> Your patch would change this long standing behavior, therefor NACK from me.
> ~ Jo
On Thu, Nov 3, 2022 at 3:10 PM <dsmtngoat@gmail.com> wrote:
>
> From: Gordon Maclean <dsmtngoat@gmail.com>
>
> This is a resumbit of
> [PATCH] fw4: handle bad forward_zone packets with v_from_z,
> with an updated commit message.
>
> Below, FROM and TO are capitalized for emphasis.
>
> Packets on the input chain that fail accceptance
> rules are eventually handled by a rule created by
> "jump {{ zone.input }}_FROM_{{ zone.name }}" in ruleset.uc.
>
> For a wan zone, with input policy "drop", this results in a jump to
> the "drop_FROM_wan" chain where they are optionally logged, and
> dropped.
>
> However, packets on the forward chain that fail acceptance rules
> are eventually handled by the rule created by
> "jump {{ zone.forward }}_TO_{{ zone.name }}" in ruleset.uc.
>
> For zone wan, with forward policy "drop", packets would be sent to
> the "drop_TO_wan" chain.
>
> This is a bug, since that chain matches packets sent TO the
> interface for the zone, not FROM the interface, and so will
> fail to match all unaccepted forwarded packets received on the
> zone. As a result these forwarded packets are handled by the
> global forward policy and not by the forward policy for the
> zone, and will not be logged.
>
> This patch sets the final disposition for unaccepted forwarded
> packets to be the same as for unaccepted input packets.
>
> Signed-off-by: Gordon Maclean <dsmtngoat@gmail.com>
> ---
> 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 d6eedfd..833c762 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 (fw4.forward_policy() != "accept" && (zone.log & 1)): %}
> log prefix "{{ fw4.forward_policy() }} {{ zone.name }} forward: "
> {% endif %}
> diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc
> index 47e86cd..74b7c81 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.38.1
>
diff --git a/root/usr/share/firewall4/templates/ruleset.uc b/root/usr/share/firewall4/templates/ruleset.uc index d6eedfd..833c762 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 (fw4.forward_policy() != "accept" && (zone.log & 1)): %} log prefix "{{ fw4.forward_policy() }} {{ zone.name }} forward: " {% endif %} diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc index 47e86cd..74b7c81 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;