diff mbox series

[LEDE-DEV] firewall: fix logging of dropped & rejected packets

Message ID 1522762272-27393-1-git-send-email-alin.nastac@gmail.com
State Superseded
Delegated to: John Crispin
Headers show
Series [LEDE-DEV] firewall: fix logging of dropped & rejected packets | expand

Commit Message

Alin Năstac April 3, 2018, 1:31 p.m. UTC 
Reproduction scenario:
 - use 3 interfaces with 3 different zones - lan, wan and guest
 - configure firewall to allow forwarding from lan to wan
 - add DROP rule to prevent forwarding from lan to guest
 - although packets are forwarded from lan to wan, "DROP(dest guest)"
 traces are generated by zone_guest_dest_DROP chain

Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 zones.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 12 deletions(-)

Comments

Jo-Philipp Wich April 3, 2018, 1:44 p.m. UTC | #1 
Hi Alin,

thanks for the patch.

Unfortunately it definitely is too big for a simple "fix logging". Will
take a deeper look at it later but from a first glance it does a few
unrelated changes, renames chains and has some minor style deviations.

Regards,
Jo
Alin Năstac April 3, 2018, 2:22 p.m. UTC | #2 
Hi Jo,

The idea is to fix log issues created by chains such as these:
iptables -S zone_lan_forward
-A zone_lan_forward -m comment --comment "!fw3: user chain for
forwarding" -j forwarding_lan_rule
-A zone_lan_forward -m comment --comment "!fw3: drop_lan_2_guest" -j
zone_guest_dest_DROP
-A zone_lan_forward -m comment --comment "!fw3: Default action for
outgoing NAT" -j zone_wan_dest_ACCEPT
-A zone_lan_forward -m comment --comment "!fw3: forwarding lan -> wan"
-j zone_wan_dest_ACCEPT
-A zone_lan_forward -m conntrack --ctstate DNAT -m comment --comment
"!fw3: Accept port forwards" -j ACCEPT
-A zone_lan_forward -m comment --comment "!fw3" -j zone_lan_dest_ACCEPT
iptables -S zone_guest_dest_DROP
-A zone_guest_dest_DROP -m limit --limit 5/min -m comment --comment
"!fw3" -j LOG --log-prefix "DROP(dest guest)"
-A zone_guest_dest_DROP -o br-guest -m comment --comment "!fw3" -j DROP

As you can see, packets forwarded from lan to wan will also pass
zone_guest_dest_DROP which will generate traces such as these:
[17091.072000] DROP(dest guest)IN=br-lan OUT=pppoe-wan
MAC=a4:91:b1:46:44:6e:30:91:8f:f7:e5:e5:08:00 SRC=192.168.1.105
DST=83.170.84.172 LEN=52 TOS=0x00 PREC=0x00 TTL=127 ID=20150 DF
PROTO=TCP SPT=53122 DPT=80 WINDOW=8192 RES=0x00 SYN URGP=0 MARK=0x9800

To do that I had to unify the LOG and DROP targets in a new chain
called DROP_dest_guest. These types of chains are created only when
necessary, i.e. when zone has log=1. Here is an example of how such
chains are created:
iptables -S zone_wan_dest_DROP
-A zone_wan_dest_DROP -o pppoe-wan -m comment --comment "!fw3" -j DROP_dest_wan
iptables -S DROP_dest_wan
-A DROP_dest_wan -m limit --limit 10/sec -m comment --comment "!fw3"
-j LOG --log-prefix "DROP(dest wan)"
-A DROP_dest_wan -m comment --comment "!fw3" -j DROP

BR,
Alin

On Tue, Apr 3, 2018 at 3:44 PM, Jo-Philipp Wich <jo@mein.io> wrote:
> Hi Alin,
>
> thanks for the patch.
>
> Unfortunately it definitely is too big for a simple "fix logging". Will
> take a deeper look at it later but from a first glance it does a few
> unrelated changes, renames chains and has some minor style deviations.
>
> Regards,
> Jo
diff mbox series

Patch

diff --git a/zones.c b/zones.c
index e00d527..1f55aa6 100644
--- a/zones.c
+++ b/zones.c
@@ -20,6 +20,8 @@ 
 #include "ubus.h"
 #include "helpers.h"
 
+#define filter_target(t) \
+	((t == FW3_FLAG_REJECT) ? "reject" : fw3_flag_names[t])
 
 #define C(f, tbl, tgt, fmt) \
 	{ FW3_FAMILY_##f, FW3_TABLE_##tbl, FW3_FLAG_##tgt, fmt }
@@ -401,6 +403,19 @@  print_zone_chain(struct fw3_ipt_handle *handle, struct fw3_state *state,
 	set(zone->flags, handle->family, handle->table);
 }
 
+static const char*
+jump_target(enum fw3_flag t, bool src, struct fw3_zone *zone, char *buf, size_t size)
+{
+	if ((zone->log & FW3_ZONE_LOG_FILTER) && t > FW3_FLAG_ACCEPT)
+	{
+		snprintf(buf, size, "%s_%s_%s", fw3_flag_names[t],
+				src ? "src" : "dest", zone->name);
+		return buf;
+	}
+
+	return filter_target(t);
+}
+
 static void
 print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 					 bool reload, struct fw3_zone *zone,
@@ -420,9 +435,6 @@  print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 		"forward", "FORWARD",
 	};
 
