ipfilter: Verify frentry on entry into kernel

The frentry struct is built by ipf(8), specifically ipf_y.y when parsing
the ipfilter configuration file (typically ipf.conf). frentry contains
a variable length string field at the end of the struct. This data field,
called fr_names, may contain various text strings such as NIC names,
destination list (dstlist) names, and filter rule comments.  The length
field specifies the length of fr_names within the frentry structure and
fr_size specifies the size of the frentry structure itself.

The upper bound limit to the length of strings field is controlled by the
fr_max_namelen sysctl/kenv or the max_namelen ipfilter tuneable.

The initial concepts were discussed with emaste and jrm.

Reported by:		Ilja Van Sprundel <[email protected]>
Reviewed by:		markj
MFC after:		1 week
Differential revision:	https://reviews.freebsd.org/D53843

sbin/ipf/libipf/interror.c

@@ -177,6 +177,11 @@ static ipf_error_entry_t ipf_errors[] = {
 	{	149,	"object size validation failed for kernel copyout" },
 	{	150,	"error copying data out for kernel copyout" },
 	{	151,	"version mismatch for kernel copyout" },
+	{	152,	"fr_names offset is wrapped negative" },
+	{	153,	"fr_names larger than fr_namelen" },
+	{	154,	"frentry larger than fr_size" },
+	{	155,	"frentry and fr_namelen mismatch fr_size" },
+	{	156,	"fr_namelen too large" },
 /* -------------------------------------------------------------------------- */
 	{	10001,	"could not find token for auth iterator" },
 	{	10002,	"write permissions require to add/remove auth rule" },

sys/netpfil/ipfilter/netinet/fil.c

@@ -363,6 +363,10 @@ static ipftuneable_t ipf_main_tuneables[] = {
 		"ip_timeout",		1,	0x7fffffff,
 		stsizeof(ipf_main_softc_t, ipf_iptimeout),
 		0,			NULL,	ipf_settimeout },
+	{ { (void *)offsetof(ipf_main_softc_t, ipf_max_namelen) },
+		"max_namelen",		0,	0x7fffffff,
+		stsizeof(ipf_main_softc_t, ipf_max_namelen),
+		0,			NULL,	NULL },
 #if defined(INSTANCES) && defined(_KERNEL)
 	{ { (void *)offsetof(ipf_main_softc_t, ipf_get_loopback) },
 		"intercept_loopback",	0,	1,
@@ -4399,7 +4403,8 @@ int
 frrequest(ipf_main_softc_t *softc, int unit, ioctlcmd_t req, caddr_t data,
 	int set, int makecopy)
 {
-	int error = 0, in, family, need_free = 0;
+	int error = 0, in, family, need_free = 0, interr, i;
+	int interr_tbl[3] = { 152, 156, 153};
 	enum {	OP_ADD,		/* add rule */
 		OP_REM,		/* remove rule */
 		OP_ZERO 	/* zero statistics and counters */ }
@@ -4408,7 +4413,9 @@ frrequest(ipf_main_softc_t *softc, int unit, ioctlcmd_t req, caddr_t data,
 	void *ptr, *uptr;
 	u_int *p, *pp;
 	frgroup_t *fg;
-	char *group;
+	char *group, *name;
+	size_t v_fr_size, v_element_size;
+	int v_rem_namelen, v_fr_toend;
 
 	ptr = NULL;
 	fg = NULL;
@@ -4423,6 +4430,17 @@ frrequest(ipf_main_softc_t *softc, int unit, ioctlcmd_t req, caddr_t data,
 			IPFERROR(6);
 			return (EINVAL);
 		}
+		if (fp->fr_size < sizeof(frd)) {
+			return (EINVAL);
+		}
+		if (sizeof(frd) + fp->fr_namelen != fp->fr_size ) {
+			IPFERROR(155);
+			return (EINVAL);
+		}
+		if (fp->fr_namelen < 0 || fp->fr_namelen > softc->ipf_max_namelen) {
+			IPFERROR(156);
+			return (EINVAL);
+		}
 		KMALLOCS(f, frentry_t *, fp->fr_size);
 		if (f == NULL) {
 			IPFERROR(131);
@@ -4449,6 +4467,44 @@ frrequest(ipf_main_softc_t *softc, int unit, ioctlcmd_t req, caddr_t data,
 		fp->fr_ptr = NULL;
 		fp->fr_ref = 0;
 		fp->fr_flags |= FR_COPIED;
+
+		for (i = 0; i <= 3; i++) {
+			if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_ifnames[i])) != 0) {
+				IPFERROR(interr_tbl[interr-1]);
+				error = EINVAL;
+				goto donenolock;
+			}
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_comment)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_group)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_grhead)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_tif.fd_name)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_rif.fd_name)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
+		if ((interr = ipf_check_names_string(fp->fr_names, fp->fr_namelen, fp->fr_dif.fd_name)) != 0) {
+			IPFERROR(interr_tbl[interr-1]);
+			error = EINVAL;
+			goto donenolock;
+		}
 	} else {
 		fp = (frentry_t *)data;
 		if ((fp->fr_type & FR_T_BUILTIN) == 0) {
@@ -9040,6 +9096,7 @@ ipf_main_soft_create(void *arg)
 #endif
 	softc->ipf_minttl = 4;
 	softc->ipf_icmpminfragmtu = 68;
+	softc->ipf_max_namelen = 128;
 	softc->ipf_flags = IPF_LOGGING;
 
 #ifdef LARGE_NAT

sys/netpfil/ipfilter/netinet/ip_fil.h

@@ -1529,6 +1529,7 @@ typedef struct ipf_main_softc_s {
 	int		ipf_pass;
 	int		ipf_minttl;
 	int		ipf_icmpminfragmtu;
+	int		ipf_max_namelen;
 	int		ipf_interror;	/* Should be in a struct that is per  */
 					/* thread or process. Does not belong */
 					/* here but there's a lot more work   */

sys/netpfil/ipfilter/netinet/mlfk_ipl.c

@@ -135,6 +135,7 @@ SYSCTL_IPF(_net_inet_ipf, OID_AUTO, fr_running, CTLFLAG_RD,
 SYSCTL_IPF(_net_inet_ipf, OID_AUTO, fr_chksrc, CTLFLAG_RW, &VNET_NAME(ipfmain.ipf_chksrc), 0, "");
 SYSCTL_IPF(_net_inet_ipf, OID_AUTO, fr_minttl, CTLFLAG_RW, &VNET_NAME(ipfmain.ipf_minttl), 0, "");
 SYSCTL_IPF(_net_inet_ipf, OID_AUTO, large_nat, CTLFLAG_RDTUN | CTLFLAG_NOFETCH, &VNET_NAME(ipfmain.ipf_large_nat), 0, "large_nat");
+SYSCTL_IPF(_net_inet_ipf, OID_AUTO, fr_max_namelen, CTLFLAG_RWTUN, &VNET_NAME(ipfmain.ipf_max_namelen), 0, "max_namelen");
 
 #define CDEV_MAJOR 79
 #include <sys/poll.h>