[master] b9756475f Warn about ACL entries with non-zero host bits

[Nils Goroll]

commit b9756475fa0ca69e0a3a31b919a6f26141c30f79
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Thu Dec 12 20:32:53 2019 +0100

    Warn about ACL entries with non-zero host bits
    
    Summary:
    
    ACL entries with netmasks shorter than the maximum for the respective
    protocol represent network addresses and as such, by convention,
    should be written with all zero bits in the host part to avoid
    confusion.
    
    This patch adds VCL compile warnings and improved logging if they are
    not.
    
    Discussion:
    
    For example, while 1.2.3.0/24 and 1.2.3.255/24, in CIDR notation, both
    specify all addresses with the first three octets matching 1, 2 and 3,
    using the latter can be a source of subtle confusion.
    
    This becomes particularly apparent with netmasks outside byte
    boundaries: 1.2.6.0/22 specifies addresses 1.2.4.0 - 1.2.7.255, but
    not so experienced administrators might be tempted to think that it
    specified 1.2.6.0 - 1.2.9.255.
    
    To summarize, denoting network addresses in non-canonical form is
    confusing, a possible source of error and additionally complicates
    analyses.
    
    This patch makes sure that such mishaps do not remain unnoticed by
    
    - issuing warnings during VCL compilation about non-canonical network
      addresses
    
    - Logging ACL matches together with the canonical address
    
    The actual matching code is not touched, but a minor simplification
    can be applied later.

diff --git a/bin/varnishtest/tests/c00005.vtc b/bin/varnishtest/tests/c00005.vtc
index 92ef9f9f2..0eed7ef55 100644
--- a/bin/varnishtest/tests/c00005.vtc
+++ b/bin/varnishtest/tests/c00005.vtc
@@ -115,11 +115,11 @@ varnish v1 -vcl {
 
 logexpect l1 -v v1 -g raw {
 	expect * 1007	ReqHeader	{^\Qip: 1.2.3.0\E$}
-	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.4"/24\E$}
+	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.4"/24 fixed: 1.2.3.0/24\E$}
 	expect 1 =	ReqHeader	{^\Qip: 1.2.3.63\E$}
-	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.4"/24\E$}
+	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.4"/24 fixed: 1.2.3.0/24\E$}
 	expect 1 =	ReqHeader	{^\Qip: 1.2.3.64\E$}
-	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.66"/26\E$}
+	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.2.3.66"/26 fixed: 1.2.3.64/26\E$}
 
 	expect 1 =	ReqHeader	{^\Qip: 1.3.4.255\E$}
 	expect 0 =	VCL_acl	{^\QMATCH acl1 "1.3.4.0"/23\E$}
diff --git a/lib/libvcc/vcc_acl.c b/lib/libvcc/vcc_acl.c
index cf18107d7..443d9189e 100644
--- a/lib/libvcc/vcc_acl.c
+++ b/lib/libvcc/vcc_acl.c
@@ -40,6 +40,8 @@
 #include <string.h>
 
 #include "vcc_compile.h"
+#include <vtcp.h>
+#include <vsa.h>
 
 #define ACL_MAXADDR	(sizeof(struct in6_addr) + 1)
 
@@ -50,6 +52,7 @@ struct acl_e {
 	unsigned		not;
 	unsigned		para;
 	char			*addr;
+	char			*fixed;
 	struct token		*t_addr;
 	struct token		*t_mask;
 };
@@ -91,10 +94,60 @@ vcl_acl_cmp(struct acl_e *ae1, struct acl_e *ae2)
 	return (0);
 }
 
+static char *
+vcc_acl_chk(struct vcc *tl, const struct acl_e *ae, const int l,
+    unsigned char *p, int fam)
+{
+	const unsigned char *u;
+	char h[VTCP_ADDRBUFSIZE];
+	char t[VTCP_ADDRBUFSIZE + 10];
+	char s[vsa_suckaddr_len];
+	struct suckaddr *sa;
+	unsigned m;
+	int ll, ret = 0;
+
+	u = p;
+	ll = l;
+	m = ae->mask;
+
+	p += m / 8;
+	ll -= m / 8;
+	assert (ll >= 0);
+	m %= 8;
+
+	if (m && (*p << m & 0xff) != 0) {
+		ret = 1;
+		m = 0xff00 >> m;
+		*p &= m;
+	}
+	if (m) {
+		p++;
+		ll--;
+	}
+
+	for ( ; ll > 0; p++, ll--) {
+		if (*p == 0)
+			continue;
+		ret = 1;
+		*p = 0;
+	}
+	if (ret == 0)
+		return (NULL);
+
+	sa = VSA_BuildFAP(s, fam, u, l, NULL, 0);
+	AN(sa);
+	VTCP_name(sa, h, sizeof h, NULL, 0);
+	bprintf(t, "%s/%d", h, ae->mask);
+	VSB_printf(tl->sb, "Address/Netmask mismatch, changed to %s\n", t);
+	vcc_ErrWhere(tl, ae->t_addr);
+	vcc_Warn(tl);
+	return (strdup(t));
+}
+
 
 static void
 vcc_acl_add_entry(struct vcc *tl, const struct acl_e *ae, int l,
-    const unsigned char *u, int fam)
+    unsigned char *u, int fam)
 {
 	struct acl_e *ae2, *aen;
 	int i;
@@ -120,6 +173,8 @@ vcc_acl_add_entry(struct vcc *tl, const struct acl_e *ae, int l,
 	AN(aen);
 	*aen = *ae;
 
+	aen->fixed = vcc_acl_chk(tl, ae, l, u, fam);
+
 	/* We treat family as part of address, it saves code */
 	assert(fam <= 0xff);
 	aen->data[0] = fam & 0xff;
@@ -410,6 +465,7 @@ vcc_acl_emit(struct vcc *tl, const char *name, const char *rname, int anon)
 		}
 
 		if (m > 0) {
+			// XXX can remove masking due to fixup
 			/* Do fractional byte compares */
 			Fh(tl, 0, "\t%*s%sif ((a[%d] & 0x%x) == %d) {\n",
 			    -i, "", "", i - 1, (0xff00 >> m) & 0xff,
@@ -436,6 +492,9 @@ vcc_acl_emit(struct vcc *tl, const char *name, const char *rname, int anon)
 				t = VTAILQ_NEXT(t, list);
 				AN(t);
 			} while (ae->t_mask != NULL);
+			if (ae->fixed)
+				Fh(tl, 0, "\" fixed: %s\"",
+				   ae->fixed);
 			Fh(tl, 0, ");\n");
 		}