OpenBSD PF Little-Endian Address Ranges Mishandling

For some reason this – in my opinion – important vulnerability did not take any attention. This was publised by the OpenBSD Project on 16 February 2011 as a security fix. The exact code is available at src/sys/net/pf.c that you can see here.

/*
 * Return 1 if b <= a <= e, otherwise return 0.
 */
int
pf_match_addr_range(struct pf_addr *b, struct pf_addr *e,
    struct pf_addr *a, sa_family_t af)
{
	switch (af) {
#ifdef INET
	case AF_INET:
		if ((a->addr32[0] < b->addr32[0]) ||
		    (a->addr32[0] > e->addr32[0]))
			return (0);
		break;
#endif /* INET */
#ifdef INET6
	case AF_INET6: {
		int	i;

		/* check a >= b */
		for (i = 0; i < 4; ++i)
			if (a->addr32[i] > b->addr32[i])
				break;
			else if (a->addr32[i] < b->addr32[i])
				return (0);
		/* check a <= e */
		for (i = 0; i < 4; ++i)
			if (a->addr32[i] < e->addr32[i])
				break;
			else if (a->addr32[i] > e->addr32[i])
				return (0);
		break;
	}
#endif /* INET6 */
	}
	return (1);
}

In the AF_INET case above you can clearly see that the checks do not parse the address’ parts through ntohl() (network to host 32-bit unsigned integer) in order to have correct bit ordering (aka endianness). Due to this mistake, PF rules containing address ranges such as “10.1.1.1 – 10.1.1.5″ were not handled correctly on Little-Endian archictures which in the case of OpenBSD, the supported are:
- DEC Alpha
- AMD64
- ARM
- i386
- MIPS64EL
- VAX

To fix this bug, the above routine was patched like this:

@@ -2215,8 +2215,8 @@ pf_match_addr_range(struct pf_addr *b, s
 	switch (af) {
 #ifdef INET
 	case AF_INET:
-		if ((a->addr32[0] < b->addr32[0]) ||
-		    (a->addr32[0] > e->addr32[0]))
+		if ((ntohl(a->addr32[0]) < ntohl(b->addr32[0])) ||
+		    (ntohl(a->addr32[0]) > ntohl(e->addr32[0])))
 			return (0);
 		break;
 #endif /* INET */
@@ -2226,15 +2226,15 @@ pf_match_addr_range(struct pf_addr *b, s
 
 		/* check a >= b */
 		for (i = 0; i < 4; ++i)
-			if (a->addr32[i] > b->addr32[i])
+			if (ntohl(a->addr32[i]) > ntohl(b->addr32[i]))
 				break;
-			else if (a->addr32[i] < b->addr32[i])
+			else if (ntohl(a->addr32[i]) < ntohl(b->addr32[i]))
 				return (0);
 		/* check a <= e */
 		for (i = 0; i < 4; ++i)
-			if (a->addr32[i] < e->addr32[i])
+			if (ntohl(a->addr32[i]) < ntohl(e->addr32[i]))
 				break;
-			else if (a->addr32[i] > e->addr32[i])
+			else if (ntohl(a->addr32[i]) > ntohl(e->addr32[i]))
 				return (0);
 		break;
 	}

About these ads