Squid - firewall rules sanitization #289

doktornotor · 2017-02-03T14:46:03Z

Do NOT merge now, this is for discussion ATM.

OK, guys... so, reading the code, it appears to me the current firewall rules generated by Squid do not make particularly good sense. So I've come up with this:

use rdr pass to pass traffic when we have transparent proxy enabled and are doing NAT
when non-transparent (case 'rule':) we should pass the traffic to the configured proxy ports only, and the transparent proxy ports/interfaces really have nothing to do with those rules, so no idea why's the code referencing them at all
finally, the PPPoE cases - no clue about this, no way to test, never used. Regardless, the current code there appears to be complete nonsense.

For a quick idea, here's a comparison of rules generated by the old and new code. with a really simple setup

LAN = igb0_vlan10 being a non-transparent interface
WLAN = igb0_vlan88 being set up as transparent interface with SSL/MITM
proxy ports default 3128/3129
RFC1918 excluded from proxying

Old rules

no rdr on igb0_vlan88 inet proto tcp from any to 192.168.0.0/16 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 192.168.0.0/16 port = https
no rdr on igb0_vlan88 inet proto tcp from any to 172.16.0.0/12 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 172.16.0.0/12 port = https
no rdr on igb0_vlan88 inet proto tcp from any to 10.0.0.0/8 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 10.0.0.0/8 port = https

rdr on igb0_vlan88 inet proto tcp from any to ! (igb0_vlan88) port = http -> 127.0.0.1 port 3128
rdr on igb0_vlan88 inet proto tcp from any to ! (igb0_vlan88) port = https -> 127.0.0.1 port 3129

pass in quick on igb0_vlan88 proto tcp from any to ! (igb0_vlan88) port = http flags S/SA keep state
pass in quick on igb0_vlan88 proto tcp from any to ! (igb0_vlan88) port = https flags S/SA keep state
pass in quick on igb0_vlan88 proto tcp from any to ! (igb0_vlan88) port = 3128 flags S/SA keep state
pass in quick on igb0_vlan88 proto tcp from any to ! (igb0_vlan88) port = 3129 flags S/SA keep state

New rules

no rdr on igb0_vlan88 inet proto tcp from any to 192.168.0.0/16 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 192.168.0.0/16 port = https
no rdr on igb0_vlan88 inet proto tcp from any to 172.16.0.0/12 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 172.16.0.0/12 port = https
no rdr on igb0_vlan88 inet proto tcp from any to 10.0.0.0/8 port = http
no rdr on igb0_vlan88 inet proto tcp from any to 10.0.0.0/8 port = https

rdr pass on igb0_vlan88 inet proto tcp from any to ! (igb0_vlan88) port = http -> 127.0.0.1 port 3128
rdr pass on igb0_vlan88 inet proto tcp from any to ! (igb0_vlan88) port = https -> 127.0.0.1 port 3129

pass in quick on igb0_vlan10 proto tcp from any to (igb0_vlan10) port = 3128 flags S/SA keep state
pass in quick on igb0_vlan88 proto tcp from any to (igb0_vlan88) port = 3128 flags S/SA keep state
pass in quick on lo0 proto tcp from any to (lo0) port = 3128 flags S/SA keep state
pass in quick on igb0_vlan10 proto tcp from any to (igb0_vlan10) port = 3129 flags S/SA keep state
pass in quick on igb0_vlan88 proto tcp from any to (igb0_vlan88) port = 3129 flags S/SA keep state
pass in quick on lo0 proto tcp from any to (lo0) port = 3129 flags S/SA keep state

Also added some comments to the individual changes below.

doktornotor · 2017-02-03T16:06:20Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

-		$PPTP_ALIAS = "\$pptp";
-	} else {
-		$PPTP_ALIAS = "\$PPTP";
-	}


Removed some unused PPTP leftovers.

doktornotor · 2017-02-03T16:09:00Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 	if (strstr($fw_aliases, "PPPoE =")) {
 		$PPPOE_ALIAS = "\$PPPoE";
 	} else {
 		$PPPOE_ALIAS = "\$pppoe";
 	}

 	// define ports based on transparent options and ssl filtering
-	$pf_rule_port = ($squid_conf['ssl_proxy'] == "on" ? "{80,443}" : "80");
+	$pf_nat_ports = ($squid_conf['ssl_proxy'] == "on" ? "{80,443}" : "80");
+	$pf_rule_ports = "{{$port},{$ssl_port}}";


$pf_nat_ports is for transparent proxy

$pf_rule_ports is for the defined proxy interfaces (nontransparent) - 3128/3129 by default

(with, or without HTTPS/SSL Interception)

$pf_transparent_rule_port - always 80, plus 443 if the interface is selected under SSL Intercept Interface(s)

