From: tb <tb@openbsd.org>
Date: Thu, 14 Dec 2023 07:52:53 +0000 (+0000)
Subject: rpki-client: make IP address block checks stricter
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=de494ec361d1c2bf7497d6f9ed4f183691607a42;p=openbsd

rpki-client: make IP address block checks stricter

There are only two valid AFIs in this context, so check that we have one
or two of them. We only accept the IPv4 and IPv6 AFIs in ip_add_afi_parse()
and reject any SAFI, so enforce that neither AFI is repeated. This doesn't
change things for certificates, where all this is implied by other checks
combined. Making this explicit and match the logic needed for ROAs is a win.

looks good to job
ok claudio
---

diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c
index f9add47d32c..f695f8370a2 100644
--- a/usr.sbin/rpki-client/cert.c
+++ b/usr.sbin/rpki-client/cert.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: cert.c,v 1.120 2023/12/10 14:18:23 job Exp $ */
+/*	$OpenBSD: cert.c,v 1.121 2023/12/14 07:52:53 tb Exp $ */
 /*
  * Copyright (c) 2022 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2021 Job Snijders <job@openbsd.org>
@@ -362,11 +362,20 @@ sbgp_parse_ipaddrblk(const char *fn, const IPAddrBlocks *addrblk,
 	enum afi		 afi;
 	struct cert_ip		*ips = NULL;
 	size_t			 ipsz = 0, sz;
-	int			 i, j;
+	int			 ipv4_seen = 0, ipv6_seen = 0;
+	int			 i, j, ipaddrblocksz;
 
 	assert(*out_ips == NULL && *out_ipsz == 0);
 
-	for (i = 0; i < sk_IPAddressFamily_num(addrblk); i++) {
+	ipaddrblocksz = sk_IPAddressFamily_num(addrblk);
+	if (ipaddrblocksz != 1 && ipaddrblocksz != 2) {
+		warnx("%s: RFC 6487 section 4.8.10: unexpected number of "
+		    "ipAddrBlocks (got %d, expected 1 or 2)",
+		    fn, ipaddrblocksz);
+		goto out;
+	}
+
+	for (i = 0; i < ipaddrblocksz; i++) {
 		af = sk_IPAddressFamily_value(addrblk, i);
 
 		switch (af->ipAddressChoice->type) {
@@ -400,6 +409,23 @@ sbgp_parse_ipaddrblk(const char *fn, const IPAddrBlocks *addrblk,
 			goto out;
 		}
 
+		switch(afi) {
+		case AFI_IPV4:
+			if (ipv4_seen++ > 0) {
+				warnx("%s: RFC 6487 section 4.8.10: "
+				    "IPv4 appears twice", fn);
+				goto out;
+			}
+			break;
+		case AFI_IPV6:
+			if (ipv6_seen++ > 0) {
+				warnx("%s: RFC 6487 section 4.8.10: "
+				    "IPv6 appears twice", fn);
+				goto out;
+			}
+			break;
+		}
+
 		if (aors == NULL) {
 			if (!sbgp_addr_inherit(fn, ips, &ipsz, afi))
 				goto out;
diff --git a/usr.sbin/rpki-client/roa.c b/usr.sbin/rpki-client/roa.c
index 93838d120a0..6e19ca7c53a 100644
--- a/usr.sbin/rpki-client/roa.c
+++ b/usr.sbin/rpki-client/roa.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: roa.c,v 1.71 2023/10/13 12:06:49 job Exp $ */
+/*	$OpenBSD: roa.c,v 1.72 2023/12/14 07:52:53 tb Exp $ */
 /*
  * Copyright (c) 2022 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -104,7 +104,7 @@ roa_parse_econtent(const unsigned char *d, size_t dsz, struct parse *p)
 	RouteOriginAttestation		*roa;
 	const ROAIPAddressFamily	*addrfam;
 	const STACK_OF(ROAIPAddress)	*addrs;
-	int				 addrsz;
+	int				 addrsz, ipv4_seen = 0, ipv6_seen = 0;
 	enum afi			 afi;
 	const ROAIPAddress		*addr;
 	uint64_t			 maxlen;
@@ -129,8 +129,8 @@ roa_parse_econtent(const unsigned char *d, size_t dsz, struct parse *p)
 	}
 
 	ipaddrblocksz = sk_ROAIPAddressFamily_num(roa->ipAddrBlocks);
-	if (ipaddrblocksz > 2) {
-		warnx("%s: draft-rfc6482bis: too many ipAddrBlocks "
+	if (ipaddrblocksz != 1 && ipaddrblocksz != 2) {
+		warnx("%s: draft-rfc6482bis: unexpected number of ipAddrBlocks "
 		    "(got %d, expected 1 or 2)", p->fn, ipaddrblocksz);
 		goto out;
 	}
@@ -146,6 +146,29 @@ roa_parse_econtent(const unsigned char *d, size_t dsz, struct parse *p)
 			goto out;
 		}
 
+		switch(afi) {
+		case AFI_IPV4:
+			if (ipv4_seen++ > 0) {
+				warnx("%s: RFC 6482bis section 4.3.2: "
+				    "IPv4 appears twice", p->fn);
+				goto out;
+			}
+			break;
+		case AFI_IPV6:
+			if (ipv6_seen++ > 0) {
+				warnx("%s: RFC 6482bis section 4.3.2: "
+				    "IPv6 appears twice", p->fn);
+				goto out;
+			}
+			break;
+		}
+
+		if (addrsz == 0) {
+			warnx("%s: RFC 6482bis, section 4.3.2: "
+			    "empty ROAIPAddressFamily", p->fn);
+			goto out;
+		}
+
 		if (p->res->ipsz + addrsz >= MAX_IP_SIZE) {
 			warnx("%s: too many ROAIPAddress entries: limit %d",
 			    p->fn, MAX_IP_SIZE);