From 36c440758fc9fd606fbdfe342a4b3fd842d05c4e Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 4 Aug 2026 17:38:10 +0300 Subject: l2tp: read and write AVP values through unaligned-safe accessors AVPs are packed back to back in the receive buffer, so the offset of any given AVP is the sum of the lengths of all the AVPs before it, i.e. a value the peer picks. Casting avp->val to uint16_t/uint32_t/uint64_t therefore dereferences a pointer with an arbitrary alignment, which is undefined behaviour, is caught by -fsanitize=alignment, and is only harmless on x86 by accident. The same applies to the send path, where the AVPs being built are laid out the same way. Introduce unaligned_{ntohs,ntohl,be64toh}() and their store counterparts, all memcpy() based, and use them for every multi-byte field read out of or written into an AVP body. Accesses to the members of struct l2tp_avp_t itself are fine as it is declared packed. memxor() had the same problem, in a worse form: it cast both of its uint8_t pointers to uintmax_t and walked them word by word. Since it is only ever called on MD5 sized chunks, replace it with a plain byte loop which compilers vectorize just as well, and which no longer breaks strict aliasing either. No functional change intended, this is a portability and UB fix; it also removes a source of noise for the s390x (big endian) CI job. --- accel-pppd/ctrl/l2tp/packet.c | 96 ++++++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 34 deletions(-) diff --git a/accel-pppd/ctrl/l2tp/packet.c b/accel-pppd/ctrl/l2tp/packet.c index 1e1488b5..68f36ece 100644 --- a/accel-pppd/ctrl/l2tp/packet.c +++ b/accel-pppd/ctrl/l2tp/packet.c @@ -111,36 +111,64 @@ void l2tp_packet_free(struct l2tp_packet_t *pack) mempool_free(pack); } +/* + * AVPs are not aligned in any way inside the packet buffer: their offset + * depends on the length of every preceding AVP, which is peer chosen. + * Always go through memcpy() to read multi-byte fields out of them, both to + * stay portable on strict alignment architectures and to avoid tripping + * -fsanitize=alignment. + */ +static uint16_t unaligned_ntohs(const void *ptr) +{ + uint16_t val; + + memcpy(&val, ptr, sizeof(val)); + + return ntohs(val); +} + +static uint32_t unaligned_ntohl(const void *ptr) +{ + uint32_t val; + + memcpy(&val, ptr, sizeof(val)); + + return ntohl(val); +} + +static uint64_t unaligned_be64toh(const void *ptr) +{ + uint64_t val; + + memcpy(&val, ptr, sizeof(val)); + + return be64toh(val); +} + +static void unaligned_htons(void *ptr, uint16_t val) +{ + val = htons(val); + memcpy(ptr, &val, sizeof(val)); +} + +static void unaligned_htonl(void *ptr, uint32_t val) +{ + val = htonl(val); + memcpy(ptr, &val, sizeof(val)); +} + +static void unaligned_htobe64(void *ptr, uint64_t val) +{ + val = htobe64(val); + memcpy(ptr, &val, sizeof(val)); +} + static void memxor(uint8_t *dst, const uint8_t *src, size_t sz) { - const uintmax_t *umax_src = (const uintmax_t *)src; - uintmax_t *umax_dst = (uintmax_t *)dst; - size_t left = sz % sizeof(uintmax_t); size_t indx; - for (indx = 0; indx < sz / sizeof(uintmax_t); ++indx) - umax_dst[indx] ^= umax_src[indx]; - - src += sz - left; - dst += sz - left; - while (left) { - if (left >= sizeof(uint32_t)) { - *(uint32_t *)dst ^= *(uint32_t *)src; - src += sizeof(uint32_t); - dst += sizeof(uint32_t); - left -= sizeof(uint32_t); - } else if (left >= sizeof(uint16_t)) { - *(uint16_t *)dst ^= *(uint16_t *)src; - src += sizeof(uint16_t); - dst += sizeof(uint16_t); - left -= sizeof(uint16_t); - } else { - *dst ^= *src; - src += sizeof(uint8_t); - dst += sizeof(uint8_t); - left -= sizeof(uint8_t); - } - } + for (indx = 0; indx < sz; ++indx) + dst[indx] ^= src[indx]; } /* @@ -185,7 +213,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, } memxor(p1, avp->val, MD5_DIGEST_LENGTH); - orig_attr_len = ntohs(*(uint16_t *)p1); + orig_attr_len = unaligned_ntohs(p1); if (orig_attr_len <= MD5_DIGEST_LENGTH - 2) { /* Enough bytes decoded already, no need to decode padding */ @@ -423,7 +451,7 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info, if (decode_avp(avp, RV, secret, secret_len) < 0) goto out_err; - orig_avp_len = ntohs(*(uint16_t *)avp->val) + sizeof(*avp); + orig_avp_len = unaligned_ntohs(avp->val) + sizeof(*avp); orig_avp_val = avp->val + sizeof(uint16_t); } else { orig_avp_len = avp_len; @@ -445,17 +473,17 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info, case ATTR_TYPE_INT16: if (orig_avp_len != sizeof(*avp) + 2) goto out_err_len; - attr->val.uint16 = ntohs(*(uint16_t *)orig_avp_val); + attr->val.uint16 = unaligned_ntohs(orig_avp_val); break; case ATTR_TYPE_INT32: if (orig_avp_len != sizeof(*avp) + 4) goto out_err_len; - attr->val.uint32 = ntohl(*(uint32_t *)orig_avp_val); + attr->val.uint32 = unaligned_ntohl(orig_avp_val); break; case ATTR_TYPE_INT64: if (orig_avp_len != sizeof(*avp) + 8) goto out_err_len; - attr->val.uint64 = be64toh(*(uint64_t *)orig_avp_val); + attr->val.uint64 = unaligned_be64toh(orig_avp_val); break; case ATTR_TYPE_OCTETS: attr->val.octets = _malloc(attr->length); @@ -532,13 +560,13 @@ int l2tp_packet_send(int sock, struct l2tp_packet_t *pack) else switch (attr->attr->type) { case ATTR_TYPE_INT16: - *(int16_t *)avp->val = htons(attr->val.int16); + unaligned_htons(avp->val, attr->val.int16); break; case ATTR_TYPE_INT32: - *(int32_t *)avp->val = htonl(attr->val.int32); + unaligned_htonl(avp->val, attr->val.int32); break; case ATTR_TYPE_INT64: - *(uint64_t *)avp->val = htobe64(attr->val.uint64); + unaligned_htobe64(avp->val, attr->val.uint64); break; case ATTR_TYPE_STRING: case ATTR_TYPE_OCTETS: -- cgit v1.2.3 From 9554ce05e13fdb8c55a54e259b4154c9aeaca58d Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 4 Aug 2026 17:40:34 +0300 Subject: l2tp: validate the deciphered length of hidden AVPs decode_avp() only checked the 2 bytes length prefix of the Hidden AVP Subformat when the ciphered attribute spanned more than one MD5 block. For an attribute of at most 16 bytes it deciphered the single block and returned straight away, leaving l2tp_recv() to take the prefix at face value: orig_avp_len = ntohs(*(uint16_t *)avp->val) + sizeof(*avp); ... attr->length = orig_avp_len - sizeof(*avp); That prefix is an output of the cipher, so a peer which does not know the secret (i.e. anyone able to reach the L2TP socket, no handshake needed beyond a preceding Random-Vector AVP) turns it into 16 random bits. Worse than an over-read: orig_avp_len is a uint16_t, so a prefix of 0xffff wraps to 5, attr->length becomes -1, and the ATTR_TYPE_STRING and ATTR_TYPE_OCTETS cases then run attr->val.string = _malloc(attr->length + 1); /* _malloc(0) */ memcpy(attr->val.string, orig_avp_val, attr->length); /* SIZE_MAX */ that is an unbounded memcpy() into a zero sized allocation. Remotely triggerable heap corruption on any tunnel with a secret configured. The accel-ppp encoder always pads hidden AVPs by at least 16 bytes, so it never produces an attribute short enough to reach this path; only a crafted packet does. Fix it at the source rather than at the call site: decode_avp() now returns the deciphered length through an output parameter, and the length is bounded against the room actually available in the received AVP on every path out of the function, single block included. Callers can no longer re-derive it from the AVP body and get it wrong, and the existing multi-block check keeps guarding the deciphering loop itself. Add packet_test.c, a standalone test which drives the real parser through a real UDP socket: hand-crafted packets cover the length prefix checks (including the boundaries of what fits and the single block path the encoder cannot produce), and l2tp_packet_send()/l2tp_recv() round trips cover the multi-block cipher and the unaligned AVP accessors. It reproduces the corruption above under ASan on unpatched code. Wire it, and the so far unused bitpool_test.c, into the ASAN/UBSAN workflow. Inspired by the equivalent hardening in accel-ppp-ng (commit a8ca0f3f), which bounds the length at the call site; the fix here is placed inside decode_avp() and covered by a regression test. --- .github/workflows/run-tests-asan-ubsan.yml | 15 + accel-pppd/ctrl/l2tp/packet.c | 47 ++- accel-pppd/ctrl/l2tp/packet_test.c | 491 +++++++++++++++++++++++++++++ 3 files changed, 544 insertions(+), 9 deletions(-) create mode 100644 accel-pppd/ctrl/l2tp/packet_test.c diff --git a/.github/workflows/run-tests-asan-ubsan.yml b/.github/workflows/run-tests-asan-ubsan.yml index 12bf748d..8b25701e 100644 --- a/.github/workflows/run-tests-asan-ubsan.yml +++ b/.github/workflows/run-tests-asan-ubsan.yml @@ -60,6 +60,21 @@ jobs: working-directory: ./build run: make && sudo make install + - name: Run standalone unit tests (with ${{ matrix.sanitizer }}) + env: + ${{ matrix.env_name }}: ${{ matrix.env_value }} + run: | + gcc -O1 -g -Wall -fno-strict-aliasing -D_GNU_SOURCE \ + -DOPENSSL_API_COMPAT=0x10100000L \ + -fsanitize=${{ matrix.sanitizer }} -fno-sanitize-recover=all \ + -I accel-pppd/include -I accel-pppd/ctrl/l2tp \ + -o /tmp/l2tp_packet_test \ + accel-pppd/ctrl/l2tp/packet_test.c accel-pppd/ctrl/l2tp/packet.c \ + -lcrypto + /tmp/l2tp_packet_test + gcc -O2 -Wall -o /tmp/bitpool_test accel-pppd/extra/bitpool_test.c + /tmp/bitpool_test + - name: Insert and check kernel modules (ipoe, vlan-mon, ppposeq) # if: ${{ false }} run: | diff --git a/accel-pppd/ctrl/l2tp/packet.c b/accel-pppd/ctrl/l2tp/packet.c index 68f36ece..0a4113a0 100644 --- a/accel-pppd/ctrl/l2tp/packet.c +++ b/accel-pppd/ctrl/l2tp/packet.c @@ -174,9 +174,17 @@ static void memxor(uint8_t *dst, const uint8_t *src, size_t sz) /* * Decipher hidden AVPs, keeping the Hidden AVP Subformat (i.e. the attribute * value is prefixed by 2 bytes indicating its length in network byte order). + * + * On success the deciphered original attribute length is stored into + * *orig_attr_len, already validated against the size of the received AVP. + * Callers must never re-read that length from the AVP body themselves: it is + * the output of the cipher, so a peer using a mismatching secret (or an + * attacker blindly injecting hidden AVPs) makes it an essentially random + * 16 bits value. */ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, - const char *secret, size_t secret_len) + const char *secret, size_t secret_len, + uint16_t *orig_attr_len_out) { MD5_CTX md5_ctx; uint8_t md5[MD5_DIGEST_LENGTH]; @@ -190,7 +198,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, uint16_t last_block_len; avp_len = avp->flags & L2TP_AVP_LEN_MASK; - if (avp_len < sizeof(struct l2tp_avp_t) + 2) { + if (avp_len < sizeof(struct l2tp_avp_t) + sizeof(uint16_t)) { /* Hidden AVPs must contain at least two bytes for storing original attribute length */ log_warn("l2tp: incorrect hidden avp received (type %hu):" @@ -208,20 +216,22 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, MD5_Final(p1, &md5_ctx); if (attr_len <= MD5_DIGEST_LENGTH) { + /* The whole attribute fits in the first block: it is fully + deciphered, nothing more to do but to check its length */ memxor(avp->val, p1, attr_len); - return 0; + goto out; } memxor(p1, avp->val, MD5_DIGEST_LENGTH); orig_attr_len = unaligned_ntohs(p1); - if (orig_attr_len <= MD5_DIGEST_LENGTH - 2) { + if (orig_attr_len <= MD5_DIGEST_LENGTH - sizeof(uint16_t)) { /* Enough bytes decoded already, no need to decode padding */ memcpy(avp->val, p1, MD5_DIGEST_LENGTH); - return 0; + goto out; } - if (orig_attr_len > attr_len - 2) { + if (orig_attr_len > attr_len - sizeof(uint16_t)) { log_warn("l2tp: incorrect hidden avp received (type %hu):" " original attribute length too big (ciphered" " attribute length: %hu bytes, advertised original" @@ -232,7 +242,7 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, /* Decode remaining blocks. Start from the last block as preceding blocks must be kept hidden for computing MD5s */ - bytes_left = orig_attr_len + 2 - MD5_DIGEST_LENGTH; + bytes_left = orig_attr_len + sizeof(uint16_t) - MD5_DIGEST_LENGTH; last_block_len = bytes_left % MD5_DIGEST_LENGTH; blocks_left = bytes_left / MD5_DIGEST_LENGTH; if (last_block_len) { @@ -256,6 +266,23 @@ static int decode_avp(struct l2tp_avp_t *avp, const struct l2tp_attr_t *RV, } memcpy(avp->val, p1, MD5_DIGEST_LENGTH); +out: + /* The length prefix comes out of the cipher, so it is only as + trustworthy as the peer's knowledge of the shared secret. Bound it + against the room actually available in the received AVP before + letting it drive any read of the attribute value */ + orig_attr_len = unaligned_ntohs(avp->val); + if (orig_attr_len > attr_len - sizeof(uint16_t)) { + log_warn("l2tp: incorrect hidden avp received (type %hu):" + " deciphered attribute length too big (ciphered" + " attribute length: %hu bytes, deciphered original" + " attribute length: %hu bytes), wrong secret?\n", + ntohs(avp->type), attr_len, orig_attr_len); + return -1; + } + + *orig_attr_len_out = orig_attr_len; + return 0; } @@ -269,6 +296,7 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info, struct sockaddr_in addr; socklen_t addr_len; uint16_t orig_avp_len; + uint16_t orig_attr_len; void *orig_avp_val; uint8_t *buf, *ptr; int n, length; @@ -448,10 +476,11 @@ int l2tp_recv(int fd, struct l2tp_packet_t **p, struct in_pktinfo *pkt_info, ntohs(avp->type)); goto out_err; } - if (decode_avp(avp, RV, secret, secret_len) < 0) + if (decode_avp(avp, RV, secret, secret_len, + &orig_attr_len) < 0) goto out_err; - orig_avp_len = unaligned_ntohs(avp->val) + sizeof(*avp); + orig_avp_len = orig_attr_len + sizeof(*avp); orig_avp_val = avp->val + sizeof(uint16_t); } else { orig_avp_len = avp_len; diff --git a/accel-pppd/ctrl/l2tp/packet_test.c b/accel-pppd/ctrl/l2tp/packet_test.c new file mode 100644 index 00000000..9f962407 --- /dev/null +++ b/accel-pppd/ctrl/l2tp/packet_test.c @@ -0,0 +1,491 @@ +/* + * Standalone regression test for the L2TP control message parser. + * + * Not part of the cmake build. Compile and run with: + * gcc -O1 -g -Wall -fno-strict-aliasing -D_GNU_SOURCE \ + * -fsanitize=address,undefined -fno-sanitize-recover=all \ + * -I accel-pppd/include -I accel-pppd/ctrl/l2tp \ + * -o /tmp/l2tp_packet_test \ + * accel-pppd/ctrl/l2tp/packet_test.c accel-pppd/ctrl/l2tp/packet.c \ + * -lcrypto && /tmp/l2tp_packet_test + * + * The interesting part is the hidden AVP subformat: the 2 bytes length prefix + * of a hidden AVP is an *output of the cipher*, so a peer using a different + * secret -- or an attacker injecting hidden AVPs blindly -- turns it into an + * essentially random 16 bits value. It must never be trusted to bound a read + * of the attribute value. + * + * The test drives the real parser through a real UDP socket: + * - hand-crafted packets exercise the hidden AVP length checks, including + * the single block (attribute <= 16 bytes) cipher path which the accel-ppp + * encoder itself never produces (it always pads by >= 16 bytes); + * - l2tp_packet_send()/l2tp_recv() round trips exercise the multi block + * cipher path and the unaligned AVP accessors. + * + * Everything packet.c needs besides libcrypto is stubbed below. + */ +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "triton.h" +#include "log.h" +#include "mempool.h" +#include "l2tp.h" +#include "attr_defs.h" + +static int failures; +#define CHECK(cond) do { if (!(cond)) { \ + fprintf(stderr, "FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); failures++; } } while (0) + +/* ------------------------------------------------------------------ stubs */ + +int conf_verbose = 1; +int conf_avp_permissive = 0; + +/* A dictionary just big enough for the attributes used here. Types are the + ones accel-ppp's own dictionary uses, except Tie_Breaker which is turned + into an INT64 to get coverage of the 64 bits accessor. */ +static struct l2tp_dict_attr_t dict[] = { + { .name = "Message-Type", .id = Message_Type, .type = ATTR_TYPE_INT16, .M = 1, .H = 0 }, + { .name = "Tie-Breaker", .id = Tie_Breaker, .type = ATTR_TYPE_INT64, .M = 0, .H = -1 }, + { .name = "Host-Name", .id = Host_Name, .type = ATTR_TYPE_STRING, .M = 1, .H = -1 }, + { .name = "Assigned-Tunnel-Id", .id = Assigned_Tunnel_ID, .type = ATTR_TYPE_INT16, .M = 1, .H = -1 }, + { .name = "Call-Serial-Number", .id = Call_Serial_Number, .type = ATTR_TYPE_INT32, .M = 1, .H = -1 }, + { .name = "Random-Vector", .id = Random_Vector, .type = ATTR_TYPE_OCTETS, .M = 1, .H = 0 }, +}; + +struct l2tp_dict_attr_t *l2tp_dict_find_attr_by_id(int id) +{ + size_t indx; + + for (indx = 0; indx < sizeof(dict) / sizeof(dict[0]); ++indx) + if (dict[indx].id == id) + return &dict[indx]; + + return NULL; +} + +const struct l2tp_dict_value_t *l2tp_dict_find_value(const struct l2tp_dict_attr_t *attr, + l2tp_value_t val) +{ + return NULL; +} + +/* Size carrying mempool: allocations stay exactly as large as the pool's + object size, so that ASan traps any read past the end of a packet buffer */ +mempool_t *mempool_create(int size) +{ + int *pool = malloc(sizeof(int)); + + *pool = size; + + return (mempool_t *)pool; +} + +void *mempool_alloc(mempool_t *pool) +{ + return malloc(*(int *)pool); +} + +void mempool_free(void *ptr) +{ + free(ptr); +} + +void triton_register_init(int order, void (*func)(void)) +{ + func(); +} + +int u_randbuf(void *buf, size_t buf_len, int *err) +{ + uint8_t *u8_buf = buf; + size_t indx; + + /* Deterministic on purpose: reproducible failures beat real entropy */ + for (indx = 0; indx < buf_len; ++indx) + u8_buf[indx] = (uint8_t)(indx * 7 + 0x5a); + + return 0; +} + +#define DEFINE_LOG_STUB(name) \ + void name(const char *fmt, ...) {} +DEFINE_LOG_STUB(log_emerg) +DEFINE_LOG_STUB(log_error) +DEFINE_LOG_STUB(log_warn) +DEFINE_LOG_STUB(log_ppp_debug) + +/* -------------------------------------------------------- packet building */ + +struct pktbuf { + uint8_t data[2048]; + size_t len; +}; + +static void pkt_init(struct pktbuf *pkt) +{ + struct l2tp_hdr_t hdr; + + memset(&hdr, 0, sizeof(hdr)); + hdr.flags = htons(L2TP_FLAG_T | L2TP_FLAG_L | L2TP_FLAG_S | 2); + + memset(pkt, 0, sizeof(*pkt)); + memcpy(pkt->data, &hdr, sizeof(hdr)); + pkt->len = sizeof(hdr); +} + +/* Append an AVP and return a pointer to its value */ +static uint8_t *pkt_add_avp(struct pktbuf *pkt, uint16_t extra_flags, + uint16_t type, const void *val, size_t val_len) +{ + struct l2tp_avp_t avp; + uint8_t *ptr = pkt->data + pkt->len; + + memset(&avp, 0, sizeof(avp)); + avp.flags = htons(extra_flags | ((sizeof(avp) + val_len) & L2TP_AVP_LEN_MASK)); + avp.type = htons(type); + + memcpy(ptr, &avp, sizeof(avp)); + if (val_len) + memcpy(ptr + sizeof(avp), val, val_len); + pkt->len += sizeof(avp) + val_len; + + return ptr + sizeof(avp); +} + +static void pkt_finish(struct pktbuf *pkt) +{ + uint16_t length = htons(pkt->len); + + memcpy(pkt->data + offsetof(struct l2tp_hdr_t, length), + &length, sizeof(length)); +} + +/* + * Cipher a hidden AVP whose cleartext (length prefix included) is at most one + * MD5 block long, i.e. the path that never validated the length prefix. + */ +static void hide_single_block(uint8_t *val, size_t val_len, uint16_t type, + const char *secret, size_t secret_len, + const uint8_t *rv, size_t rv_len) +{ + uint8_t md5[MD5_DIGEST_LENGTH]; + uint16_t attr_type = htons(type); + MD5_CTX md5_ctx; + size_t indx; + + MD5_Init(&md5_ctx); + MD5_Update(&md5_ctx, &attr_type, sizeof(attr_type)); + MD5_Update(&md5_ctx, secret, secret_len); + MD5_Update(&md5_ctx, rv, rv_len); + MD5_Final(md5, &md5_ctx); + + for (indx = 0; indx < val_len && indx < MD5_DIGEST_LENGTH; ++indx) + val[indx] ^= md5[indx]; +} + +/* --------------------------------------------------------------- plumbing */ + +static const char secret[] = "s3cr3t"; +static int sock = -1; +static struct sockaddr_in sock_addr; + +static void loopback_socket(void) +{ + socklen_t addr_len = sizeof(sock_addr); + + sock = socket(AF_INET, SOCK_DGRAM, 0); + if (sock < 0) { + perror("socket"); + exit(1); + } + + memset(&sock_addr, 0, sizeof(sock_addr)); + sock_addr.sin_family = AF_INET; + sock_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + if (bind(sock, (struct sockaddr *)&sock_addr, sizeof(sock_addr)) < 0 + || getsockname(sock, (struct sockaddr *)&sock_addr, &addr_len) < 0) { + perror("bind"); + exit(1); + } +} + +/* Feed raw bytes to the parser, NULL means "packet rejected" */ +static struct l2tp_packet_t *parse(const struct pktbuf *pkt) +{ + struct l2tp_packet_t *pack = NULL; + + if (sendto(sock, pkt->data, pkt->len, 0, + (struct sockaddr *)&sock_addr, sizeof(sock_addr)) < 0) { + perror("sendto"); + exit(1); + } + + CHECK(l2tp_recv(sock, &pack, NULL, secret, sizeof(secret) - 1) == 0); + + return pack; +} + +static const struct l2tp_attr_t *find_attr(const struct l2tp_packet_t *pack, int id) +{ + const struct l2tp_attr_t *attr; + + list_for_each_entry(attr, &pack->attrs, entry) + if (attr->attr->id == id) + return attr; + + return NULL; +} + +/* ------------------------------------------------------------------ tests */ + +/* + * A hidden AVP small enough to be ciphered in a single block: its deciphered + * length prefix used to be taken at face value, so anything up to 65535 was + * handed to the memcpy() feeding attr->val, reading way past the packet + * buffer. The parser must accept a prefix only if the attribute value it + * announces really fits in the received AVP. + */ +static void test_hidden_avp_length_prefix(void) +{ + static const struct { + const char *name; + size_t attr_len; /* ciphered attribute length */ + uint16_t declared; /* deciphered length prefix */ + int accept; + } cases[] = { + { "lies about 64K", 16, 0xffff, 0 }, + { "lies, minimal avp", 2, 0xffff, 0 }, + { "off by one", 16, 15, 0 }, + { "one byte too big", 4, 3, 0 }, + { "fits exactly", 16, 14, 1 }, + { "fits", 16, 4, 1 }, + { "empty value", 2, 0, 1 }, + { "no length prefix", 1, 0, 0 }, + }; + static const uint8_t rv[16] = { + 0xf3, 0x1a, 0x00, 0xff, 0x42, 0x7c, 0x91, 0x08, + 0x5d, 0xe6, 0x33, 0xb0, 0x14, 0xaa, 0x69, 0xc2, + }; + uint8_t value[MD5_DIGEST_LENGTH]; + struct l2tp_packet_t *pack; + const struct l2tp_attr_t *attr; + struct pktbuf pkt; + uint16_t declared; + size_t indx, i; + + for (indx = 0; indx < sizeof(cases) / sizeof(cases[0]); ++indx) { + pkt_init(&pkt); + pkt_add_avp(&pkt, L2TP_AVP_FLAG_M, Random_Vector, rv, sizeof(rv)); + + /* Cleartext: 2 bytes length prefix, then the value, then padding. + The value is a recognizable pattern so that a short read shows + up as wrong content rather than as a lucky pass. */ + memset(value, 0, sizeof(value)); + declared = htons(cases[indx].declared); + memcpy(value, &declared, cases[indx].attr_len < sizeof(declared) + ? cases[indx].attr_len : sizeof(declared)); + for (i = sizeof(declared); i < cases[indx].attr_len; ++i) + value[i] = 'a' + (i % 26); + + hide_single_block(value, cases[indx].attr_len, Host_Name, + secret, sizeof(secret) - 1, rv, sizeof(rv)); + pkt_add_avp(&pkt, L2TP_AVP_FLAG_M | L2TP_AVP_FLAG_H, Host_Name, + value, cases[indx].attr_len); + pkt_finish(&pkt); + + pack = parse(&pkt); + if (!cases[indx].accept) { + if (pack) { + fprintf(stderr, "FAIL %s:%d: hidden avp accepted" + " (%s)\n", __FILE__, __LINE__, + cases[indx].name); + failures++; + l2tp_packet_free(pack); + } + continue; + } + + if (!pack) { + fprintf(stderr, "FAIL %s:%d: hidden avp rejected (%s)\n", + __FILE__, __LINE__, cases[indx].name); + failures++; + continue; + } + + attr = find_attr(pack, Host_Name); + CHECK(attr != NULL); + if (attr) { + CHECK(attr->length == cases[indx].declared); + for (i = 0; i < cases[indx].declared; ++i) + CHECK((uint8_t)attr->val.string[i] == + 'a' + ((i + sizeof(declared)) % 26)); + CHECK(attr->val.string[cases[indx].declared] == '\0'); + } + l2tp_packet_free(pack); + } +} + +/* + * A hidden AVP is rejected outright when no Random Vector was received, or + * when its length cannot even hold the length prefix. + */ +static void test_hidden_avp_prerequisites(void) +{ + static const uint8_t value[16] = { 0 }; + struct l2tp_packet_t *pack; + struct pktbuf pkt; + + pkt_init(&pkt); + pkt_add_avp(&pkt, L2TP_AVP_FLAG_M | L2TP_AVP_FLAG_H, Host_Name, + value, sizeof(value)); + pkt_finish(&pkt); + pack = parse(&pkt); + CHECK(pack == NULL); + if (pack) + l2tp_packet_free(pack); + + /* Random Vector present, but the hidden AVP carries no value at all */ + pkt_init(&pkt); + pkt_add_avp(&pkt, L2TP_AVP_FLAG_M, Random_Vector, value, sizeof(value)); + pkt_add_avp(&pkt, L2TP_AVP_FLAG_M | L2TP_AVP_FLAG_H, Host_Name, NULL, 0); + pkt_finish(&pkt); + pack = parse(&pkt); + CHECK(pack == NULL); + if (pack) + l2tp_packet_free(pack); +} + +/* + * Round trip through the real encoder. With hide_avps set every attribute but + * Message-Type and Random-Vector goes through the multi block cipher, since + * encode_attr() always appends at least 16 bytes of padding. + */ +static void test_roundtrip(int hide_avps) +{ + static const char host_name[] = "accel-ppp regression test host name"; + struct l2tp_packet_t *pack; + const struct l2tp_attr_t *attr; + int ret; + + pack = l2tp_packet_alloc(2, Message_Type_Hello, &sock_addr, hide_avps, + secret, sizeof(secret) - 1); + CHECK(pack != NULL); + if (!pack) + return; + + /* Odd length string first: everything after it sits on an odd offset, + so the integer accessors below run unaligned */ + CHECK(l2tp_packet_add_string(pack, Host_Name, host_name, 1) == 0); + CHECK(l2tp_packet_add_int16(pack, Assigned_Tunnel_ID, 0x1234, 1) == 0); + CHECK(l2tp_packet_add_int32(pack, Call_Serial_Number, 0x89abcdef, 1) == 0); + CHECK(l2tp_packet_add_int64(pack, Tie_Breaker, 0x0123456789abcdefULL, 0) == 0); + + ret = l2tp_packet_send(sock, pack); + CHECK(ret == 0); + l2tp_packet_free(pack); + if (ret < 0) + return; + + pack = NULL; + CHECK(l2tp_recv(sock, &pack, NULL, secret, sizeof(secret) - 1) == 0); + CHECK(pack != NULL); + if (!pack) + return; + + attr = find_attr(pack, Message_Type); + CHECK(attr && attr->val.uint16 == Message_Type_Hello); + attr = find_attr(pack, Host_Name); + CHECK(attr && attr->length == (int)strlen(host_name)); + CHECK(attr && strcmp(attr->val.string, host_name) == 0); + attr = find_attr(pack, Assigned_Tunnel_ID); + CHECK(attr && attr->val.uint16 == 0x1234); + attr = find_attr(pack, Call_Serial_Number); + CHECK(attr && attr->val.uint32 == 0x89abcdef); + attr = find_attr(pack, Tie_Breaker); + CHECK(attr && attr->val.uint64 == 0x0123456789abcdefULL); + + l2tp_packet_free(pack); +} + +/* + * A hidden AVP deciphered with the wrong secret yields a random length + * prefix. Whatever it is, the parser must not read outside the AVP. + */ +static void test_wrong_secret(void) +{ + struct l2tp_packet_t *pack; + struct pktbuf pkt; + uint8_t buf[1024]; + size_t len, indx; + int ret; + + pack = l2tp_packet_alloc(2, Message_Type_Hello, &sock_addr, 1, + secret, sizeof(secret) - 1); + CHECK(pack != NULL); + if (!pack) + return; + + CHECK(l2tp_packet_add_string(pack, Host_Name, "hidden", 1) == 0); + CHECK(l2tp_packet_send(sock, pack) == 0); + l2tp_packet_free(pack); + + len = recv(sock, buf, sizeof(buf), 0); + CHECK(len > 0); + + /* Same bytes on the wire, every other secret at the receiving end */ + for (indx = 0; indx < 64; ++indx) { + char wrong[8]; + + snprintf(wrong, sizeof(wrong), "wrong%02zu", indx); + memcpy(pkt.data, buf, len); + pkt.len = len; + + if (sendto(sock, pkt.data, pkt.len, 0, + (struct sockaddr *)&sock_addr, sizeof(sock_addr)) < 0) { + perror("sendto"); + exit(1); + } + pack = NULL; + ret = l2tp_recv(sock, &pack, NULL, wrong, strlen(wrong)); + CHECK(ret == 0); + if (pack) { + /* Accepting is fine (the random prefix may happen to be + plausible), reading out of the AVP is not */ + const struct l2tp_attr_t *attr = find_attr(pack, Host_Name); + + CHECK(!attr || attr->length <= (int)len); + l2tp_packet_free(pack); + } + } +} + +int main(void) +{ + loopback_socket(); + + test_hidden_avp_length_prefix(); + test_hidden_avp_prerequisites(); + test_roundtrip(0); + test_roundtrip(1); + test_wrong_secret(); + + close(sock); + + if (failures) { + fprintf(stderr, "%d failure(s)\n", failures); + return 1; + } + + printf("all tests passed\n"); + + return 0; +} -- cgit v1.2.3 From 4e36e08e90dbd3c27f566ca396f4284b77f1bf40 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 4 Aug 2026 17:50:49 +0300 Subject: crypto: drop the dangling crypto.h symlink and its last references c912d090 ("crypto: Removed internal tomcat crypto.") deleted the crypto/ tree but left accel-pppd/include/crypto.h behind, a tracked symlink to ../../crypto/crypto.h which has pointed at nothing since. backup_file.c still includes it, so it fails to compile with fatal error: crypto.h: No such file or directory That goes unnoticed because accel-pppd/CMakeLists.txt has ADD_SUBDIRECTORY(backup) commented out, i.e. backup_file.c is not part of any build; the breakage only shows up for whoever re-enables it. Include instead, which is what the file actually needs (MD5_CTX and friends) and what c912d090 did for every file it touched. sstp.c and radius/packet.c defer to crypto.h for the rationale behind their OPENSSL_API_COMPAT define. Spell it out locally instead, and point at the project wide ADD_DEFINITIONS() in the top level CMakeLists.txt added by c1689506 ("openssl: suppress deprecated API warnings"), noting why the local define is kept despite being redundant with it: it has to be visible before the first OpenSSL header. Then remove the symlink, which nothing references anymore. --- accel-pppd/backup/backup_file.c | 3 ++- accel-pppd/ctrl/sstp/sstp.c | 7 +++++-- accel-pppd/include/crypto.h | 1 - accel-pppd/radius/packet.c | 7 +++++-- 4 files changed, 12 insertions(+), 6 deletions(-) delete mode 120000 accel-pppd/include/crypto.h diff --git a/accel-pppd/backup/backup_file.c b/accel-pppd/backup/backup_file.c index 06dc02d4..472694f0 100644 --- a/accel-pppd/backup/backup_file.c +++ b/accel-pppd/backup/backup_file.c @@ -10,11 +10,12 @@ #include #include +#include + #include "triton.h" #include "log.h" #include "ap_session.h" #include "backup.h" -#include "crypto.h" #include "memdebug.h" #define VERSION 1 diff --git a/accel-pppd/ctrl/sstp/sstp.c b/accel-pppd/ctrl/sstp/sstp.c index 2fd8cb35..db08ae93 100644 --- a/accel-pppd/ctrl/sstp/sstp.c +++ b/accel-pppd/ctrl/sstp/sstp.c @@ -21,8 +21,11 @@ #include "linux_ppp.h" /* - * Suppress OpenSSL 3.0 deprecation warnings for DH API. - * See crypto.h for detailed explanation. + * Suppress OpenSSL 3.0 deprecation warnings for the DH API: it is deprecated + * but still functional, and still required for protocol compatibility. The + * project sets this for every target (see the top level CMakeLists.txt); it + * is repeated here because it only takes effect if it is defined before the + * first OpenSSL header is pulled in. */ #define OPENSSL_API_COMPAT 0x10100000L #include diff --git a/accel-pppd/include/crypto.h b/accel-pppd/include/crypto.h deleted file mode 120000 index 2f3f63cb..00000000 --- a/accel-pppd/include/crypto.h +++ /dev/null @@ -1 +0,0 @@ -../../crypto/crypto.h \ No newline at end of file diff --git a/accel-pppd/radius/packet.c b/accel-pppd/radius/packet.c index cfc0bc29..c7e91349 100644 --- a/accel-pppd/radius/packet.c +++ b/accel-pppd/radius/packet.c @@ -10,8 +10,11 @@ #include /* - * Suppress OpenSSL 3.0 deprecation warnings for HMAC API. - * See crypto.h for detailed explanation. + * Suppress OpenSSL 3.0 deprecation warnings for the HMAC API: it is + * deprecated but still functional, and still required for protocol + * compatibility. The project sets this for every target (see the top level + * CMakeLists.txt); it is repeated here because it only takes effect if it is + * defined before the first OpenSSL header is pulled in. */ #define OPENSSL_API_COMPAT 0x10100000L #include -- cgit v1.2.3 From 58ef850d1705d88f006d761d32c55bde5205a27d Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 4 Aug 2026 18:39:51 +0300 Subject: ipv6: assign DNS servers per session from RADIUS The ipv6_nd and ipv6_dhcp modules could only advertise the DNS servers configured in [ipv6-dns], the same set for every subscriber. RFC 6911 defines DNS-Server-IPv6-Address (attribute 169) for exactly this, and the attribute was already in the shipped dictionary and in attr_defs.h; nothing read it. Give struct ap_session an ipv6_dns list, filled by the radius module from that attribute, and have both modules advertise it when the session has one: in the RDNSS option of the router advertisements, and in the DNS_SERVERS option of DHCPv6 replies. Sessions without a list of their own keep getting the configured servers, so nothing changes for anyone not sending the attribute. The selection is a single ipv6_dns_get() shared by both modules rather than a copy in each: they already duplicate the whole [ipv6-dns] parser, and two copies of a precedence rule are two chances to drift. It caps what it returns, so neither the RDNSS option length (one byte, in units of 8) nor the router advertisement buffer can be pushed around by what a RADIUS server sends. The radius module caps at the same 3 servers as [ipv6-dns] accepts and warns once when a reply carries more. An Access-Accept which carries the attribute replaces the whole previously assigned list rather than appending to it, so a re-authorized session ends up with the servers of the latest reply and not with a concatenation. One which does not carry it leaves the current list alone, which is how the IPv4 MS-Primary-DNS-Server attribute already behaves. ipv6_dns_test.c covers the selection: assigned wins over configured, empty list means "nothing assigned" rather than "no DNS", the cap holds for both sources, and a caller with no room gets nothing rather than a stomped buffer. Wired into the ASAN/UBSAN workflow. Inspired by the per-session IPv6 DNS support in accel-ppp-ng (commit 2df6eb99), reimplemented against mainline's ap_session and ipdb types. --- .github/workflows/run-tests-asan-ubsan.yml | 5 + accel-pppd/accel-ppp.conf | 2 + accel-pppd/accel-ppp.conf.5 | 6 ++ accel-pppd/include/ap_session.h | 6 ++ accel-pppd/include/ipv6_dns.h | 48 +++++++++ accel-pppd/ipv6/dhcpv6.c | 14 ++- accel-pppd/ipv6/ipv6_dns_test.c | 157 +++++++++++++++++++++++++++++ accel-pppd/ipv6/nd.c | 14 ++- accel-pppd/radius/radius.c | 50 +++++++++ accel-pppd/radius/radius_p.h | 1 + 10 files changed, 293 insertions(+), 10 deletions(-) create mode 100644 accel-pppd/include/ipv6_dns.h create mode 100644 accel-pppd/ipv6/ipv6_dns_test.c diff --git a/.github/workflows/run-tests-asan-ubsan.yml b/.github/workflows/run-tests-asan-ubsan.yml index 8b25701e..90074463 100644 --- a/.github/workflows/run-tests-asan-ubsan.yml +++ b/.github/workflows/run-tests-asan-ubsan.yml @@ -72,6 +72,11 @@ jobs: accel-pppd/ctrl/l2tp/packet_test.c accel-pppd/ctrl/l2tp/packet.c \ -lcrypto /tmp/l2tp_packet_test + gcc -O2 -Wall -D_GNU_SOURCE -DAP_SESSIONID_LEN=16 \ + -fsanitize=${{ matrix.sanitizer }} -fno-sanitize-recover=all \ + -I accel-pppd/include -I accel-pppd/triton -I build \ + -o /tmp/ipv6_dns_test accel-pppd/ipv6/ipv6_dns_test.c + /tmp/ipv6_dns_test gcc -O2 -Wall -o /tmp/bitpool_test accel-pppd/extra/bitpool_test.c /tmp/bitpool_test diff --git a/accel-pppd/accel-ppp.conf b/accel-pppd/accel-ppp.conf index e34ae919..c32e7612 100644 --- a/accel-pppd/accel-ppp.conf +++ b/accel-pppd/accel-ppp.conf @@ -415,6 +415,8 @@ delegate=fc00:2::/36,48,name=pool3 delegate=fc00:3::/36,48,name=pool4,next=pool3 [ipv6-dns] +# Used for sessions which were not assigned their own DNS servers, e.g. by the +# RADIUS DNS-Server-IPv6-Address attribute (RFC 6911) #fc00:1::1 #fc00:1::2 #fc00:1::3 diff --git a/accel-pppd/accel-ppp.conf.5 b/accel-pppd/accel-ppp.conf.5 index 07fb363d..7c042541 100644 --- a/accel-pppd/accel-ppp.conf.5 +++ b/accel-pppd/accel-ppp.conf.5 @@ -671,6 +671,12 @@ Specifies primary NBNS to be sent to peer. .BI "wins2=" x.x.x.x Specifies secondary NBNS to be sent to peer. .SH [ipv6-dns] +These options apply to sessions which were not assigned DNS servers of their +own. A RADIUS server may assign per session ones by returning up to 3 +DNS-Server-IPv6-Address attributes (RFC 6911) in the Access-Accept; those +replace, rather than extend, the servers configured here, for that session +only. Both the ipv6_nd module (RDNSS option of the router advertisements) and +the ipv6_dhcp module (DNS_SERVERS option) honour them. .TP .BI "dns=" IPv6_address Specifies IPv6 DNS to be sent to peer. You may specify up to 3 dns options. diff --git a/accel-pppd/include/ap_session.h b/accel-pppd/include/ap_session.h index a4d3b867..a0733986 100644 --- a/accel-pppd/include/ap_session.h +++ b/accel-pppd/include/ap_session.h @@ -85,6 +85,12 @@ struct ap_session struct ipv4db_item_t *ipv4; struct ipv6db_item_t *ipv6; struct ipv6db_prefix_t *ipv6_dp; + /* Per session IPv6 DNS servers, NULL when none were assigned and the + globally configured ones ([ipv6-dns]) should be advertised instead. + Only addr_list, and only the addr member of its entries, is + meaningful here. Owned by whoever sets it, currently the radius + module from the DNS-Server-IPv6-Address attribute (RFC 6911) */ + struct ipv6db_item_t *ipv6_dns; char *ipv4_pool_name; char *ipv6_pool_name; char *dpv6_pool_name; diff --git a/accel-pppd/include/ipv6_dns.h b/accel-pppd/include/ipv6_dns.h new file mode 100644 index 00000000..b604d006 --- /dev/null +++ b/accel-pppd/include/ipv6_dns.h @@ -0,0 +1,48 @@ +#ifndef __IPV6_DNS_H +#define __IPV6_DNS_H + +#include + +#include "list.h" +#include "ipdb.h" +#include "ap_session.h" + +/* + * Pick the IPv6 DNS servers to advertise to a session. + * + * A session may have been assigned its own servers (currently by the radius + * module, from the DNS-Server-IPv6-Address attribute of RFC 6911); those take + * precedence. Sessions without any fall back to the globally configured ones, + * which is what every session got before per session servers existed. + * + * Up to 'max' addresses are written to 'dns', the number written is returned. + * Callers advertise nothing when that is 0. + */ +static inline int ipv6_dns_get(const struct ap_session *ses, + const struct in6_addr *conf_dns, int conf_dns_count, + struct in6_addr *dns, int max) +{ + struct ipv6db_addr_t *a; + int count = 0; + + if (ses && ses->ipv6_dns) { + list_for_each_entry(a, &ses->ipv6_dns->addr_list, entry) { + if (count == max) + break; + dns[count++] = a->addr; + } + + /* An empty list means "nothing assigned", not "no DNS at all" */ + if (count) + return count; + } + + while (count < conf_dns_count && count < max) { + dns[count] = conf_dns[count]; + count++; + } + + return count; +} + +#endif diff --git a/accel-pppd/ipv6/dhcpv6.c b/accel-pppd/ipv6/dhcpv6.c index b284df5f..4a80bdb9 100644 --- a/accel-pppd/ipv6/dhcpv6.c +++ b/accel-pppd/ipv6/dhcpv6.c @@ -20,6 +20,7 @@ #include "log.h" #include "ppp.h" #include "ipdb.h" +#include "ipv6_dns.h" #include "events.h" #include "iputils.h" @@ -220,16 +221,19 @@ static void insert_status(struct dhcpv6_packet *pkt, struct dhcpv6_option *opt, static void insert_oro(struct dhcpv6_packet *reply, struct dhcpv6_option *opt) { struct dhcpv6_option *opt1; - int i, j; + int i, j, dns_count; uint16_t *ptr; struct in6_addr addr, *addr_ptr; + struct in6_addr dns[MAX_DNS_COUNT]; for (i = ntohs(opt->hdr->len) / 2, ptr = (uint16_t *)opt->hdr->data; i; i--, ptr++) { if (ntohs(*ptr) == D6_OPTION_DNS_SERVERS) { - if (conf_dns_count) { - opt1 = dhcpv6_option_alloc(reply, D6_OPTION_DNS_SERVERS, conf_dns_count * sizeof(addr)); - for (j = 0, addr_ptr = (struct in6_addr *)opt1->hdr->data; j < conf_dns_count; j++, addr_ptr++) - memcpy(addr_ptr, conf_dns + j, sizeof(addr)); + dns_count = ipv6_dns_get(reply->ses, conf_dns, conf_dns_count, + dns, MAX_DNS_COUNT); + if (dns_count) { + opt1 = dhcpv6_option_alloc(reply, D6_OPTION_DNS_SERVERS, dns_count * sizeof(addr)); + for (j = 0, addr_ptr = (struct in6_addr *)opt1->hdr->data; j < dns_count; j++, addr_ptr++) + memcpy(addr_ptr, dns + j, sizeof(addr)); } } else if (ntohs(*ptr) == D6_OPTION_DOMAIN_LIST) { if (conf_dnssl_size) { diff --git a/accel-pppd/ipv6/ipv6_dns_test.c b/accel-pppd/ipv6/ipv6_dns_test.c new file mode 100644 index 00000000..72b4751c --- /dev/null +++ b/accel-pppd/ipv6/ipv6_dns_test.c @@ -0,0 +1,157 @@ +/* + * Standalone test for the IPv6 DNS server selection shared by the ipv6_nd and + * ipv6_dhcp modules. + * + * Not part of the cmake build. Compile and run from the top of the tree, with + * a configured build directory around for config.h: + * gcc -O2 -Wall -D_GNU_SOURCE -DAP_SESSIONID_LEN=16 \ + * -I accel-pppd/include -I accel-pppd/triton -I build \ + * -o /tmp/ipv6_dns_test accel-pppd/ipv6/ipv6_dns_test.c && /tmp/ipv6_dns_test + */ +#include +#include +#include +#include + +#include "ipv6_dns.h" + +static int failures; +#define CHECK(cond) do { if (!(cond)) { \ + fprintf(stderr, "FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); failures++; } } while (0) + +#define MAX_DNS_COUNT 3 /* as in nd.c and dhcpv6.c */ + +static struct in6_addr a6(const char *str) +{ + struct in6_addr addr; + + if (inet_pton(AF_INET6, str, &addr) != 1) { + fprintf(stderr, "bad address %s\n", str); + exit(1); + } + + return addr; +} + +static int is(const struct in6_addr *addr, const char *str) +{ + struct in6_addr expect = a6(str); + + return memcmp(addr, &expect, sizeof(expect)) == 0; +} + +/* A session carrying 'count' DNS servers taken from 'str' */ +static struct ap_session *session_with(const char **str, int count) +{ + struct ap_session *ses = calloc(1, sizeof(*ses)); + struct ipv6db_item_t *item = calloc(1, sizeof(*item)); + int i; + + INIT_LIST_HEAD(&item->addr_list); + for (i = 0; i < count; i++) { + struct ipv6db_addr_t *a = calloc(1, sizeof(*a)); + + a->addr = a6(str[i]); + a->prefix_len = 128; + list_add_tail(&a->entry, &item->addr_list); + } + + ses->ipv6_dns = item; + + return ses; +} + +static void session_free(struct ap_session *ses) +{ + if (ses->ipv6_dns) { + while (!list_empty(&ses->ipv6_dns->addr_list)) { + struct ipv6db_addr_t *a = list_entry(ses->ipv6_dns->addr_list.next, + typeof(*a), entry); + + list_del(&a->entry); + free(a); + } + free(ses->ipv6_dns); + } + + free(ses); +} + +int main(void) +{ + static const char *four[] = { "2001:db8::1", "2001:db8::2", + "2001:db8::3", "2001:db8::4" }; + struct in6_addr conf_dns[MAX_DNS_COUNT]; + struct in6_addr dns[MAX_DNS_COUNT]; + struct ap_session *ses; + int n; + + conf_dns[0] = a6("fc00::53"); + conf_dns[1] = a6("fc00::54"); + + /* No session at all: the configured servers, as before the feature */ + n = ipv6_dns_get(NULL, conf_dns, 2, dns, MAX_DNS_COUNT); + CHECK(n == 2); + CHECK(is(&dns[0], "fc00::53")); + CHECK(is(&dns[1], "fc00::54")); + + /* Session without assigned servers: same fallback */ + ses = calloc(1, sizeof(*ses)); + n = ipv6_dns_get(ses, conf_dns, 2, dns, MAX_DNS_COUNT); + CHECK(n == 2); + CHECK(is(&dns[0], "fc00::53")); + free(ses); + + /* Nothing configured and nothing assigned: advertise nothing */ + n = ipv6_dns_get(NULL, conf_dns, 0, dns, MAX_DNS_COUNT); + CHECK(n == 0); + + /* Assigned servers win over the configured ones */ + ses = session_with(four, 2); + n = ipv6_dns_get(ses, conf_dns, 2, dns, MAX_DNS_COUNT); + CHECK(n == 2); + CHECK(is(&dns[0], "2001:db8::1")); + CHECK(is(&dns[1], "2001:db8::2")); + + /* ... and win even when nothing is configured */ + n = ipv6_dns_get(ses, conf_dns, 0, dns, MAX_DNS_COUNT); + CHECK(n == 2); + CHECK(is(&dns[0], "2001:db8::1")); + + session_free(ses); + + /* An empty assigned list is "nothing assigned", not "no DNS" */ + ses = session_with(four, 0); + n = ipv6_dns_get(ses, conf_dns, 2, dns, MAX_DNS_COUNT); + CHECK(n == 2); + CHECK(is(&dns[0], "fc00::53")); + session_free(ses); + + /* More assigned than fit: keep the first max, never overrun */ + ses = session_with(four, 4); + memset(dns, 0, sizeof(dns)); + n = ipv6_dns_get(ses, conf_dns, 2, dns, MAX_DNS_COUNT); + CHECK(n == MAX_DNS_COUNT); + CHECK(is(&dns[0], "2001:db8::1")); + CHECK(is(&dns[1], "2001:db8::2")); + CHECK(is(&dns[2], "2001:db8::3")); + + /* Same for the configured ones, should they ever exceed max */ + n = ipv6_dns_get(NULL, conf_dns, 99, dns, MAX_DNS_COUNT); + CHECK(n == MAX_DNS_COUNT); + + /* A caller with no room gets nothing rather than a stomped buffer */ + n = ipv6_dns_get(ses, conf_dns, 2, dns, 0); + CHECK(n == 0); + + session_free(ses); + + if (failures) { + fprintf(stderr, "%d failure(s)\n", failures); + return 1; + } + + printf("all tests passed\n"); + + return 0; +} diff --git a/accel-pppd/ipv6/nd.c b/accel-pppd/ipv6/nd.c index 6dd00ee2..8b22da14 100644 --- a/accel-pppd/ipv6/nd.c +++ b/accel-pppd/ipv6/nd.c @@ -17,6 +17,7 @@ #include "events.h" #include "mempool.h" #include "ipdb.h" +#include "ipv6_dns.h" #include "iputils.h" #include "memdebug.h" @@ -107,7 +108,8 @@ static void ipv6_nd_send_ra(struct ipv6_nd_handler_t *h, struct sockaddr_in6 *ds //struct nd_opt_mtu *mtu; struct ipv6db_addr_t *a; struct in6_addr addr, peer_addr; - int i, prefix_len; + struct in6_addr dns[MAX_DNS_COUNT]; + int i, prefix_len, dns_count; if (!buf) { log_emerg("out of memory\n"); @@ -174,15 +176,17 @@ static void ipv6_nd_send_ra(struct ipv6_nd_handler_t *h, struct sockaddr_in6 *ds rinfo++; }*/ - if (conf_dns_count) { + dns_count = ipv6_dns_get(ses, conf_dns, conf_dns_count, dns, MAX_DNS_COUNT); + + if (dns_count) { rdnssinfo = (struct nd_opt_rdnss_info_local *)pinfo; memset(rdnssinfo, 0, sizeof(*rdnssinfo)); rdnssinfo->nd_opt_rdnssi_type = ND_OPT_RDNSS_INFORMATION; - rdnssinfo->nd_opt_rdnssi_len = 1 + 2 * conf_dns_count; + rdnssinfo->nd_opt_rdnssi_len = 1 + 2 * dns_count; rdnssinfo->nd_opt_rdnssi_lifetime = htonl(conf_rdnss_lifetime); rdnss_addr = (struct in6_addr *)rdnssinfo->nd_opt_rdnssi; - for (i = 0; i < conf_dns_count; i++) { - memcpy(rdnss_addr, &conf_dns[i], sizeof(*rdnss_addr)); + for (i = 0; i < dns_count; i++) { + memcpy(rdnss_addr, &dns[i], sizeof(*rdnss_addr)); rdnss_addr++; } } else diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c index cb53f59a..fa77a916 100644 --- a/accel-pppd/radius/radius.c +++ b/accel-pppd/radius/radius.c @@ -488,12 +488,31 @@ err: return -1; } +/* + * Number of IPv6 DNS servers kept per session. Matches the number of dns= + * options the ipv6_nd and ipv6_dhcp modules accept in [ipv6-dns], and keeps + * the RDNSS option of a router advertisement to a sane size. + */ +#define MAX_DNS6_COUNT 3 + +static void free_ipv6_dns(struct radius_pd_t *rpd) +{ + struct ipv6db_addr_t *a; + + while (!list_empty(&rpd->ipv6_dns.addr_list)) { + a = list_entry(rpd->ipv6_dns.addr_list.next, typeof(*a), entry); + list_del(&a->entry); + _free(a); + } +} + int rad_proc_attrs(struct rad_req_t *req) { struct ev_wins_t wins = {}; struct ev_dns_t dns = {}; struct rad_attr_t *attr; struct ipv6db_addr_t *a; + int dns6_count = -1; int res = 0; struct radius_pd_t *rpd = req->rpd; @@ -602,6 +621,28 @@ int rad_proc_attrs(struct rad_req_t *req) a->addr = attr->val.ipv6prefix.prefix; list_add_tail(&a->entry, &rpd->ipv6_dp.prefix_list); break; + case DNS_Server_IPv6_Address: + if (dns6_count < 0) { + /* This reply carries a DNS server list of + its own, it replaces whatever a previous + one assigned */ + free_ipv6_dns(rpd); + dns6_count = 0; + } + if (dns6_count >= MAX_DNS6_COUNT) { + if (dns6_count == MAX_DNS6_COUNT) + log_ppp_warn("radius: ignoring DNS-Server-IPv6-Address" + " beyond the first %i\n", MAX_DNS6_COUNT); + dns6_count++; + break; + } + a = _malloc(sizeof(*a)); + memset(a, 0, sizeof(*a)); + a->prefix_len = 128; + a->addr = attr->val.ipv6addr; + list_add_tail(&a->entry, &rpd->ipv6_dns.addr_list); + dns6_count++; + break; case NAS_Port: rpd->ses->unit_idx = attr->val.integer; break; @@ -635,6 +676,11 @@ int rad_proc_attrs(struct rad_req_t *req) if (!rpd->ses->ipv6_dp && !list_empty(&rpd->ipv6_dp.prefix_list)) rpd->ses->ipv6_dp = &rpd->ipv6_dp; + /* Like the IPv4 DNS servers, absent attributes leave whatever a + previous reply assigned in place */ + if (!list_empty(&rpd->ipv6_dns.addr_list)) + rpd->ses->ipv6_dns = &rpd->ipv6_dns; + return res; } @@ -799,6 +845,7 @@ static void ses_starting(struct ap_session *ses) INIT_LIST_HEAD(&rpd->plugin_list); INIT_LIST_HEAD(&rpd->ipv6_addr.addr_list); INIT_LIST_HEAD(&rpd->ipv6_dp.prefix_list); + INIT_LIST_HEAD(&rpd->ipv6_dns.addr_list); rpd->ipv4_addr.owner = &ipdb; rpd->ipv6_addr.owner = &ipdb; @@ -981,6 +1028,9 @@ static void ses_finished(struct ap_session *ses) _free(a); } + ses->ipv6_dns = NULL; + free_ipv6_dns(rpd); + fr6 = rpd->fr6; while (fr6) { struct framed_ip6_route *next = fr6->next; diff --git a/accel-pppd/radius/radius_p.h b/accel-pppd/radius/radius_p.h index d3a72204..e4b84740 100644 --- a/accel-pppd/radius/radius_p.h +++ b/accel-pppd/radius/radius_p.h @@ -65,6 +65,7 @@ struct radius_pd_t { struct ipv4db_item_t ipv4_addr; struct ipv6db_item_t ipv6_addr; struct ipv6db_prefix_t ipv6_dp; + struct ipv6db_item_t ipv6_dns; int acct_interim_interval; int acct_interim_jitter; -- cgit v1.2.3