Date: Tue, 4 Apr 2023 19:28:49 GMT From: Eric van Gyzen <vangyzen@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 179bffddf530 - main - cap_dns, cap_net: fix host and service buffer handling Message-ID: <202304041928.334JSnCO093109@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by vangyzen: URL: https://cgit.FreeBSD.org/src/commit/?id=179bffddf530d20b13cbc0056c908458ddfb2ae7 commit 179bffddf530d20b13cbc0056c908458ddfb2ae7 Author: Eric van Gyzen <vangyzen@FreeBSD.org> AuthorDate: 2023-03-30 22:54:01 +0000 Commit: Eric van Gyzen <vangyzen@FreeBSD.org> CommitDate: 2023-04-04 20:26:12 +0000 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 --- lib/libcasper/services/cap_dns/cap_dns.c | 8 ++--- lib/libcasper/services/cap_net/cap_net.c | 8 ++--- lib/libcasper/services/cap_net/tests/net_test.c | 43 +++++++++++++++++++++++++ 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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202304041928.334JSnCO093109>