cap_dns, cap_net: fix host and service buffer handling

If a malicious casper process sent a host or service string that was
too long, cap_getnameinfo would overrun the caller's buffer by one byte.

The backends for this function needlessly allocated one extra byte
for these buffers.  This was harmless, but could be confusing to readers.

Reported by:	Coverity (an internal run at Dell)
Reviewed by:	oshogbo, emaste
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
Differential Revision: https://reviews.freebsd.org/D39347

3 files changed, 51 insertions, 8 deletions

diff --git a/lib/libcasper/services/cap_dns/cap_dns.c b/lib/libcasper/services/cap_dns/cap_dns.c
index 85e5f41f052b..04ddf26633d2 100644
--- a/lib/libcasper/services/cap_dns/cap_dns.c
+++ b/lib/libcasper/services/cap_dns/cap_dns.c
@@ -302,9 +302,9 @@ cap_getnameinfo(cap_channel_t *chan, const struct sockaddr *sa, socklen_t salen,
 	}
 
 	if (host != NULL && nvlist_exists_string(nvl, "host"))
-		strlcpy(host, nvlist_get_string(nvl, "host"), hostlen + 1);
+		strlcpy(host, nvlist_get_string(nvl, "host"), hostlen);
 	if (serv != NULL && nvlist_exists_string(nvl, "serv"))
-		strlcpy(serv, nvlist_get_string(nvl, "serv"), servlen + 1);
+		strlcpy(serv, nvlist_get_string(nvl, "serv"), servlen);
 	nvlist_destroy(nvl);
 	return (0);
 }
@@ -538,14 +538,14 @@ dns_getnameinfo(const nvlist_t *limits, const nvlist_t *nvlin, nvlist_t *nvlout)
 	servlen = (size_t)nvlist_get_number(nvlin, "servlen");
 
 	if (hostlen > 0) {
-		host = calloc(1, hostlen + 1);
+		host = calloc(1, hostlen);
 		if (host == NULL) {
 			error = EAI_MEMORY;
 			goto out;
 		}
 	}
 	if (servlen > 0) {
-		serv = calloc(1, servlen + 1);
+		serv = calloc(1, servlen);
 		if (serv == NULL) {
 			error = EAI_MEMORY;
 			goto out;
diff --git a/lib/libcasper/services/cap_net/cap_net.c b/lib/libcasper/services/cap_net/cap_net.c
index c6abaa69faf6..c9282c3e139a 100644
--- a/lib/libcasper/services/cap_net/cap_net.c
+++ b/lib/libcasper/services/cap_net/cap_net.c
@@ -370,9 +370,9 @@ cap_getnameinfo(cap_channel_t *chan, const struct sockaddr *sa, socklen_t salen,
 	}
 
 	if (host != NULL && nvlist_exists_string(nvl, "host"))
-		strlcpy(host, nvlist_get_string(nvl, "host"), hostlen + 1);
+		strlcpy(host, nvlist_get_string(nvl, "host"), hostlen);
 	if (serv != NULL && nvlist_exists_string(nvl, "serv"))
-		strlcpy(serv, nvlist_get_string(nvl, "serv"), servlen + 1);
+		strlcpy(serv, nvlist_get_string(nvl, "serv"), servlen);
 	nvlist_destroy(nvl);
 	return (0);
 }
@@ -879,14 +879,14 @@ net_getnameinfo(const nvlist_t *limits, const nvlist_t *nvlin, nvlist_t *nvlout)
 	servlen = (size_t)nvlist_get_number(nvlin, "servlen");
 
 	if (hostlen > 0) {
-		host = calloc(1, hostlen + 1);
+		host = calloc(1, hostlen);
 		if (host == NULL) {
 			error = EAI_MEMORY;
 			goto out;
 		}
 	}
 	if (servlen > 0) {
-		serv = calloc(1, servlen + 1);
+		serv = calloc(1, servlen);
 		if (serv == NULL) {
 			error = EAI_MEMORY;
 			goto out;
diff --git a/lib/libcasper/services/cap_net/tests/net_test.c b/lib/libcasper/services/cap_net/tests/net_test.c
index 49cb0da44a4e..4c5f4751902b 100644
--- a/lib/libcasper/services/cap_net/tests/net_test.c
+++ b/lib/libcasper/services/cap_net/tests/net_test.c
@@ -44,6 +44,8 @@ __FBSDID("$FreeBSD$");
 #define	TEST_IPV4	"1.1.1.1"
 #define	TEST_IPV6	"2001:4860:4860::8888"
 #define	TEST_BIND_IPV4	"127.0.0.1"
+#define	TEST_PORT	80
+#define	TEST_PORT_STR	"80"
 
 static cap_channel_t *
 create_network_service(void)
@@ -376,6 +378,45 @@ ATF_TC_BODY(capnet__gethostbyaddr, tc)
 	cap_close(capnet);
 }
 
+ATF_TC_WITHOUT_HEAD(capnet__getnameinfo_buffer);
+ATF_TC_BODY(capnet__getnameinfo_buffer, tc)
+{
+	cap_channel_t *chan;
+	struct sockaddr_in sin;
+	int ret;
+	struct {
+		char host[sizeof(TEST_IPV4)];
+		char host_canary;
+		char serv[sizeof(TEST_PORT_STR)];
+		char serv_canary;
+	} buffers;
+
+	memset(&sin, 0, sizeof(sin));
+	sin.sin_family = AF_INET;
+	sin.sin_port = htons(TEST_PORT);
+	ret = inet_pton(AF_INET, TEST_IPV4, &sin.sin_addr);
+	ATF_REQUIRE_EQ(1, ret);
+
+	memset(&buffers, '!', sizeof(buffers));
+
+	chan = create_network_service();
+	ret = cap_getnameinfo(chan, (struct sockaddr *)&sin, sizeof(sin),
+	    buffers.host, sizeof(buffers.host),
+	    buffers.serv, sizeof(buffers.serv),
+	    NI_NUMERICHOST | NI_NUMERICSERV);
+	ATF_REQUIRE_EQ_MSG(0, ret, "%d", ret);
+
+	// Verify that cap_getnameinfo worked with minimally sized buffers.
+	ATF_CHECK_EQ(0, strcmp(TEST_IPV4, buffers.host));
+	ATF_CHECK_EQ(0, strcmp(TEST_PORT_STR, buffers.serv));
+
+	// Verify that cap_getnameinfo did not overflow the buffers.
+	ATF_CHECK_EQ('!', buffers.host_canary);
+	ATF_CHECK_EQ('!', buffers.serv_canary);
+
+	cap_close(chan);
+}
+
 ATF_TC_WITHOUT_HEAD(capnet__limits_addr2name_mode);
 ATF_TC_BODY(capnet__limits_addr2name_mode, tc)
 {
@@ -1248,6 +1289,8 @@ ATF_TP_ADD_TCS(tp)
 	ATF_TP_ADD_TC(tp, capnet__gethostbyname);
 	ATF_TP_ADD_TC(tp, capnet__gethostbyaddr);
 
+	ATF_TP_ADD_TC(tp, capnet__getnameinfo_buffer);
+
 	ATF_TP_ADD_TC(tp, capnet__limits_addr2name_mode);
 	ATF_TP_ADD_TC(tp, capnet__limits_addr2name_family);
 	ATF_TP_ADD_TC(tp, capnet__limits_addr2name);