-#define jump_target(t) \
-	((t == FW3_FLAG_REJECT) ? "reject" : fw3_flag_names[t])
-
 	if (handle->table == FW3_TABLE_FILTER)
 	{
 		for (t = FW3_FLAG_ACCEPT; t <= FW3_FLAG_DROP; t++)
@@ -430,7 +442,7 @@  print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 			if (has(zone->flags, handle->family, fw3_to_src_target(t)))
 			{
 				r = fw3_ipt_rule_create(handle, NULL, dev, NULL, sub, NULL);
-				fw3_ipt_rule_target(r, jump_target(t));
+				fw3_ipt_rule_target(r, jump_target(t, true, zone, buf, sizeof(buf)));
 				fw3_ipt_rule_extra(r, zone->extra_src);
 
 				if (t == FW3_FLAG_ACCEPT && !state->defaults.drop_invalid)
@@ -455,7 +467,7 @@  print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 				}
 
 				r = fw3_ipt_rule_create(handle, NULL, NULL, dev, NULL, sub);
-				fw3_ipt_rule_target(r, jump_target(t));
+				fw3_ipt_rule_target(r, jump_target(t, false, zone, buf, sizeof(buf)));
 				fw3_ipt_rule_extra(r, zone->extra_dest);
 				fw3_ipt_rule_replace(r, "zone_%s_dest_%s", zone->name,
 				                     fw3_flag_names[t]);
@@ -503,7 +515,7 @@  print_interface_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 		{
 			if (zone->log & FW3_ZONE_LOG_MANGLE)
 			{
-				snprintf(buf, sizeof(buf) - 1, "MSSFIX(%s): ", zone->name);
+				snprintf(buf, sizeof(buf), "MSSFIX(%s): ", zone->name);
 
 				r = fw3_ipt_rule_create(handle, &tcp, NULL, dev, NULL, sub);
 				fw3_ipt_rule_addarg(r, false, "--tcp-flags", "SYN,RST");
@@ -640,30 +652,46 @@  print_zone_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
 			{
 				if (has(zone->flags, handle->family, fw3_to_src_target(t)))
 				{
+					fw3_ipt_create_chain(handle, "%s_src_%s",
+					                     fw3_flag_names[t], zone->name);
+
 					r = fw3_ipt_rule_new(handle);
 
-					snprintf(buf, sizeof(buf) - 1, "%s(src %s)",
+					snprintf(buf, sizeof(buf), "%s(src %s)",
 					         fw3_flag_names[t], zone->name);
 
 					fw3_ipt_rule_limit(r, &zone->log_limit);
 					fw3_ipt_rule_target(r, "LOG");
 					fw3_ipt_rule_addarg(r, false, "--log-prefix", buf);
-					fw3_ipt_rule_append(r, "zone_%s_src_%s",
-					                    zone->name, fw3_flag_names[t]);
+					fw3_ipt_rule_append(r, "%s_src_%s",
+					                    fw3_flag_names[t], zone->name);
+
+					r = fw3_ipt_rule_new(handle);
+					fw3_ipt_rule_target(r, filter_target(t));
+					fw3_ipt_rule_append(r, "%s_src_%s",
+					                    fw3_flag_names[t], zone->name);
 				}
 
 				if (has(zone->flags, handle->family, t))
 				{
+					fw3_ipt_create_chain(handle, "%s_dest_%s",
+					                     fw3_flag_names[t], zone->name);
+
 					r = fw3_ipt_rule_new(handle);
 
-					snprintf(buf, sizeof(buf) - 1, "%s(dest %s)",
+					snprintf(buf, sizeof(buf), "%s(dest %s)",
 					         fw3_flag_names[t], zone->name);
 
 					fw3_ipt_rule_limit(r, &zone->log_limit);
 					fw3_ipt_rule_target(r, "LOG");
 					fw3_ipt_rule_addarg(r, false, "--log-prefix", buf);
-					fw3_ipt_rule_append(r, "zone_%s_dest_%s",
-					                    zone->name, fw3_flag_names[t]);
+					fw3_ipt_rule_append(r, "%s_dest_%s",
+					                    fw3_flag_names[t], zone->name);
+
+					r = fw3_ipt_rule_new(handle);
+					fw3_ipt_rule_target(r, filter_target(t));
+					fw3_ipt_rule_append(r, "%s_dest_%s",
+					                    fw3_flag_names[t], zone->name);
 				}
 			}
 		}
@@ -758,6 +786,7 @@  fw3_flush_zones(struct fw3_ipt_handle *handle, struct fw3_state *state,
 	struct fw3_zone *z, *tmp;
 	const struct fw3_chain_spec *c;
 	char chain[32];
+	enum fw3_flag t;
 
 	list_for_each_entry_safe(z, tmp, &state->zones, list)
 	{
@@ -790,6 +819,27 @@  fw3_flush_zones(struct fw3_ipt_handle *handle, struct fw3_state *state,
 			fw3_ipt_delete_chain(handle, chain);
 		}
 
+		if (z->log & FW3_ZONE_LOG_FILTER)
+		{
+			for (t = FW3_FLAG_REJECT; t <= FW3_FLAG_DROP; t++)
+			{
+				if (has(z->flags, handle->family, fw3_to_src_target(t)))
+				{
+					snprintf(chain, sizeof(chain), "%s_src_%s",
+					         fw3_flag_names[t], z->name);
+					fw3_ipt_flush_chain(handle, chain);
+					fw3_ipt_delete_chain(handle, chain);
+				}
+				if (has(z->flags, handle->family, t))
+				{
+					snprintf(chain, sizeof(chain), "%s_dest_%s",
+					         fw3_flag_names[t], z->name);
+					fw3_ipt_flush_chain(handle, chain);
+					fw3_ipt_delete_chain(handle, chain);
+				}
+			}
+		}
+
 		del(z->flags, handle->family, handle->table);
 	}
 }