Hi,

> On 25 Sep 2019, at 20:45, peter.mueller(a)ipfire.org wrote:
> 
> Allowing outgoing DNS traffic (destination port 53, both TCP
> and UDP) to the root servers is BCP for some reasons. First,
> RFC 5011 assumes resolvers are able to fetch new trust ancors
> from the root servers for a certain time period in order to
> do key rollovers.
> 
> Second, Unbound shows some side effects if it cannot do trust
> anchor signaling (see RFC 8145) or fetch the current trust anchor,
> resulting in SERVFAILs for arbitrary requests a few minutes.
> 
> There is little security implication of allowing DNS traffic
> to the root servers: An attacker might abuse this for exfiltrating
> data via DNS queries, but is unable to infiltrate data unless
> he gains control over at least one root server instance. If
> there is no firewall ruleset in place which prohibits any other
> DNS traffic than to chosen DNS servers, this patch will not
> have security implications at all.

I think we need to document this on the wiki before we merge this patch.

> Fixes #12183
> 
> Cc: Michael Tremer <michael.tremer(a)ipfire.org>
> Suggested-by: Horace Michael <horace.michael(a)gmx.com>
> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
> ---
> config/rootfiles/core/137/filelists/files |  1 +
> src/initscripts/system/firewall           | 16 ++++++++++++++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/config/rootfiles/core/137/filelists/files b/config/rootfiles/core/137/filelists/files
> index ce4e51768..a02840d12 100644
> --- a/config/rootfiles/core/137/filelists/files
> +++ b/config/rootfiles/core/137/filelists/files
> @@ -1,4 +1,5 @@
> etc/system-release
> etc/issue
> +etc/rc.d/init.d/firewall
> srv/web/ipfire/cgi-bin/credits.cgi
> var/ipfire/langs
> diff --git a/src/initscripts/system/firewall b/src/initscripts/system/firewall
> index ec396c708..ff63a2ede 100644
> --- a/src/initscripts/system/firewall
> +++ b/src/initscripts/system/firewall
> @@ -6,10 +6,11 @@
> eval $(/usr/local/bin/readhash /var/ipfire/ppp/settings)
> eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
> eval $(/usr/local/bin/readhash /var/ipfire/optionsfw/settings)
> -IFACE=`/bin/cat /var/ipfire/red/iface 2> /dev/null | /usr/bin/tr -d '\012'`
> +ROOTHINTS="/etc/unbound/root.hints"
> +IFACE=$( /bin/cat /var/ipfire/red/iface 2> /dev/null | /usr/bin/tr -d '\012' )
> 
> if [ -f /var/ipfire/red/device ]; then
> -	DEVICE=`/bin/cat /var/ipfire/red/device 2> /dev/null | /usr/bin/tr -d '\012'`
> +	DEVICE=$( /bin/cat /var/ipfire/red/device 2> /dev/null | /usr/bin/tr -d '\012' )
> fi

Why the added whitespace? Should have been an extra patch.

> 
> function iptables() {
> @@ -307,6 +308,17 @@ iptables_init() {
> 	iptables -A INPUT -j TOR_INPUT
> 	iptables -N TOR_OUTPUT
> 	iptables -A OUTPUT -j TOR_OUTPUT
> +
> +	# Allow outgoing DNS traffic (TCP and UDP) to DNS root servers
> +	ROOTSERVERIPS="$( awk '/\s+A\s+/ { print $4 }' ${ROOTHINTS} | xargs )"
> +	ipset -N root-servers iphash

ROOTSERVERIPS could have been an array and could have been local.

You do not need to call xargs. It is a rather expensive way to remove line breaks.

> +
> +	for ip in ${ROOTSERVERIPS}; do
> +		ipset add root-servers $ip
> +	done

It is also interesting that ipset does not allow to add more IP addresses in one go. This looks like a very expensive loop for a lot of IP addresses. I think this is fine here for about a dozen addresses, but importing a blacklist of thousands or tens of thousands of IP addresses will take a long time.

> +
> +	iptables -A OUTPUT -m set --match-set root-servers dst -p tcp --dport 53 -j ACCEPT
> +	iptables -A OUTPUT -m set --match-set root-servers dst -p udp --dport 53 -j ACCEPT
> 	
> 	# Jump into the actual firewall ruleset.
> 	iptables -N INPUTFW
> -- 
> 2.16.4

-Michael