From 1b67ba0785a847eee6adb69b7e68995c33803805 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Thu, 21 May 2026 15:50:42 +0300 Subject: strip: Fix invalid parsing strip() memmove count was one short, dropping the NULL terminator; dpado_parse error path leaked already-parsed range entries. Also affects ipoe. Since strip is identical in both, we can place fixed common function in utils. Reported-by: Khedor Signed-off-by: Denys Fedoryshchenko --- accel-pppd/ctrl/ipoe/ipoe.c | 32 +++++++++++++++----------------- accel-pppd/ctrl/pppoe/dpado.c | 33 ++++++++++++++++----------------- accel-pppd/utils.c | 11 +++++++++++ accel-pppd/utils.h | 1 + 4 files changed, 43 insertions(+), 34 deletions(-) diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c index 5592845f..4bf43434 100644 --- a/accel-pppd/ctrl/ipoe/ipoe.c +++ b/accel-pppd/ctrl/ipoe/ipoe.c @@ -3574,19 +3574,6 @@ static void load_radius_attrs(void) } #endif -static void strip(char *str) -{ - char *ptr = str; - char *endptr = strchr(str, 0); - while (1) { - ptr = strchr(ptr, ' '); - if (ptr) - memmove(ptr, ptr + 1, endptr - ptr - 1); - else - break; - } -} - int parse_offer_delay(const char *str) { char *str1; @@ -3603,7 +3590,7 @@ int parse_offer_delay(const char *str) return 0; str1 = _strdup(str); - strip(str1); + u_strstrip(str1, ' '); ptr1 = str1; @@ -3619,17 +3606,23 @@ int parse_offer_delay(const char *str) memset(r, 0, sizeof(*r)); r->delay = strtol(ptr1, &endptr, 10); - if (*endptr) + if (*endptr) { + _free(r); goto out_err; + } if (list_empty(&conf_offer_delay)) r->conn_cnt = 0; else { - if (!ptr3) + if (!ptr3) { + _free(r); goto out_err; + } r->conn_cnt = strtol(ptr3 + 1, &endptr, 10); - if (*endptr) + if (*endptr) { + _free(r); goto out_err; + } } list_add_tail(&r->entry, &conf_offer_delay); @@ -3644,6 +3637,11 @@ int parse_offer_delay(const char *str) return 0; out_err: + while (!list_empty(&conf_offer_delay)) { + r = list_entry(conf_offer_delay.next, typeof(*r), entry); + list_del(&r->entry); + _free(r); + } _free(str1); log_error("ipoe: failed to parse offer-delay\n"); return -1; diff --git a/accel-pppd/ctrl/pppoe/dpado.c b/accel-pppd/ctrl/pppoe/dpado.c index 351a43dc..dc570dda 100644 --- a/accel-pppd/ctrl/pppoe/dpado.c +++ b/accel-pppd/ctrl/pppoe/dpado.c @@ -11,6 +11,7 @@ #include "triton.h" #include "log.h" #include "memdebug.h" +#include "utils.h" #include "pppoe.h" @@ -60,19 +61,6 @@ void dpado_check_prev(int conn_cnt) pthread_mutex_unlock(&dpado_range_lock); } -static void strip(char *str) -{ - char *ptr = str; - char *endptr = strchr(str, 0); - while (1) { - ptr = strchr(ptr, ' '); - if (ptr) - memmove(ptr, ptr + 1, endptr - ptr - 1); - else - break; - } -} - int dpado_parse(const char *str) { char *str1 = _strdup(str); @@ -81,7 +69,7 @@ int dpado_parse(const char *str) LIST_HEAD(range_list); struct dpado_range_t *r; - strip(str1); + u_strstrip(str1, ' '); ptr1 = str1; @@ -97,17 +85,23 @@ int dpado_parse(const char *str) memset(r, 0, sizeof(*r)); r->pado_delay = strtol(ptr1, &endptr, 10); - if (*endptr) + if (*endptr) { + _free(r); goto out_err; + } if (list_empty(&range_list)) r->conn_cnt = INT_MAX; else { - if (!ptr3) + if (!ptr3) { + _free(r); goto out_err; + } r->conn_cnt = strtol(ptr3 + 1, &endptr, 10); - if (*endptr) + if (*endptr) { + _free(r); goto out_err; + } } list_add_tail(&r->entry, &range_list); @@ -160,6 +154,11 @@ int dpado_parse(const char *str) return 0; out_err: + while (!list_empty(&range_list)) { + r = list_entry(range_list.next, typeof(*r), entry); + list_del(&r->entry); + _free(r); + } _free(str1); log_emerg("pppoe: pado_delay: invalid format\n"); return -1; diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c index 018b6efa..f92ebe19 100644 --- a/accel-pppd/utils.c +++ b/accel-pppd/utils.c @@ -347,3 +347,14 @@ int __export u_randbuf(void *buf, size_t buf_len, int *err) return 0; } + +void __export u_strstrip(char *str, char c) +{ + char *src = str, *dst = str; + while (*src) { + if (*src != c) + *dst++ = *src; + src++; + } + *dst = '\0'; +} diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h index 06859a6b..aad4025f 100644 --- a/accel-pppd/utils.h +++ b/accel-pppd/utils.h @@ -24,6 +24,7 @@ size_t u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t *plen); size_t u_parse_ip4cidr(const char *str, struct in_addr *netp, uint8_t *plen); size_t u_parse_ip4range(const char *str, struct in_addr *base_ip, uint8_t *max); +void u_strstrip(char *str, char c); int u_randbuf(void *buf, size_t buf_len, int *err); #endif -- cgit v1.2.3 From 22c61e8e2e16ebe8e9576fe46487e3614982e8ac Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Thu, 21 May 2026 17:03:17 +0300 Subject: fix: dict.c: split() could return 0, making dict_load read ptr[-1] If a RADIUS dictionary contains a line consisting of exactly one word with no trailing spaces (for example, standard keywords like "END-VENDOR\n" or "END-TLV\n" ), this bug getting triggered. Triggering crash is compiler dependent, it might not happen now, but a bit different compiler, flags, and it might crash on load. Reported-by: Khedor Signed-off-by: Denys Fedoryshchenko --- accel-pppd/radius/dict.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel-pppd/radius/dict.c b/accel-pppd/radius/dict.c index 145c5da2..c181c7cd 100644 --- a/accel-pppd/radius/dict.c +++ b/accel-pppd/radius/dict.c @@ -92,7 +92,7 @@ static int dict_load(const char *fname) continue; r = split(buf, ptr); - if (*ptr[r - 1] == '#') + if (r > 0 && *ptr[r - 1] == '#') r--; if (!strcmp(buf, "VENDOR")) { -- cgit v1.2.3 From 4823556fbbadcd5205175d423ebca627dc317843 Mon Sep 17 00:00:00 2001 From: khedor Date: Fri, 22 May 2026 20:14:57 +0300 Subject: pppoe/disc: fix init_net allocation size In init_net(), the buffer for struct disc_net was sized as n = _malloc(sizeof(*net) + (HASH_BITS + 1) * sizeof(struct tree)); but 'net' is the const struct ap_net * argument, not the disc_net being allocated. sizeof(*net) is therefore sizeof(struct ap_net), which is substantially smaller than sizeof(struct disc_net). Every field of *n written past the ap_net-sized prefix (ctx, hnd, net, refs, etc.) lands in unallocated heap memory. Use sizeof(*n) so the allocation matches the actual destination type. Signed-off-by: khedor --- accel-pppd/ctrl/pppoe/disc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel-pppd/ctrl/pppoe/disc.c b/accel-pppd/ctrl/pppoe/disc.c index e673a97a..25d4be26 100644 --- a/accel-pppd/ctrl/pppoe/disc.c +++ b/accel-pppd/ctrl/pppoe/disc.c @@ -77,7 +77,7 @@ static struct disc_net *init_net(const struct ap_net *net) fcntl(sock, F_SETFD, FD_CLOEXEC); net->set_nonblocking(sock, 1); - n = _malloc(sizeof(*net) + (HASH_BITS + 1) * sizeof(struct tree)); + n = _malloc(sizeof(*n) + (HASH_BITS + 1) * sizeof(struct tree)); tree = n->tree; for (i = 0; i <= HASH_BITS; i++) { -- cgit v1.2.3 From a68821aecb2fecfc7c35fd5d49867f202d64df7f Mon Sep 17 00:00:00 2001 From: khedor Date: Fri, 22 May 2026 20:15:10 +0300 Subject: pppoe/disc: fix free_net memmove count and overlap handling free_net() compacts the nets[] array by sliding entries left after removing one: memcpy(nets + i, nets + i + 1, net_cnt - i - 1); Two bugs: 1. The count is a raw element count, not a byte count. nets[] holds 'struct disc_net *' pointers, so only (net_cnt - i - 1) bytes are moved instead of (net_cnt - i - 1) * sizeof(nets[0]). On the usual 8-byte-pointer build, 7 of every 8 surviving pointers are lost, leaving uninitialised holes in the array. 2. Source and destination overlap (nets + i and nets + i + 1), so memcpy is undefined behaviour. The correct primitive is memmove. Switch to memmove and multiply the count by sizeof(nets[0]). Signed-off-by: khedor --- accel-pppd/ctrl/pppoe/disc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel-pppd/ctrl/pppoe/disc.c b/accel-pppd/ctrl/pppoe/disc.c index 25d4be26..c302eaff 100644 --- a/accel-pppd/ctrl/pppoe/disc.c +++ b/accel-pppd/ctrl/pppoe/disc.c @@ -110,7 +110,7 @@ static void free_net(struct disc_net *net) pthread_mutex_lock(&nets_lock); for (i = 0; i < MAX_NET; i++) { if (nets[i] == net) { - memcpy(nets + i, nets + i + 1, net_cnt - i - 1); + memmove(nets + i, nets + i + 1, (net_cnt - i - 1) * sizeof(nets[0])); net_cnt--; break; } -- cgit v1.2.3 From 028e2090389d2b8968dfb30e4d956264718613e5 Mon Sep 17 00:00:00 2001 From: khedor Date: Fri, 22 May 2026 20:15:23 +0300 Subject: pppoe/disc: drop packets with unsupported PPPoE type The unsupported-PPPoE-type branch in disc_read() logs a warning but falls through to forward() instead of dropping the packet. Compare with the unsupported-version branch immediately above, which has the same shape but continues: if (hdr->ver != 1) { if (conf_verbose) log_warn(...unsupported version...); continue; } if (hdr->type != 1) { if (conf_verbose) log_warn(...unsupported type...); /* falls through into forward() */ } Add the missing continue so malformed packets are dropped instead of processed. Signed-off-by: khedor --- accel-pppd/ctrl/pppoe/disc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/accel-pppd/ctrl/pppoe/disc.c b/accel-pppd/ctrl/pppoe/disc.c index c302eaff..f7f5c781 100644 --- a/accel-pppd/ctrl/pppoe/disc.c +++ b/accel-pppd/ctrl/pppoe/disc.c @@ -363,6 +363,7 @@ static int disc_read(struct triton_md_handler_t *h) if (hdr->type != 1) { if (conf_verbose) log_warn("pppoe: discarding packet (unsupported type %i)\n", hdr->type); + continue; } if (forward(net, src.sll_ifindex, pack, n)) -- cgit v1.2.3 From f8dfbeca348d9f4eb794cdf3756a94f2d0acc730 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Fri, 29 May 2026 08:58:41 +0300 Subject: Rejecting any PADR lacking a Service-Name tag (RFC 2516 compliance) Signed-off-by: Denys Fedoryshchenko --- accel-pppd/ctrl/pppoe/pppoe.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c index 93824af5..d543a129 100644 --- a/accel-pppd/ctrl/pppoe/pppoe.c +++ b/accel-pppd/ctrl/pppoe/pppoe.c @@ -1234,6 +1234,12 @@ padr_tags_done: return; } + if (!service_name_tag) { + if (conf_verbose) + log_warn("pppoe: discard PADR packet (no Service-Name tag present)\n"); + return; + } + if (ntohs(ac_cookie_tag->tag_len) != COOKIE_LENGTH) { if (conf_verbose) log_warn("pppoe: discard PADR packet (incorrect AC-Cookie tag length)\n"); -- cgit v1.2.3