Date: Thu, 15 May 2025 00:04:23 GMT From: Lexi Winter <ivy@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: a1215090416b - main - link_addr: be more strict about address formats Message-ID: <202505150004.54F04NZw047136@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by ivy: URL: https://cgit.FreeBSD.org/src/commit/?id=a1215090416b8afb346fb2ff5b38f25ba0134a3a commit a1215090416b8afb346fb2ff5b38f25ba0134a3a Author: Lexi Winter <ivy@FreeBSD.org> AuthorDate: 2025-05-14 22:02:59 +0000 Commit: Lexi Winter <ivy@FreeBSD.org> CommitDate: 2025-05-15 00:02:52 +0000 link_addr: be more strict about address formats instead of accepting any character as a delimiter, only accept ':', '.' and '-', and only permit a single delimiter in an address. this prevents accepting bizarre addresses like: ifconfig epair2a link 10.1.2.200/28 ... which is particularly problematic on an INET6-only system, in which case ifconfig defaults to the 'link' family, meaning that: ifconfig epair2a 10.1.2.200/28 ... changes the Ethernet address of the interface. bump __FreeBSD_version so link_addr() consumers can detect the change. Reviewed by: kp, des Approved by: des (mentor) Differential Revision: https://reviews.freebsd.org/D49936 --- lib/libc/net/linkaddr.3 | 34 +++--- lib/libc/net/linkaddr.c | 198 +++++++++++++++++++++++------------ lib/libc/tests/net/link_addr_test.cc | 152 ++++++++++++++++++++++++++- sbin/ifconfig/af_link.c | 3 +- sys/net/if_dl.h | 2 +- sys/sys/param.h | 2 +- 6 files changed, 307 insertions(+), 84 deletions(-) diff --git a/lib/libc/net/linkaddr.3 b/lib/libc/net/linkaddr.3 index ccfbf4c8d2e1..38ee6a0aedea 100644 --- a/lib/libc/net/linkaddr.3 +++ b/lib/libc/net/linkaddr.3 @@ -28,7 +28,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd May 7, 2025 +.Dd May 9, 2025 .Dt LINK_ADDR 3 .Os .Sh NAME @@ -42,7 +42,7 @@ .In sys/types.h .In sys/socket.h .In net/if_dl.h -.Ft void +.Ft int .Fn link_addr "const char *addr" "struct sockaddr_dl *sdl" .Ft char * .Fn link_ntoa "const struct sockaddr_dl *sdl" @@ -51,9 +51,19 @@ .Sh DESCRIPTION The routine .Fn link_addr -interprets character strings representing -link-level addresses, returning binary information suitable -for use in system calls. +parses a character string +.Fa addr +representing a link-level address, +and stores the resulting address in the structure pointed to by +.Fa sdl . +A link-level address consists of an optional interface name, followed by +a colon (which is required in all cases), followed by an address +consisting of either a string of hexadecimal digits, or a series of +hexadecimal octets separated by one of the characters +.Sq "." , +.Sq ":" , +or +.Sq - . .Pp The routine .Fn link_ntoa @@ -133,10 +143,11 @@ was at least one byte in size. .Pp The .Fn link_addr -function -has no return value. -(See -.Sx BUGS . ) +function returns 0 on success. +If the address did not appear to be a valid link-level address, -1 is +returned and +.Va errno +is set to indicate the error. .Sh SEE ALSO .Xr getnameinfo 3 .Sh HISTORY @@ -154,11 +165,6 @@ function appeared in The returned values for link_ntoa reside in a static memory area. .Pp -The function -.Fn link_addr -should diagnose improperly formed input, and there should be an unambiguous -way to recognize this. -.Pp If the .Va sdl_len field of the link socket address diff --git a/lib/libc/net/linkaddr.c b/lib/libc/net/linkaddr.c index 27d444666185..5be4c0a7a43e 100644 --- a/lib/libc/net/linkaddr.c +++ b/lib/libc/net/linkaddr.c @@ -36,87 +36,153 @@ #include <net/if_dl.h> #include <assert.h> +#include <errno.h> #include <stdint.h> #include <string.h> -/* States*/ -#define NAMING 0 -#define GOTONE 1 -#define GOTTWO 2 -#define RESET 3 -/* Inputs */ -#define DIGIT (4*0) -#define END (4*1) -#define DELIM (4*2) -#define LETTER (4*3) - -void +int link_addr(const char *addr, struct sockaddr_dl *sdl) { char *cp = sdl->sdl_data; char *cplim = sdl->sdl_len + (char *)sdl; - int byte = 0, state = NAMING, new; + const char *nptr; + size_t newsize; + int error = 0; + char delim = 0; + /* Initialise the sdl to zero, except for sdl_len. */ bzero((char *)&sdl->sdl_family, sdl->sdl_len - 1); sdl->sdl_family = AF_LINK; - do { - state &= ~LETTER; - if ((*addr >= '0') && (*addr <= '9')) { - new = *addr - '0'; - } else if ((*addr >= 'a') && (*addr <= 'f')) { - new = *addr - 'a' + 10; - } else if ((*addr >= 'A') && (*addr <= 'F')) { - new = *addr - 'A' + 10; - } else if (*addr == 0) { - state |= END; - } else if (state == NAMING && - (((*addr >= 'A') && (*addr <= 'Z')) || - ((*addr >= 'a') && (*addr <= 'z')))) - state |= LETTER; - else - state |= DELIM; - addr++; - switch (state /* | INPUT */) { - case NAMING | DIGIT: - case NAMING | LETTER: - *cp++ = addr[-1]; - continue; - case NAMING | DELIM: - state = RESET; - sdl->sdl_nlen = cp - sdl->sdl_data; - continue; - case GOTTWO | DIGIT: - *cp++ = byte; - /* FALLTHROUGH */ - case RESET | DIGIT: - state = GOTONE; - byte = new; - continue; - case GOTONE | DIGIT: - state = GOTTWO; - byte = new + (byte << 4); - continue; - default: /* | DELIM */ - state = RESET; - *cp++ = byte; - byte = 0; - continue; - case GOTONE | END: - case GOTTWO | END: - *cp++ = byte; - /* FALLTHROUGH */ - case RESET | END: + + /* + * Everything up to the first ':' is the interface name. Usually the + * ':' should always be present even if there's no interface name, but + * since this interface was poorly specified in the past, accept a + * missing colon as meaning no interface name. + */ + if ((nptr = strchr(addr, ':')) != NULL) { + size_t namelen = nptr - addr; + + /* Ensure the sdl is large enough to store the name. */ + if (namelen > cplim - cp) { + errno = ENOSPC; + return (-1); + } + + memcpy(cp, addr, namelen); + cp += namelen; + sdl->sdl_nlen = namelen; + /* Skip the interface name and the colon. */ + addr += namelen + 1; + } + + /* + * The remainder of the string should be hex digits representing the + * address, with optional delimiters. Each two hex digits form one + * octet, but octet output can be forced using a delimiter, so we accept + * a long string of hex digits, or a mix of delimited and undelimited + * digits like "1122.3344.5566", or delimited one- or two-digit octets + * like "1.22.3". + * + * If anything fails at this point, exit the loop so we set sdl_alen and + * sdl_len based on whatever we did manage to parse. This preserves + * compatibility with the 4.3BSD version of link_addr, which had no way + * to indicate an error and would just return. + */ +#define DIGIT(c) \ + (((c) >= '0' && (c) <= '9') ? ((c) - '0') \ + : ((c) >= 'a' && (c) <= 'f') ? ((c) - 'a' + 10) \ + : ((c) >= 'A' && (c) <= 'F') ? ((c) - 'A' + 10) \ + : (-1)) +#define ISDELIM(c) (((c) == '.' || (c) == ':' || (c) == '-') && \ + (delim == 0 || delim == (c))) + + for (;;) { + int digit, digit2; + + /* + * Treat any leading delimiters as empty bytes. This supports + * the (somewhat obsolete) form of Ethernet addresses with empty + * octets, e.g. "1::3:4:5:6". + */ + while (ISDELIM(*addr) && cp < cplim) { + delim = *addr++; + *cp++ = 0; + } + + /* Did we reach the end of the string? */ + if (*addr == '\0') + break; + + /* + * If not, the next character must be a digit, so make sure we + * have room for at least one more octet. + */ + + if (cp >= cplim) { + error = ENOSPC; break; } - break; - } while (cp < cplim); + + if ((digit = DIGIT(*addr)) == -1) { + error = EINVAL; + break; + } + + ++addr; + + /* If the next character is another digit, consume it. */ + if ((digit2 = DIGIT(*addr)) != -1) { + digit = (digit << 4) | digit2; + ++addr; + } + + if (ISDELIM(*addr)) { + /* + * If the digit is followed by a delimiter, write it + * and consume the delimiter. + */ + delim = *addr++; + *cp++ = digit; + } else if (DIGIT(*addr) != -1) { + /* + * If two digits are followed by a third digit, treat + * the two digits we have as a single octet and + * continue. + */ + *cp++ = digit; + } else if (*addr == '\0') { + /* If the digit is followed by EOS, we're done. */ + *cp++ = digit; + break; + } else { + /* Otherwise, the input was invalid. */ + error = EINVAL; + break; + } + } +#undef DIGIT +#undef ISDELIM + + /* How many bytes did we write to the address? */ sdl->sdl_alen = cp - LLADDR(sdl); - new = cp - (char *)sdl; - if (new > sizeof(*sdl)) - sdl->sdl_len = new; - return; + + /* + * The user might have given us an sdl which is larger than sizeof(sdl); + * in that case, record the actual size of the new sdl. + */ + newsize = cp - (char *)sdl; + if (newsize > sizeof(*sdl)) + sdl->sdl_len = (u_char)newsize; + + if (error == 0) + return (0); + + errno = error; + return (-1); } + char * link_ntoa(const struct sockaddr_dl *sdl) { diff --git a/lib/libc/tests/net/link_addr_test.cc b/lib/libc/tests/net/link_addr_test.cc index ea8f64c6723b..b973b924dc13 100644 --- a/lib/libc/tests/net/link_addr_test.cc +++ b/lib/libc/tests/net/link_addr_test.cc @@ -71,10 +71,12 @@ sockaddr_dl make_linkaddr(const std::string &addr) { auto sdl = sockaddr_dl{}; + int ret; sdl.sdl_len = sizeof(sdl); - ::link_addr(addr.c_str(), &sdl); + ret = ::link_addr(addr.c_str(), &sdl); + ATF_REQUIRE_EQ(0, ret); ATF_REQUIRE_EQ(AF_LINK, static_cast<int>(sdl.sdl_family)); ATF_REQUIRE_EQ(true, sdl.sdl_len > 0); ATF_REQUIRE_EQ(true, sdl.sdl_nlen >= 0); @@ -176,6 +178,10 @@ std::vector<test_address> test_addresses{ {"aa:bb:cc:dd:ee:ff"s, "aa.bb.cc.dd.ee.ff", ether_addr{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}}, + + // Address with a blank octet; this is an old form of Ethernet address. + {"00:11::33:44:55"s, "0.11.0.33.44.55", + ether_addr{0x00, 0x11, 0x00, 0x33, 0x44, 0x55}}, }; /* @@ -219,6 +225,43 @@ ATF_TEST_CASE_BODY(ifname) } +/* + * Test with some invalid addresses. + */ +ATF_TEST_CASE_WITHOUT_HEAD(invalid) +ATF_TEST_CASE_BODY(invalid) +{ + auto const invalid_addresses = std::vector{ + // Invalid separator + ":1/2/3"s, + "ix0:1/2/3"s, + + // Multiple different separators + ":1.2-3"s, + "ix0:1.2-3"s, + + // An IP address + ":10.1.2.200/28"s, + "ix0:10.1.2.200/28"s, + + // Valid address followed by garbage + ":1.2.3xxx"s, + ":1.2.3.xxx"s, + "ix0:1.2.3xxx"s, + "ix0:1.2.3.xxx"s, + }; + + for (auto const &addr : invalid_addresses) { + int ret; + + auto sdl = sockaddr_dl{}; + sdl.sdl_len = sizeof(sdl); + + ret = link_addr(addr.c_str(), &sdl); + ATF_REQUIRE_EQ(-1, ret); + } +} + /* * Test some non-Ethernet addresses. */ @@ -245,6 +288,111 @@ ATF_TEST_CASE_BODY(nonether) lladdr(sdl))); } +/* + * Test link_addr behaviour with undersized buffers. + */ +ATF_TEST_CASE_WITHOUT_HEAD(smallbuf) +ATF_TEST_CASE_BODY(smallbuf) +{ + static constexpr auto garbage = std::byte{0xcc}; + auto buf = std::vector<std::byte>(); + sockaddr_dl *sdl; + int ret; + + /* + * Make an sdl with an sdl_data member of the appropriate size, and + * place it in buf. Ensure it's followed by a trailing byte of garbage + * so we can test that link_addr doesn't write past the end. + */ + auto mksdl = [&buf](std::size_t datalen) -> sockaddr_dl * { + auto actual_size = datalen + offsetof(sockaddr_dl, sdl_data); + + buf.resize(actual_size + 1); + std::ranges::fill(buf, garbage); + + auto *sdl = new(reinterpret_cast<sockaddr_dl *>(&buf[0])) + sockaddr_dl; + sdl->sdl_len = actual_size; + return (sdl); + }; + + /* An sdl large enough to store the interface name */ + sdl = mksdl(3); + ret = link_addr("ix0:1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(-1, ret); + ATF_REQUIRE_EQ(ENOSPC, errno); + ATF_REQUIRE_EQ(3, sdl->sdl_nlen); + ATF_REQUIRE_EQ("ix0", ifname(*sdl)); + ATF_REQUIRE_EQ(0, static_cast<int>(sdl->sdl_alen)); + + /* + * For these tests, test both with and without an interface name, since + * that will overflow the buffer in different places. + */ + + /* An empty sdl. Nothing may grow on this cursed ground. */ + + sdl = mksdl(0); + ret = link_addr("ix0:1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(-1, ret); + ATF_REQUIRE_EQ(ENOSPC, errno); + ATF_REQUIRE_EQ(0, sdl->sdl_nlen); + ATF_REQUIRE_EQ(0, static_cast<int>(sdl->sdl_alen)); + + sdl = mksdl(0); + ret = link_addr(":1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(-1, ret); + ATF_REQUIRE_EQ(ENOSPC, errno); + ATF_REQUIRE_EQ(0, sdl->sdl_nlen); + ATF_REQUIRE_EQ(0, static_cast<int>(sdl->sdl_alen)); + + /* + * An sdl large enough to store the interface name and two octets of the + * address. + */ + + sdl = mksdl(5); + ret = link_addr("ix0:1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(-1, ret); + ATF_REQUIRE_EQ(ENOSPC, errno); + ATF_REQUIRE_EQ("ix0", ifname(*sdl)); + ATF_REQUIRE(std::ranges::equal( + std::vector{ 0x01, 0x02 }, lladdr(*sdl))); + + sdl = mksdl(2); + ret = link_addr(":1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(-1, ret); + ATF_REQUIRE_EQ(ENOSPC, errno); + ATF_REQUIRE_EQ("", ifname(*sdl)); + ATF_REQUIRE(std::ranges::equal( + std::vector{ 0x01, 0x02 }, lladdr(*sdl))); + + /* + * An sdl large enough to store the entire address. + */ + + sdl = mksdl(6); + ret = link_addr("ix0:1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(0, ret); + ATF_REQUIRE_EQ("ix0", ifname(*sdl)); + ATF_REQUIRE(std::ranges::equal( + std::vector{ 0x01, 0x02, 0x03 }, lladdr(*sdl))); + + sdl = mksdl(3); + ret = link_addr(":1.2.3", sdl); + ATF_REQUIRE(*rbegin(buf) == garbage); + ATF_REQUIRE_EQ(0, ret); + ATF_REQUIRE_EQ("", ifname(*sdl)); + ATF_REQUIRE(std::ranges::equal( + std::vector{ 0x01, 0x02, 0x03 }, lladdr(*sdl))); +} + /* * Test an extremely long address which would overflow link_ntoa's internal * buffer. It should handle this by truncating the output. @@ -376,6 +524,8 @@ ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, basic); ATF_ADD_TEST_CASE(tcs, ifname); + ATF_ADD_TEST_CASE(tcs, smallbuf); + ATF_ADD_TEST_CASE(tcs, invalid); ATF_ADD_TEST_CASE(tcs, nonether); ATF_ADD_TEST_CASE(tcs, overlong); ATF_ADD_TEST_CASE(tcs, link_ntoa_r); diff --git a/sbin/ifconfig/af_link.c b/sbin/ifconfig/af_link.c index ab35d04a8709..55b75d847c16 100644 --- a/sbin/ifconfig/af_link.c +++ b/sbin/ifconfig/af_link.c @@ -210,7 +210,8 @@ link_getaddr(const char *addr, int which) temp[0] = ':'; strcpy(temp + 1, addr); sdl.sdl_len = sizeof(sdl); - link_addr(temp, &sdl); + if (link_addr(temp, &sdl) == -1) + errx(1, "malformed link-level address"); free(temp); } if (sdl.sdl_alen > sizeof(sa->sa_data)) diff --git a/sys/net/if_dl.h b/sys/net/if_dl.h index 74089277291c..93a094d1ff59 100644 --- a/sys/net/if_dl.h +++ b/sys/net/if_dl.h @@ -83,7 +83,7 @@ struct sockaddr_dl *link_init_sdl(struct ifnet *ifp, struct sockaddr *paddr, #include <sys/cdefs.h> __BEGIN_DECLS -void link_addr(const char *, struct sockaddr_dl *); +int link_addr(const char *, struct sockaddr_dl *); char *link_ntoa(const struct sockaddr_dl *); int link_ntoa_r(const struct sockaddr_dl *, char *, size_t *); __END_DECLS diff --git a/sys/sys/param.h b/sys/sys/param.h index 88edc92e65e7..6ce6903826d3 100644 --- a/sys/sys/param.h +++ b/sys/sys/param.h @@ -73,7 +73,7 @@ * cannot include sys/param.h and should only be updated here. */ #undef __FreeBSD_version -#define __FreeBSD_version 1500042 +#define __FreeBSD_version 1500043 /* * __FreeBSD_kernel__ indicates that this system uses the kernel of FreeBSD,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202505150004.54F04NZw047136>