rfxn / advanced-policy-firewall

Advanced Policy Firewall (APF)
GNU General Public License v2.0
93 stars 46 forks source link

DNS firewall logic mistake with RESV_DNS_DROP enabled? #25

Closed Jas0n99 closed 5 years ago

Jas0n99 commented 5 years ago

In the firewall file, there is the following code:

# DNS
if [ -f "/etc/resolv.conf" ] && [ "$RESV_DNS" == "1" ]; then
LDNS=`cat /etc/resolv.conf  | grep -v "#" | grep -w nameserver | awk '{print$2}' | grep -v 127.0.0.1`
  if [ ! "$LDNS" == "" ]; then
        for i in `echo $LDNS`; do
        eout "{glob} resolv dns discovery for $i"
        $IPT -A INPUT -p udp -s $i --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A INPUT -p tcp -s $i --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p udp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p tcp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
        if [ "$RESV_DNS_DROP" == "1" ]; then
                $IPT -A INPUT  -p tcp -s 0/0 --sport 53 --dport 1023:65535 -j $ALL_STOP
                $IPT -A INPUT  -p udp -s 0/0 --sport 53 --dport 1023:65535 -j $ALL_STOP
                $IPT -A OUTPUT -p udp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
                $IPT -A OUTPUT -p tcp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
        fi
        done
  fi
else
        $IPT -A INPUT  -p udp --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A INPUT  -p tcp --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p udp --dport 53 --sport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p tcp --dport 53 --sport 1023:65535 -j ACCEPT
fi

However, with multiple DNS servers the INPUT/OUTPUT chains end up kind funky and I think only the first DNS server in the list would actually function...

If you have 2 DNS IPs.... Let's call them 1.1.1.1 & 2.2.2.2 your INPUT would look like (edited for brevity):

ACCEPT udp 1.1.1.1 0.0.0.0/0 udp spt:53 dpts:1023:65535 ACCEPT tcp 1.1.1.1 0.0.0.0/0 udp spt:53 dpts:1023:65535 DROP udp 0.0.0.0/0 0.0.0.0/0 udp spt:53 dpts:1023:65535 DROP tcp 0.0.0.0/0 0.0.0.0/0 udp spt:53 dpts:1023:65535 ACCEPT udp 2.2.2.2 0.0.0.0/0 udp spt:53 dpts:1023:65535 ACCEPT tcp 2.2.2.2 0.0.0.0/0 udp spt:53 dpts:1023:65535 DROP udp 0.0.0.0/0 0.0.0.0/0 udp spt:53 dpts:1023:65535 DROP tcp 0.0.0.0/0 0.0.0.0/0 udp spt:53 dpts:1023:65535

The first pair of DROP's would prevent any traffic being accepted from 2.2.2.2

On the OUTPUT side, the two OUTPUT lines in the RESV_DNS_DROP seem to be identical / redundant to the block right above them.

I believe the correct way (at least it's how I changed it in my code) would be as follows. Simply moving the done loop above the RESV_DNS_DROP if statement, and removing the two duplicate rows. This puts a single pair of DROP rules AFTER all the valid DNS server IPs have been listed to ACCEPT.

# DNS
if [ -f "/etc/resolv.conf" ] && [ "$RESV_DNS" == "1" ]; then
LDNS=`cat /etc/resolv.conf  | grep -v "#" | grep -w nameserver | awk '{print$2}' | grep -v 127.0.0.1`
  if [ ! "$LDNS" == "" ]; then
        for i in `echo $LDNS`; do
        eout "{glob} resolv dns discovery for $i"
        $IPT -A INPUT -p udp -s $i --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A INPUT -p tcp -s $i --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p udp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p tcp -d $i --dport 53 --sport 1023:65535 -j ACCEPT
        done
        if [ "$RESV_DNS_DROP" == "1" ]; then
                $IPT -A INPUT  -p tcp -s 0/0 --sport 53 --dport 1023:65535 -j $ALL_STOP
                $IPT -A INPUT  -p udp -s 0/0 --sport 53 --dport 1023:65535 -j $ALL_STOP
        fi
  fi
else
        $IPT -A INPUT  -p udp --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A INPUT  -p tcp --sport 53 --dport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p udp --dport 53 --sport 1023:65535 -j ACCEPT
        $IPT -A OUTPUT -p tcp --dport 53 --sport 1023:65535 -j ACCEPT
fi
rfxn commented 5 years ago

Thank you for the feedback and input. You sir are absolutely correct, fixed in master commit 6b4027b.

Thanks!