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

Message ID 1522767066-4911-1-git-send-email-alin.nastac@technicolor.com
State Superseded
Delegated to: Jo-Philipp Wich
Headers show
Series
  • [LEDE-DEV,PATCHv2] firewall: fix logging of dropped & rejected packets
Related show

Commit Message

Alin Năstac April 3, 2018, 2:51 p.m.
From: Alin Nastac <alin.nastac@gmail.com>

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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 12 deletions(-)

Comments

Philip Prindeville April 9, 2018, 7:07 p.m. | #1
“guest” or “salon”?



> On Apr 3, 2018, at 8:51 AM, Alin Nastac <alin.nastac@gmail.com> wrote:
> 
> From: Alin Nastac <alin.nastac@gmail.com>
> 
> 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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/zones.c b/zones.c
> index e00d527..9f00aca 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,25 @@ fw3_flush_zones(struct fw3_ipt_handle *handle, struct fw3_state *state,
> 			fw3_ipt_delete_chain(handle, chain);
> 		}
> 
> +		/* flush (DROP|REJECT)_{src|dest)_(zone name) chains */
> +		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);
> 	}
> }
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

Patch

diff --git a/zones.c b/zones.c
index e00d527..9f00aca 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,25 @@  fw3_flush_zones(struct fw3_ipt_handle *handle, struct fw3_state *state,
 			fw3_ipt_delete_chain(handle, chain);
 		}
 
+		/* flush (DROP|REJECT)_{src|dest)_(zone name) chains */
+		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);
 	}
 }