doktornotor · 2017-02-03T16:10:12Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				if (in_array($t_iface, $ssl_ifaces)) {
-					$rules .= "rdr on $t_iface proto tcp from any to !($t_iface) port 443 -> 127.0.0.1 port {$ssl_port}\n";
+					$rules .= "rdr pass on $t_iface proto tcp from any to !($t_iface) port 443 -> 127.0.0.1 port {$ssl_port}\n";


Use rdr pass to pass traffic destined to 443 to the transparent proxy

doktornotor · 2017-02-03T16:10:21Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				}
 			}
 			foreach ($transparent_ifaces as $t_iface) {
-				$pf_transparent_rule_port = (in_array($t_iface, $ssl_ifaces) ? "{80,443}" : "80");
-				$rules .= "rdr on $t_iface proto tcp from any to !($t_iface) port 80 -> 127.0.0.1 port {$port}\n";
+				$rules .= "rdr pass on $t_iface proto tcp from any to !($t_iface) port 80 -> 127.0.0.1 port {$port}\n";


Use rdr pass to pass traffic destined to 80 to the transparent proxy

doktornotor · 2017-02-03T16:11:28Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				}
 			}
 			/* Handle PPPOE case */
 			if (($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) || (function_exists("is_pppoe_server_enabled") && is_pppoe_server_enabled())) {
-				$rules .= "rdr on $PPPOE_ALIAS proto tcp from any to !127.0.0.1 port {$pf_rule_port} -> 127.0.0.1 port {$port}\n";


Well, this is really WTF. No idea what it was supposed to do, plus why limited to the HTTP proxy port only?

It's for local clients that connected using the PPPoE server. Not sure why it's HTTP only but it's necessary so they could be proxied. The way mpd works it uses a bunch of different local interfaces in a group. There is some code higher up that sets PPPOE_ALIAS conditionally that is no longer needed. It's only $pppoe now.

OK, re-added that (below) with the !127.0.0.1 destination, it also does HTTPS now when SSL proxy is enabled, not just HTTP.

doktornotor · 2017-02-03T16:12:08Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				$rules .= "# Setup squid pass rules for proxy\n";
-				$rules .= "pass in quick on $iface proto tcp from any to !($iface) port {$pf_transparent_rule_port} flags S/SA keep state\n";
-				// $rules .= "pass in quick on $iface proto tcp from any to !($iface) port {$port} flags S/SA keep state\n";
+				$rules .= "pass in quick on $iface proto tcp from any to !127.0.0.1 port {$pf_rule_ports} flags S/SA keep state\n";


Pass traffic for the proxy ports (3128/3129 by default on each proxy interface)

doktornotor · 2017-02-03T16:13:20Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				$rules .= "\n";
 			}
 			if ($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) {
-				$rules .= "pass in quick on $PPPOE_ALIAS proto tcp from any to !127.0.0.1 port {$port} flags S/SA keep state\n";
+				$rules .= "pass in quick on $PPPOE_ALIAS proto tcp from any to !127.0.0.1 port {$pf_rule_ports} flags S/SA keep state\n";


Ditto for PPPoE.

doktornotor · 2017-02-03T16:15:12Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

@@ -2095,7 +2091,7 @@ function squid_generate_rules($type) {
 				}
 				/* Handle PPPOE case */
 				if (($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) || (function_exists("is_pppoe_server_enabled") && is_pppoe_server_enabled())) {
-					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from any to { 192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8 } port {$pf_rule_port}\n";
+					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from any to { 192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8 } port {$pf_nat_ports}\n";


Do not redirect these if 'Bypass Proxy for Private Address Destination' is checked.

doktornotor · 2017-02-03T16:15:43Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

@@ -2117,7 +2113,7 @@ function squid_generate_rules($type) {
 				}
 				/* Handle PPPOE case */
 				if (($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) || (function_exists("is_pppoe_server_enabled") && is_pppoe_server_enabled())) {
-					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from { $exempt_ip } to any port {$pf_rule_port}\n";
+					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from { $exempt_ip } to any port {$pf_nat_ports}\n";


Do not redirect 'Bypass Proxy for These Source IPs'

doktornotor · 2017-02-03T16:16:59Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

@@ -2139,33 +2135,33 @@ function squid_generate_rules($type) {
 				}
 				/* Handle PPPOE case */
 				if (($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) || (function_exists("is_pppoe_server_enabled") && is_pppoe_server_enabled())) {
-					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from any to { $exempt_dest } port {$pf_rule_port}\n";
+					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from any to { $exempt_dest } port {$pf_nat_ports}\n";


Do not redirect 'Bypass Proxy for These Destination IPs'

doktornotor · 2017-02-03T16:26:37Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				}
 			}
 			foreach ($transparent_ifaces as $t_iface) {
-				$pf_transparent_rule_port = (in_array($t_iface, $ssl_ifaces) ? "{80,443}" : "80");


Removed, was not used for anything here.

doktornotor · 2017-02-03T16:35:25Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 			}
 			$rules .= "\n";
 			break;
 		case 'filter':
 		case 'rule':
-			foreach ($transparent_ifaces as $iface) {
-				$pf_transparent_rule_port = (in_array($iface, $ssl_ifaces) ? "{80,443,{$port},{$ssl_port}}" : "{80,{$port}}");
+			foreach ($proxy_ifaces as $iface) {


This really is not about transparent proxy interfaces; rdr pass does the job there.

doktornotor · 2017-02-03T17:27:45Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				$rules .= "# Setup squid pass rules for proxy\n";
-				$rules .= "pass in quick on $iface proto tcp from any to !($iface) port {$pf_transparent_rule_port} flags S/SA keep state\n";
-				// $rules .= "pass in quick on $iface proto tcp from any to !($iface) port {$port} flags S/SA keep state\n";
+				$rules .= "pass in quick on $iface proto tcp from any to ($iface) port {$pf_rule_ports} flags S/SA keep state\n";


Pass traffic to the interface IP only

doktornotor · 2017-02-03T17:27:49Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

 				$rules .= "\n";
 			}
 			if ($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) {
-				$rules .= "pass in quick on $PPPOE_ALIAS proto tcp from any to !127.0.0.1 port {$port} flags S/SA keep state\n";
+				$rules .= "pass in quick on $PPPOE_ALIAS proto tcp from any to ($PPPOE_ALIAS) port {$pf_rule_ports} flags S/SA keep state\n";


Pass traffic to the interface IP only. I guess this should really be
$rules .= "pass in quick on $PPPOE_ALIAS proto tcp from any to {$config['pppoe']['localip']} port {$pf_rule_ports} flags S/SA keep state\n"; for PPPoE???

Given that it's a group I'm not certain how pf treats using that as a destination. Using the configured local IP address would be safer.

So, you mean something like

pass in quick on $pppoe proto tcp from any to ($pppoe) port {$pf_rule_ports} ...

?

I'd say this is safer:
pass in quick on $pppoe proto tcp from any to {$config['pppoe']['localip']} port {$pf_rule_ports}
But I'd have to try it out.

Yeah I really need someone with a working PPPoE setup to test whatever is figured out here. So, to summarize so far:

nuke the weird $PPPOE_ALIAS everywhere and use $pppoe instead

use {$config['pppoe']['localip']} as destination

Right?

That looks right.
I have two VMs with PPPoE server setup but they're acting as "WAN" connections for other pfSense VMs so I can test PPPoE WANs, it's easy to setup and test, there's not much to it. Should work just as easy for clients as those.

doktornotor · 2017-02-03T20:39:35Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

+				$rules .= "rdr pass on $PPPOE_ALIAS proto tcp from any to !($PPPOE_ALIAS) port 80 -> 127.0.0.1 port {$port}\n";
+				if ($squid_conf['ssl_proxy'] == "on") {
+					$rules .= "rdr pass on $t_iface proto tcp from any to !($PPPOE_ALIAS) port 443 -> 127.0.0.1 port {$ssl_port}\n";
+				}


I guess this should really be
$rules .= "rdr pass on $PPPOE_ALIAS proto tcp from any to !{$config['pppoe']['localip']} port ...
for both PPPoE lines above???

jim-p · 2017-02-07T15:27:25Z

Is the "old rules" example above right? You show rdr pass in there already.

doktornotor · 2017-02-07T15:35:49Z

Kinda unsure what's the question. No, there's no rdr pass currently in Squid.

jim-p · 2017-02-07T15:37:28Z

In the initial comment on the PR you put two boxes of rules "comparison of rules generated by the old and new code", the one labeled "Old rules" has rdr pass lines in it.

doktornotor · 2017-02-07T15:38:56Z

Eh... probably pasted something wrong there. The new ones is correct, cannot check any old ones at the moment. There's no rdr pass anywhere there, though.

jim-p · 2017-02-07T15:40:56Z

That's what I figured, but I wanted to be certain.

doktornotor · 2017-02-07T16:05:38Z

Yeah, I'll fix that part as soon as I can revert the file somewhere and re-check, but the main point of why I started investigating this was that the pass rules created are way too permissive, they just basically let HTTP/HTTPS pass through altogether except if the proxy interface is the destination, and that's impossible to override by normal firewall rules since it's a quick one. I mean this line:

https://github.com/pfsense/FreeBSD-ports/blob/devel/www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc#L2163

doktornotor · 2017-02-07T17:01:27Z

@jim-p - Fixed the "old rules" in the first comment after reverting to squid.inc from 0.4.35_3; so yeah

there's no rdr pass anywhere.
for transparent interfaces, it's passing (with quick) 80/443 (plus 3128/3129) to everywhere except the proxy interface IP for god knows what reason.
for non-transparent ones, it's not even passing the required traffic to proxy (just a non-issue for most users due to the widely permissive rules on LANs, usually allow any).

rbgarga · 2017-02-08T09:49:14Z

www/pfSense-pkg-squid/files/usr/local/pkg/squid.inc

@@ -2095,7 +2084,7 @@ function squid_generate_rules($type) {
 				}
 				/* Handle PPPOE case */
 				if (($config['pppoe']['mode'] == "server" && $config['pppoe']['localip']) || (function_exists("is_pppoe_server_enabled") && is_pppoe_server_enabled())) {
-					$rules .= "no rdr on $PPPOE_ALIAS proto tcp from any to { 192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8 } port {$pf_rule_port}\n";
+					$rules .= "no rdr on $pppoe proto tcp from any to { 192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8 } port {$pf_nat_ports}\n";


I didn't see where $pppoe is being defined

@rbgarga - the changes here were done as suggested by @jim-p (#289 (comment)) - as I've noted, I don't have any such setup to test, and generally this is something completely outside of my knowledge. Perhaps this should be \$pppoe instead, NFC.

It's not a PHP variable but a pf macro that will be in the ruleset already if the pppoe server is enabled (which the if condition above tests for).

So you want that escaped?

Yeah, it needs to be escaped

OK, done... I guess we are slowly getting there.

(I'd prefer to do a new PR when we are done here, there's bunch of typo fixes to be done in the package as well, plus no need to have this kind of messing in git history).

doktornotor · 2017-02-14T10:29:19Z

Closing in favor of #305 - all review comments are included there, plus additional fixes.

[ Robert Edmonds ] * Release 1.3.1. * Restore protobuf-2.x compatibility (#284, #285). * Use xenial and protobuf 3.6.1 in the Travis-CI environment (#332). * Convert uses of protobuf's scoped_ptr.h to C++11 std::unique_ptr, needed to compile against protobuf 3.6.1 (#320, #333). * Use AX_CXX_COMPILE_STDCXX macro to enable C++11 support in old compilers (#312, #317, #327, #334). [ Fredrik Gustafsson ] * Add std:: to some types (#294, #305, #309). [ Sam Collinson ] * Check the return value of int_range_lookup before using as an array index; it can return -1 (#315). [ Matthias Dittrich ] * Fix compilation on mingw by using explicit protoc --plugin=NAME=PATH syntax in Makefile.am (#289, #290). Sponsored by: Farsight Security, Inc.

Squid - firewall rules sanitization

322901e

netgate-git-updates added the CLA label Feb 3, 2017

doktornotor commented Feb 3, 2017

View reviewed changes

Pass traffic to the interface IP only

affb9d6

doktornotor commented Feb 3, 2017

View reviewed changes

rbgarga requested review from rbgarga and jim-p February 7, 2017 11:17

doktornotor added 2 commits February 7, 2017 17:24

Improve firewall rules handling for PPPoE server and fix the HTTPS rule

bb33ec2

Fix the transparent rules for PPPoE

eccdc7c

rbgarga reviewed Feb 8, 2017

View reviewed changes

Escape the $pppoe pf macro to avoid it being replaced by PHP variable

c0dd60f

doktornotor closed this Feb 14, 2017

doktornotor deleted the patch-3 branch February 14, 2017 10:29

Squid - firewall rules sanitization #289

Squid - firewall rules sanitization #289

Conversation

doktornotor commented Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

doktornotor Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

jim-p commented Feb 7, 2017

doktornotor commented Feb 7, 2017

jim-p commented Feb 7, 2017

doktornotor commented Feb 7, 2017

jim-p commented Feb 7, 2017

doktornotor commented Feb 7, 2017 • edited Loading

doktornotor commented Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

doktornotor Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor commented Feb 14, 2017

doktornotor commented Feb 3, 2017 •

edited

Loading

doktornotor Feb 3, 2017 •

edited

Loading

doktornotor Feb 3, 2017 •

edited

Loading

doktornotor Feb 7, 2017 •

edited

Loading

doktornotor Feb 3, 2017 •

edited

Loading

doktornotor Feb 3, 2017 •

edited

Loading

doktornotor commented Feb 7, 2017 •

edited

Loading

doktornotor commented Feb 7, 2017 •

edited

Loading

doktornotor Feb 8, 2017 •

edited

Loading