From 028b942d2daa57b56efd61cb5cb348017fad6f05 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:43:53 +0300 Subject: ppp: harden control packet decoding Bound LCP and CCP declared lengths by the received frame, reject truncated option headers across all control protocols, and decode LCP payload integers without alignment assumptions. IPCP and IPv6CP outer frame bounds remain owned by the delayed-ack change in PR #359. --- accel-pppd/ppp/ppp_ccp.c | 42 ++++++++++++++++++++++------ accel-pppd/ppp/ppp_ipcp.c | 40 ++++++++++++++++++++++----- accel-pppd/ppp/ppp_ipv6cp.c | 40 ++++++++++++++++++++++----- accel-pppd/ppp/ppp_lcp.c | 67 +++++++++++++++++++++++++++++++++++++-------- 4 files changed, 155 insertions(+), 34 deletions(-) diff --git a/accel-pppd/ppp/ppp_ccp.c b/accel-pppd/ppp/ppp_ccp.c index f9e05e89..4dda10f7 100644 --- a/accel-pppd/ppp/ppp_ccp.c +++ b/accel-pppd/ppp/ppp_ccp.c @@ -387,10 +387,13 @@ static int ccp_recv_conf_req(struct ppp_ccp_t *ccp, uint8_t *data, int size) ccp->ropt_len = size; while (size > 0) { + if (size < sizeof(*hdr)) + return CCP_OPT_FAIL; + hdr = (struct ccp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) - break; + if (hdr->len < sizeof(*hdr) || hdr->len > size) + return CCP_OPT_FAIL; ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); @@ -482,10 +485,17 @@ static int ccp_recv_conf_rej(struct ppp_ccp_t *ccp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ccp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ccp->options, entry) { if (lopt->id == hdr->id) { @@ -523,10 +533,17 @@ static int ccp_recv_conf_nak(struct ppp_ccp_t *ccp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ccp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ccp->options, entry) { if (lopt->id == hdr->id) { @@ -566,10 +583,17 @@ static int ccp_recv_conf_ack(struct ppp_ccp_t *ccp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ccp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ccp->options, entry) { if (lopt->id == hdr->id) { @@ -647,7 +671,7 @@ static void ccp_recv(struct ppp_handler_t*h) } hdr = (struct ccp_hdr_t *)ccp->ppp->buf; - if (ntohs(hdr->len) < PPP_HEADERLEN) { + if (ntohs(hdr->len) < PPP_HEADERLEN || ntohs(hdr->len) > ccp->ppp->buf_size - 2) { log_ppp_warn("CCP: short packet received\n"); return; } @@ -695,8 +719,10 @@ static void ccp_recv(struct ppp_handler_t*h) ppp_fsm_recv_conf_ack(&ccp->fsm); break; case CONFNAK: - ccp_recv_conf_nak(ccp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN); - ppp_fsm_recv_conf_rej(&ccp->fsm); + if (ccp_recv_conf_nak(ccp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) + ap_session_terminate(&ccp->ppp->ses, TERM_USER_ERROR, 0); + else + ppp_fsm_recv_conf_rej(&ccp->fsm); break; case CONFREJ: if (ccp_recv_conf_rej(ccp, (uint8_t*)(hdr + 1),ntohs(hdr->len) - PPP_HDRLEN)) diff --git a/accel-pppd/ppp/ppp_ipcp.c b/accel-pppd/ppp/ppp_ipcp.c index 416fba93..b67bfa44 100644 --- a/accel-pppd/ppp/ppp_ipcp.c +++ b/accel-pppd/ppp/ppp_ipcp.c @@ -391,10 +391,13 @@ static int ipcp_recv_conf_req(struct ppp_ipcp_t *ipcp, uint8_t *data, int size) ipcp->ropt_len = size; while (size > 0) { + if (size < sizeof(*hdr)) + return IPCP_OPT_FAIL; + hdr = (struct ipcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) - break; + if (hdr->len < sizeof(*hdr) || hdr->len > size) + return IPCP_OPT_FAIL; ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); @@ -503,10 +506,17 @@ static int ipcp_recv_conf_rej(struct ppp_ipcp_t *ipcp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipcp->options, entry) { if (lopt->id == hdr->id) { @@ -544,10 +554,17 @@ static int ipcp_recv_conf_nak(struct ppp_ipcp_t *ipcp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipcp->options, entry) { if (lopt->id == hdr->id) { @@ -587,10 +604,17 @@ static int ipcp_recv_conf_ack(struct ppp_ipcp_t *ipcp, uint8_t *data, int size) }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipcp->options, entry) { if (lopt->id == hdr->id) { @@ -725,8 +749,10 @@ static void ipcp_recv(struct ppp_handler_t*h) ppp_fsm_recv_conf_ack(&ipcp->fsm); break; case CONFNAK: - ipcp_recv_conf_nak(ipcp,(uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN); - ppp_fsm_recv_conf_rej(&ipcp->fsm); + if (ipcp_recv_conf_nak(ipcp,(uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) + ap_session_terminate(&ipcp->ppp->ses, TERM_USER_ERROR, 0); + else + ppp_fsm_recv_conf_rej(&ipcp->fsm); break; case CONFREJ: if (ipcp_recv_conf_rej(ipcp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) diff --git a/accel-pppd/ppp/ppp_ipv6cp.c b/accel-pppd/ppp/ppp_ipv6cp.c index 7f278daa..755e8903 100644 --- a/accel-pppd/ppp/ppp_ipv6cp.c +++ b/accel-pppd/ppp/ppp_ipv6cp.c @@ -395,10 +395,13 @@ static int ipv6cp_recv_conf_req(struct ppp_ipv6cp_t *ipv6cp, uint8_t *data, int ipv6cp->ropt_len = size; while (size > 0) { + if (size < sizeof(*hdr)) + return IPV6CP_OPT_FAIL; + hdr = (struct ipv6cp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) - break; + if (hdr->len < sizeof(*hdr) || hdr->len > size) + return IPV6CP_OPT_FAIL; ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); @@ -507,10 +510,17 @@ static int ipv6cp_recv_conf_rej(struct ppp_ipv6cp_t *ipv6cp, uint8_t *data, int }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipv6cp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipv6cp->options, entry) { if (lopt->id == hdr->id) { @@ -548,10 +558,17 @@ static int ipv6cp_recv_conf_nak(struct ppp_ipv6cp_t *ipv6cp, uint8_t *data, int }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipv6cp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipv6cp->options, entry) { if (lopt->id == hdr->id) { @@ -591,10 +608,17 @@ static int ipv6cp_recv_conf_ack(struct ppp_ipv6cp_t *ipv6cp, uint8_t *data, int }*/ while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct ipv6cp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &ipv6cp->options, entry) { if (lopt->id == hdr->id) { @@ -729,8 +753,10 @@ static void ipv6cp_recv(struct ppp_handler_t*h) ppp_fsm_recv_conf_ack(&ipv6cp->fsm); break; case CONFNAK: - ipv6cp_recv_conf_nak(ipv6cp,(uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN); - ppp_fsm_recv_conf_rej(&ipv6cp->fsm); + if (ipv6cp_recv_conf_nak(ipv6cp,(uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) + ap_session_terminate(&ipv6cp->ppp->ses, TERM_USER_ERROR, 0); + else + ppp_fsm_recv_conf_rej(&ipv6cp->fsm); break; case CONFREJ: if (ipv6cp_recv_conf_rej(ipv6cp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) diff --git a/accel-pppd/ppp/ppp_lcp.c b/accel-pppd/ppp/ppp_lcp.c index ed085b3b..fb0bb8bb 100644 --- a/accel-pppd/ppp/ppp_lcp.c +++ b/accel-pppd/ppp/ppp_lcp.c @@ -47,6 +47,22 @@ static void send_term_req(struct ppp_fsm_t *fsm); static void send_term_ack(struct ppp_fsm_t *fsm); static void lcp_recv(struct ppp_handler_t*); +static uint16_t lcp_read_u16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohs(value); +} + +static uint32_t lcp_read_u32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohl(value); +} + static void lcp_options_init(struct ppp_lcp_t *lcp) { struct lcp_option_t *lopt; @@ -370,10 +386,13 @@ static int lcp_recv_conf_req(struct ppp_lcp_t *lcp, uint8_t *data, int size) lcp->ropt_len = size; while (size > 0) { + if (size < sizeof(*hdr)) + return LCP_OPT_FAIL; + hdr = (struct lcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) - break; + if (hdr->len < sizeof(*hdr) || hdr->len > size) + return LCP_OPT_FAIL; ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); @@ -462,10 +481,17 @@ static int lcp_recv_conf_rej(struct ppp_lcp_t *lcp, uint8_t *data, int size) } while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct lcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &lcp->options, entry) { if (lopt->id == hdr->id) { @@ -507,10 +533,17 @@ static int lcp_recv_conf_nak(struct ppp_lcp_t *lcp, uint8_t *data, int size) } while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct lcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt,&lcp->options,entry) { if (lopt->id == hdr->id) { @@ -550,10 +583,17 @@ static int lcp_recv_conf_ack(struct ppp_lcp_t *lcp, uint8_t *data, int size) } while (size > 0) { + if (size < sizeof(*hdr)) { + res = -1; + break; + } + hdr = (struct lcp_opt_hdr_t *)data; - if (!hdr->len || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + res = -1; break; + } list_for_each_entry(lopt, &lcp->options, entry) { if (lopt->id == hdr->id) { @@ -587,7 +627,7 @@ static void lcp_recv_echo_repl(struct ppp_lcp_t *lcp, uint8_t *data, int size) if (conf_ppp_verbose) log_ppp_debug("recv [LCP EchoRep id=%x]\n", lcp->fsm.recv_id); } else { - magic = ntohl(*(uint32_t *)data); + magic = lcp_read_u32(data); if (conf_ppp_verbose) log_ppp_debug("recv [LCP EchoRep id=%x ]\n", lcp->fsm.recv_id, magic); @@ -746,7 +786,7 @@ static void lcp_recv(struct ppp_handler_t*h) hdr = (struct lcp_hdr_t *)lcp->ppp->buf; len = ntohs(hdr->len); buf_len = lcp->ppp->buf_size; - if (len < PPP_HEADERLEN) { + if (len < PPP_HEADERLEN || len > lcp->ppp->buf_size - 2) { log_ppp_warn("LCP: short packet received\n"); return; } @@ -802,7 +842,10 @@ static void lcp_recv(struct ppp_handler_t*h) } break; case CONFNAK: - lcp_recv_conf_nak(lcp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN); + if (lcp_recv_conf_nak(lcp, (uint8_t*)(hdr + 1), ntohs(hdr->len) - PPP_HDRLEN)) { + ap_session_terminate(&lcp->ppp->ses, TERM_USER_ERROR, 0); + break; + } if (lcp->fsm.recv_id != lcp->fsm.id) break; ppp_fsm_recv_conf_rej(&lcp->fsm); @@ -838,7 +881,7 @@ static void lcp_recv(struct ppp_handler_t*h) break; } if (conf_ppp_verbose) - log_ppp_debug("recv [LCP EchoReq id=%x ]\n", hdr->id, ntohl(*(uint32_t*)(hdr + 1))); + log_ppp_debug("recv [LCP EchoReq id=%x ]\n", hdr->id, lcp_read_u32(hdr + 1)); send_echo_reply(lcp); break; case ECHOREP: @@ -854,11 +897,11 @@ static void lcp_recv(struct ppp_handler_t*h) log_ppp_warn("LCP: short ProtoRej received\n"); break; } - log_ppp_info2("recv [LCP ProtoRej id=%x <%04x>]\n", hdr->id, ntohs(*(uint16_t*)(hdr + 1))); + log_ppp_info2("recv [LCP ProtoRej id=%x <%04x>]\n", hdr->id, lcp_read_u16(hdr + 1)); } if (len < PPP_HDRLEN + 2 || buf_len < (int)(sizeof(*hdr) + 2)) break; - ppp_recv_proto_rej(lcp->ppp, ntohs(*(uint16_t *)(hdr + 1))); + ppp_recv_proto_rej(lcp->ppp, lcp_read_u16(hdr + 1)); break; case DISCARDREQ: if (conf_ppp_verbose) { @@ -866,7 +909,7 @@ static void lcp_recv(struct ppp_handler_t*h) log_ppp_warn("LCP: short DiscardReq received\n"); break; } - log_ppp_info2("recv [LCP DiscardReq id=%x ]\n", hdr->id, ntohl(*(uint32_t*)(hdr + 1))); + log_ppp_info2("recv [LCP DiscardReq id=%x ]\n", hdr->id, lcp_read_u32(hdr + 1)); } break; case IDENT: -- cgit v1.2.3 From d70fc819a4f7f0b54b374bfdae062e3c7d2d3cc6 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:44:10 +0300 Subject: ppp: cover alignment-safe MPPE prefer setup Serialize the MPPE option through an aligned temporary and exercise mppe=prefer in the existing unauthenticated PPPoE session test. --- accel-pppd/ppp/ccp_mppe.c | 3 ++- tests/accel-pppd/pppoe/test_pppoe_session_wo_auth.py | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/accel-pppd/ppp/ccp_mppe.c b/accel-pppd/ppp/ccp_mppe.c index cb41c0da..c2ffd9a0 100644 --- a/accel-pppd/ppp/ccp_mppe.c +++ b/accel-pppd/ppp/ccp_mppe.c @@ -102,11 +102,12 @@ static int setup_mppe_key(int fd, int transmit, uint8_t *key) { struct ppp_option_data data; uint8_t buf[6 + 16]; + uint32_t bits = htonl(MPPE_S | MPPE_H); memset(buf, 0, sizeof(buf)); buf[0] = CI_MPPE; buf[1] = 6; - *(uint32_t*)(buf + 2) = htonl(MPPE_S | MPPE_H); + memcpy(buf + 2, &bits, sizeof(bits)); if (key) memcpy(buf + 6, key, 16); diff --git a/tests/accel-pppd/pppoe/test_pppoe_session_wo_auth.py b/tests/accel-pppd/pppoe/test_pppoe_session_wo_auth.py index 960b7c94..b4cccc81 100644 --- a/tests/accel-pppd/pppoe/test_pppoe_session_wo_auth.py +++ b/tests/accel-pppd/pppoe/test_pppoe_session_wo_auth.py @@ -26,6 +26,9 @@ def accel_pppd_config(veth_pair_netns): [auth] any-login=1 + [ppp] + mppe=prefer + [ip-pool] gw-ip-address=192.0.2.1 192.0.2.2-255 -- cgit v1.2.3 From 94993558dab2b79107c1d67c5cdb081604dada71 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:47:00 +0300 Subject: dhcpv6: validate option and relay boundaries Enforce fixed option prefixes centrally, bound nested relay messages to their declared payload, require one valid Relay-Message, and decode ORO and diagnostic fields without unaligned or oversized reads. --- accel-pppd/ipv6/dhcpv6.c | 18 ++++--- accel-pppd/ipv6/dhcpv6_packet.c | 111 ++++++++++++++++++++++++---------------- 2 files changed, 79 insertions(+), 50 deletions(-) diff --git a/accel-pppd/ipv6/dhcpv6.c b/accel-pppd/ipv6/dhcpv6.c index 4a80bdb9..1d565a76 100644 --- a/accel-pppd/ipv6/dhcpv6.c +++ b/accel-pppd/ipv6/dhcpv6.c @@ -222,25 +222,29 @@ static void insert_oro(struct dhcpv6_packet *reply, struct dhcpv6_option *opt) { struct dhcpv6_option *opt1; int i, j, dns_count; - uint16_t *ptr; - struct in6_addr addr, *addr_ptr; + uint8_t *ptr; + uint16_t code; + struct in6_addr addr; + uint8_t *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) { + for (i = ntohs(opt->hdr->len) / 2, ptr = opt->hdr->data; i; i--, ptr += sizeof(code)) { + memcpy(&code, ptr, sizeof(code)); + code = ntohs(code); + if (code == D6_OPTION_DNS_SERVERS) { 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++) + for (j = 0, addr_ptr = opt1->hdr->data; j < dns_count; j++, addr_ptr += sizeof(addr)) memcpy(addr_ptr, dns + j, sizeof(addr)); } - } else if (ntohs(*ptr) == D6_OPTION_DOMAIN_LIST) { + } else if (code == D6_OPTION_DOMAIN_LIST) { if (conf_dnssl_size) { opt1 = dhcpv6_option_alloc(reply, D6_OPTION_DOMAIN_LIST, conf_dnssl_size); memcpy(opt1->hdr->data, conf_dnssl, conf_dnssl_size); } - } else if (ntohs(*ptr) == D6_OPTION_AFTR_NAME) { + } else if (code == D6_OPTION_AFTR_NAME) { if (conf_aftr_gw_size) { opt1 = dhcpv6_option_alloc(reply, D6_OPTION_AFTR_NAME, conf_aftr_gw_size); memcpy(opt1->hdr->data, conf_aftr_gw, conf_aftr_gw_size); diff --git a/accel-pppd/ipv6/dhcpv6_packet.c b/accel-pppd/ipv6/dhcpv6_packet.c index c0c7bc81..1bf297af 100644 --- a/accel-pppd/ipv6/dhcpv6_packet.c +++ b/accel-pppd/ipv6/dhcpv6_packet.c @@ -13,7 +13,8 @@ struct dict_option { int code; const char *name; int recv; - int len; + int min_len; + int nested; void (*print)(struct dhcpv6_option *, void (*)(const char *fmt, ...)); }; @@ -33,31 +34,39 @@ static void print_dnssl(struct dhcpv6_option *opt, void (*print)(const char *fmt static void print_ia_prefix(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)); static void print_aftr_gw(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)); +static uint16_t dhcpv6_read_u16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohs(value); +} + static struct dict_option known_options[] = { - { D6_OPTION_CLIENTID, "Client-ID", 1, 0, print_clientid }, - { D6_OPTION_SERVERID, "Server-ID", 0, 0, print_clientid }, - { D6_OPTION_IA_NA, "IA-NA", 1, sizeof(struct dhcpv6_opt_ia_na), print_ia_na }, - { D6_OPTION_IA_TA, "IA-TA", 1, sizeof(struct dhcpv6_opt_ia_ta), print_ia_ta }, - { D6_OPTION_IAADDR, "IA-Addr", 1, sizeof(struct dhcpv6_opt_ia_addr), print_ia_addr }, - { D6_OPTION_ORO, "Option-Request", 1, 0, print_oro }, - { D6_OPTION_PREFERENCE, "Preference", 0, 0, print_uint8 }, - { D6_OPTION_ELAPSED_TIME, "Elapsed-Time", 1, 0, print_time }, - { D6_OPTION_RELAY_MSG, "Relay-Message", 1, 0 }, - { D6_OPTION_AUTH, "Auth", 1, 0 }, - { D6_OPTION_PREFERENCE, "Server-Unicast", 0, 0, print_ipv6addr }, - { D6_OPTION_STATUS_CODE, "Status", 0, 0, print_status }, - { D6_OPTION_RAPID_COMMIT, "Rapid-Commit", 1, 0 }, - { D6_OPTION_USER_CLASS, "User-Class", 1, 0 }, - { D6_OPTION_VENDOR_CLASS, "Vendor-Class", 1, 0, print_hex_array }, - { D6_OPTION_VENDOR_SPECIFIC, "Vendor-Specific", 1, 0, print_hex_array }, - { D6_OPTION_INTERFACE_ID, "Interface-ID", 1, 0, print_hex_array }, - { D6_OPTION_RECONF_MSG, "Reconfigure", 0, 0, print_reconf }, - { D6_OPTION_RECONF_ACCEPT, "Reconfigure-Accept", 1, 0 }, - { D6_OPTION_DNS_SERVERS, "DNS", 1, 0, print_ipv6addr_array }, - { D6_OPTION_DOMAIN_LIST, "DNSSL", 1, 0, print_dnssl }, - { D6_OPTION_IA_PD, "IA-PD", 1, sizeof(struct dhcpv6_opt_ia_na), print_ia_na }, - { D6_OPTION_IAPREFIX, "IA-Prefix", 1, sizeof(struct dhcpv6_opt_ia_prefix), print_ia_prefix }, - { D6_OPTION_AFTR_NAME, "AFTR-Name", 1, 0, print_aftr_gw }, + { D6_OPTION_CLIENTID, "Client-ID", 1, sizeof(uint16_t), 0, print_clientid }, + { D6_OPTION_SERVERID, "Server-ID", 0, sizeof(uint16_t), 0, print_clientid }, + { D6_OPTION_IA_NA, "IA-NA", 1, sizeof(struct dhcpv6_opt_ia_na) - sizeof(struct dhcpv6_opt_hdr), 1, print_ia_na }, + { D6_OPTION_IA_TA, "IA-TA", 1, sizeof(struct dhcpv6_opt_ia_ta) - sizeof(struct dhcpv6_opt_hdr), 1, print_ia_ta }, + { D6_OPTION_IAADDR, "IA-Addr", 1, sizeof(struct dhcpv6_opt_ia_addr) - sizeof(struct dhcpv6_opt_hdr), 1, print_ia_addr }, + { D6_OPTION_ORO, "Option-Request", 1, 0, 0, print_oro }, + { D6_OPTION_PREFERENCE, "Preference", 0, sizeof(uint8_t), 0, print_uint8 }, + { D6_OPTION_ELAPSED_TIME, "Elapsed-Time", 1, sizeof(uint16_t), 0, print_time }, + { D6_OPTION_RELAY_MSG, "Relay-Message", 1, sizeof(struct dhcpv6_msg_hdr), 0 }, + { D6_OPTION_AUTH, "Auth", 1, 0, 0 }, + { D6_OPTION_UNICAST, "Server-Unicast", 0, sizeof(struct in6_addr), 0, print_ipv6addr }, + { D6_OPTION_STATUS_CODE, "Status", 0, sizeof(uint16_t), 0, print_status }, + { D6_OPTION_RAPID_COMMIT, "Rapid-Commit", 1, 0, 0 }, + { D6_OPTION_USER_CLASS, "User-Class", 1, 0, 0 }, + { D6_OPTION_VENDOR_CLASS, "Vendor-Class", 1, 0, 0, print_hex_array }, + { D6_OPTION_VENDOR_SPECIFIC, "Vendor-Specific", 1, 0, 0, print_hex_array }, + { D6_OPTION_INTERFACE_ID, "Interface-ID", 1, 0, 0, print_hex_array }, + { D6_OPTION_RECONF_MSG, "Reconfigure", 0, sizeof(uint8_t), 0, print_reconf }, + { D6_OPTION_RECONF_ACCEPT, "Reconfigure-Accept", 1, 0, 0 }, + { D6_OPTION_DNS_SERVERS, "DNS", 1, 0, 0, print_ipv6addr_array }, + { D6_OPTION_DOMAIN_LIST, "DNSSL", 1, 0, 0, print_dnssl }, + { D6_OPTION_IA_PD, "IA-PD", 1, sizeof(struct dhcpv6_opt_ia_na) - sizeof(struct dhcpv6_opt_hdr), 1, print_ia_na }, + { D6_OPTION_IAPREFIX, "IA-Prefix", 1, sizeof(struct dhcpv6_opt_ia_prefix) - sizeof(struct dhcpv6_opt_hdr), 1, print_ia_prefix }, + { D6_OPTION_AFTR_NAME, "AFTR-Name", 1, 0, 0, print_aftr_gw }, { 0 } }; @@ -89,9 +98,14 @@ static void *parse_option(void *ptr, void *endptr, struct list_head *opt_list) break; } - if (dopt->len) { + if (ntohs(opth->len) < dopt->min_len) { + log_warn("dhcpv6: invalid packet received\n"); + return NULL; + } + + if (dopt->nested) { endptr = ptr + sizeof(*opth) + ntohs(opth->len); - ptr += dopt->len; + ptr += sizeof(*opth) + dopt->min_len; while (ptr < endptr) { ptr = parse_option(ptr, endptr, &opt->opt_list); if (!ptr) @@ -109,7 +123,8 @@ struct dhcpv6_packet *dhcpv6_packet_parse(const void *buf, size_t size) struct dhcpv6_opt_hdr *opth; struct dhcpv6_relay *rel; struct dhcpv6_relay_hdr *rhdr; - void *ptr, *endptr; + struct dhcpv6_msg_hdr *inner_hdr; + void *ptr, *endptr, *relay_end, *inner_end; if (size < sizeof(struct dhcpv6_msg_hdr)) { if (conf_verbose) @@ -133,8 +148,6 @@ struct dhcpv6_packet *dhcpv6_packet_parse(const void *buf, size_t size) endptr = ((void *)pkt->hdr) + size; while (pkt->hdr->type == D6_RELAY_FORW) { - struct dhcpv6_msg_hdr *prev_hdr = pkt->hdr; - rhdr = (struct dhcpv6_relay_hdr *)pkt->hdr; if (((void *)rhdr) + sizeof(*rhdr) > endptr) { log_warn("dhcpv6: invalid packet received\n"); @@ -153,27 +166,37 @@ struct dhcpv6_packet *dhcpv6_packet_parse(const void *buf, size_t size) list_add_tail(&rel->entry, &pkt->relay_list); + inner_hdr = NULL; + inner_end = NULL; + relay_end = endptr; ptr = rhdr->data; - while (ptr < endptr) { + while (ptr < relay_end) { opth = ptr; - if (ptr + sizeof(*opth) > endptr || - ptr + sizeof(*opth) + ntohs(opth->len) > endptr) { + if (ptr + sizeof(*opth) > relay_end || + ptr + sizeof(*opth) + ntohs(opth->len) > relay_end) { log_warn("dhcpv6: invalid packet received\n"); goto error; } if (opth->code == htons(D6_OPTION_RELAY_MSG)) { - pkt->hdr = (struct dhcpv6_msg_hdr *)opth->data; - endptr = opth->data + ntohs(opth->len); + if (inner_hdr || ntohs(opth->len) < sizeof(*inner_hdr)) { + log_warn("dhcpv6: invalid packet received\n"); + goto error; + } + inner_hdr = (struct dhcpv6_msg_hdr *)opth->data; + inner_end = opth->data + ntohs(opth->len); } ptr += sizeof(*opth) + ntohs(opth->len); } - if (pkt->hdr == prev_hdr) { + if (!inner_hdr) { log_warn("dhcpv6: invalid packet received\n"); goto error; } + + pkt->hdr = inner_hdr; + endptr = inner_end; } ptr = pkt->hdr->data; @@ -462,26 +485,28 @@ static void print_ia_addr(struct dhcpv6_option *opt, void (*print)(const char *f static void print_oro(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)) { - uint16_t *ptr = (uint16_t *)opt->hdr->data; - uint16_t *end_ptr = ptr + ntohs(opt->hdr->len)/2; + uint8_t *ptr = opt->hdr->data; + uint8_t *end_ptr = ptr + ntohs(opt->hdr->len) / 2 * sizeof(uint16_t); struct dict_option *dopt; + uint16_t code; int f = 0; - for (; ptr < end_ptr; ptr++) { + for (; ptr < end_ptr; ptr += sizeof(uint16_t)) { if (f) print(","); else print(" "); + code = dhcpv6_read_u16(ptr); for (dopt = known_options; dopt->code; dopt++) { - if (ntohs(*ptr) == dopt->code) + if (code == dopt->code) break; } if (dopt->code) print("%s", dopt->name); else - print("%i", ntohs(*ptr)); + print("%i", code); f = 1; } @@ -529,9 +554,9 @@ static void print_ipv6addr_array(struct dhcpv6_option *opt, void (*print)(const char str[INET6_ADDRSTRLEN]; int i; int f = 0; - struct in6_addr *addr = (struct in6_addr *)opt->hdr->data; + uint8_t *addr = opt->hdr->data; - for (i = ntohs(opt->hdr->len) / sizeof(*addr); i; i--, addr++) { + for (i = ntohs(opt->hdr->len) / sizeof(struct in6_addr); i; i--, addr += sizeof(struct in6_addr)) { inet_ntop(AF_INET6, addr, str, sizeof(str)); print("%c%s", f ? ',' : ' ', str); f = 1; -- cgit v1.2.3 From 3bbf61a373290d63382a4d1bfa8712566eb31c7b Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:49:55 +0300 Subject: ipoe: harden DHCPv4 option decoding Validate known options before extracting fields, bound Relay-Agent and classless-route subformats, and replace unaligned DHCP option accesses in notification, relay, and session paths. --- accel-pppd/ctrl/ipoe/dhcpv4.c | 26 +++++++++++------- accel-pppd/ctrl/ipoe/dhcpv4_options.c | 14 +++++++--- accel-pppd/ctrl/ipoe/ipoe.c | 50 ++++++++++++++++++++++++----------- 3 files changed, 61 insertions(+), 29 deletions(-) diff --git a/accel-pppd/ctrl/ipoe/dhcpv4.c b/accel-pppd/ctrl/ipoe/dhcpv4.c index d80a80f3..f1427e17 100644 --- a/accel-pppd/ctrl/ipoe/dhcpv4.c +++ b/accel-pppd/ctrl/ipoe/dhcpv4.c @@ -349,6 +349,12 @@ static int dhcpv4_parse_packet(struct dhcpv4_packet *pack, int len) list_add_tail(&opt->entry, &pack->options); + } + + if (dhcpv4_check_options(pack)) + return -1; + + list_for_each_entry(opt, &pack->options, entry) { if (opt->type == 53) pack->msg_type = opt->data[0]; else if (opt->type == 82) @@ -356,17 +362,14 @@ static int dhcpv4_parse_packet(struct dhcpv4_packet *pack, int len) else if (opt->type == 62) pack->client_id = opt; else if (opt->type == 50) - memcpy(&pack->request_ip, opt->data, 4); + memcpy(&pack->request_ip, opt->data, sizeof(pack->request_ip)); else if (opt->type == 54) - memcpy(&pack->server_id, opt->data, 4); + memcpy(&pack->server_id, opt->data, sizeof(pack->server_id)); } if (pack->msg_type == 0 || pack->msg_type > 8) return -1; - if (dhcpv4_check_options(pack)) - return -1; - pack->ptr = ptr; /*if (conf_verbose) { @@ -933,6 +936,7 @@ void dhcpv4_send_notify(struct dhcpv4_serv *serv, struct dhcpv4_packet *req, uns { struct dhcpv4_packet *pack = dhcpv4_packet_alloc(); uint8_t opt[8 + ETH_ALEN]; + uint32_t value; if (!pack) { log_emerg("out of memory\n"); @@ -946,8 +950,10 @@ void dhcpv4_send_notify(struct dhcpv4_serv *serv, struct dhcpv4_packet *req, uns pack->hdr->siaddr = 0; pack->hdr->giaddr = 0; - *(uint32_t *)opt = htonl(ACCEL_PPP_MAGIC); - *(uint32_t *)(opt + 4) = htonl(weight); + value = htonl(ACCEL_PPP_MAGIC); + memcpy(opt, &value, sizeof(value)); + value = htonl(weight); + memcpy(opt + sizeof(value), &value, sizeof(value)); memcpy(opt + 8, serv->hwaddr, ETH_ALEN); dhcpv4_packet_add_opt_u8(pack, 53, DHCPDISCOVER); @@ -1100,8 +1106,8 @@ int dhcpv4_relay_send(struct dhcpv4_relay *relay, struct dhcpv4_packet *request, if (server_id) { opt = dhcpv4_packet_find_opt(request, 54); if (opt) { - _server_id = *(uint32_t *)opt->data; - *(uint32_t *)opt->data = server_id; + memcpy(&_server_id, opt->data, sizeof(_server_id)); + memcpy(opt->data, &server_id, sizeof(server_id)); } } @@ -1123,7 +1129,7 @@ int dhcpv4_relay_send(struct dhcpv4_relay *relay, struct dhcpv4_packet *request, request->hdr->giaddr = giaddr; if (opt) - *(uint32_t *)opt->data = _server_id; + memcpy(opt->data, &_server_id, sizeof(_server_id)); if (n != len) return -1; diff --git a/accel-pppd/ctrl/ipoe/dhcpv4_options.c b/accel-pppd/ctrl/ipoe/dhcpv4_options.c index 042a7406..fe6c2a5f 100644 --- a/accel-pppd/ctrl/ipoe/dhcpv4_options.c +++ b/accel-pppd/ctrl/ipoe/dhcpv4_options.c @@ -205,7 +205,10 @@ static void print_message_type(const struct dhcpv4_option *opt, int elem_size, v { const char *msg_name[] = {"", "Discover", "Offer", "Request", "Decline", "Ack", "Nak", "Release", "Inform"}; - print("%s", msg_name[opt->data[0]]); + if (opt->data[0] < sizeof(msg_name) / sizeof(msg_name[0])) + print("%s", msg_name[opt->data[0]]); + else + print("%u", opt->data[0]); } static void print_request_list(const struct dhcpv4_option *opt, int elem_size, void (*print)(const char *fmt, ...)) @@ -232,14 +235,19 @@ static void print_relay_agent(const struct dhcpv4_option *opt, int elem_size, vo int type, len; while (ptr < endptr) { + if (endptr - ptr < 2) { + print("invalid"); + return; + } + if (ptr != opt->data) print(" "); type = *ptr++; len = *ptr++; - /*if (ptr + len > endptr) { + if (endptr - ptr < len) { print(" invalid"); return; - }*/ + } if (type == 1) print("{Agent-Circuit-ID "); else if (type == 2) diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c index 1e3f7054..e7b1320f 100644 --- a/accel-pppd/ctrl/ipoe/ipoe.c +++ b/accel-pppd/ctrl/ipoe/ipoe.c @@ -1835,6 +1835,7 @@ static int check_notify(struct ipoe_serv *serv, struct dhcpv4_packet *pack) struct dhcpv4_option *opt = dhcpv4_packet_find_opt(pack, 43); struct ipoe_session *ses; unsigned int w; + uint32_t value; if (!opt) return 0; @@ -1842,10 +1843,12 @@ static int check_notify(struct ipoe_serv *serv, struct dhcpv4_packet *pack) if (opt->len != 8 + ETH_ALEN) return 0; - if (*(uint32_t *)opt->data != htonl(ACCEL_PPP_MAGIC)) + memcpy(&value, opt->data, sizeof(value)); + if (value != htonl(ACCEL_PPP_MAGIC)) return 0; - w = htonl(*(uint32_t *)(opt->data + 4)); + memcpy(&value, opt->data + sizeof(value), sizeof(value)); + w = ntohl(value); list_for_each_entry(ses, &serv->sessions, entry) { if (ses->xid == pack->hdr->xid && memcmp(pack->hdr->chaddr, ses->hwaddr, ETH_ALEN) == 0) { @@ -2029,6 +2032,7 @@ static void ipoe_ses_recv_dhcpv4_relay(struct dhcpv4_packet *pack) { struct ipoe_session *ses = container_of(triton_context_self(), typeof(*ses), ctx); struct dhcpv4_option *opt; + uint32_t value; if (ses->dhcpv4_relay_reply) dhcpv4_packet_free(ses->dhcpv4_relay_reply); @@ -2047,24 +2051,32 @@ static void ipoe_ses_recv_dhcpv4_relay(struct dhcpv4_packet *pack) } opt = dhcpv4_packet_find_opt(pack, 51); - if (opt) - ses->lease_time = ntohl(*(uint32_t *)opt->data); + if (opt) { + memcpy(&value, opt->data, sizeof(value)); + ses->lease_time = ntohl(value); + } opt = dhcpv4_packet_find_opt(pack, 58); - if (opt) - ses->renew_time = ntohl(*(uint32_t *)opt->data); + if (opt) { + memcpy(&value, opt->data, sizeof(value)); + ses->renew_time = ntohl(value); + } opt = dhcpv4_packet_find_opt(pack, 59); - if (opt) - ses->rebind_time = ntohl(*(uint32_t *)opt->data); + if (opt) { + memcpy(&value, opt->data, sizeof(value)); + ses->rebind_time = ntohl(value); + } opt = dhcpv4_packet_find_opt(pack, 1); - if (opt) - ses->mask = parse_dhcpv4_mask(ntohl(*(uint32_t *)opt->data)); + if (opt) { + memcpy(&value, opt->data, sizeof(value)); + ses->mask = parse_dhcpv4_mask(ntohl(value)); + } opt = dhcpv4_packet_find_opt(pack, 3); if (opt) - ses->router = *(uint32_t *)opt->data; + memcpy(&ses->router, opt->data, sizeof(ses->router)); if (pack->msg_type == DHCPOFFER) { if (ses->ses.state == AP_STATE_STARTING) { @@ -2409,7 +2421,7 @@ static void ev_radius_access_accept(struct ev_radius_t *ev) ses->siaddr = attr->val.ipaddr; break; case DHCP_Router_Address: - ses->router = *(in_addr_t *)attr->raw; + memcpy(&ses->router, attr->raw, sizeof(ses->router)); break; case DHCP_Subnet_Mask: ses->mask = ipaddr_to_prefix(attr->val.ipaddr); @@ -2977,32 +2989,38 @@ static void ipoe_serv_timeout(struct triton_timer_t *t) static void ipoe_ipv6_enable(struct ipoe_serv *serv) { struct ifreq ifr; + uint32_t addr; strcpy(ifr.ifr_name, serv->ifname); ifr.ifr_hwaddr.sa_family = AF_UNSPEC; ifr.ifr_hwaddr.sa_data[0] = 0x33; ifr.ifr_hwaddr.sa_data[1] = 0x33; - *(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x02); + addr = htonl(0x02); + memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr)); ioctl(sock_fd, SIOCADDMULTI, &ifr); - *(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x010002); + addr = htonl(0x010002); + memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr)); ioctl(sock_fd, SIOCADDMULTI, &ifr); } static void ipoe_ipv6_disable(struct ipoe_serv *serv) { struct ifreq ifr; + uint32_t addr; strcpy(ifr.ifr_name, serv->ifname); ifr.ifr_hwaddr.sa_family = AF_UNSPEC; ifr.ifr_hwaddr.sa_data[0] = 0x33; ifr.ifr_hwaddr.sa_data[1] = 0x33; - *(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x02); + addr = htonl(0x02); + memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr)); ioctl(sock_fd, SIOCDELMULTI, &ifr); - *(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x010002); + addr = htonl(0x010002); + memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr)); ioctl(sock_fd, SIOCDELMULTI, &ifr); } -- cgit v1.2.3 From 6c4593a87e689a83d7308efb0913a8dd9182051b Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:50:04 +0300 Subject: pppoe: decode tag integers alignment-safely Read peer-controlled PPPoE tag and TR-101 integer fields through aligned temporaries, validate PPP-Max-Payload before formatting, and avoid unaligned cookie timestamp accesses. --- accel-pppd/ctrl/pppoe/pppoe.c | 44 +++++++++++++++++++++++++++++++++++-------- accel-pppd/ctrl/pppoe/tr101.c | 36 +++++++++++++++++++++-------------- 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c index 0e65168d..00f962af 100644 --- a/accel-pppd/ctrl/pppoe/pppoe.c +++ b/accel-pppd/ctrl/pppoe/pppoe.c @@ -554,9 +554,30 @@ static void print_tag_octets(struct pppoe_tag *tag) log_info2("%02x", (uint8_t)tag->tag_data[i]); } +static uint16_t pppoe_read_u16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohs(value); +} + +static uint32_t pppoe_read_u32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohl(value); +} + static void print_tag_u16(struct pppoe_tag *tag) { - log_info2("%i", (uint16_t)ntohs(*(uint16_t *)tag->tag_data)); + if (ntohs(tag->tag_len) != sizeof(uint16_t)) { + log_info2("invalid"); + return; + } + + log_info2("%i", pppoe_read_u16(tag->tag_data)); } static void print_packet(const char *ifname, const char *op, uint8_t *pack) @@ -633,7 +654,7 @@ static void print_packet(const char *ifname, const char *op, uint8_t *pack) if (ntohs(tag->tag_len) < 4) log_info2(" "); else - log_info2(" ", ntohl(*(uint32_t *)tag->tag_data)); + log_info2(" ", pppoe_read_u32(tag->tag_data)); break; case TAG_RELAY_SESSION_ID: log_info2(" secret, SECRET_LENGTH); @@ -1073,7 +1101,7 @@ static void pppoe_recv_PADI(struct pppoe_serv_t *serv, uint8_t *pack, int size) break; case TAG_PPP_MAX_PAYLOAD: if (ntohs(tag->tag_len) == 2) - ppp_max_payload = ntohs(*(uint16_t *)tag->tag_data); + ppp_max_payload = pppoe_read_u16(tag->tag_data); break; } } @@ -1220,14 +1248,14 @@ static void pppoe_recv_PADR(struct pppoe_serv_t *serv, uint8_t *pack, int size) case TAG_VENDOR_SPECIFIC: if (ntohs(tag->tag_len) < 4) continue; - vendor_id = ntohl(*(uint32_t *)tag->tag_data); + vendor_id = pppoe_read_u32(tag->tag_data); if (vendor_id == VENDOR_ADSL_FORUM) if (conf_tr101) tr101_tag = tag; break; case TAG_PPP_MAX_PAYLOAD: if (ntohs(tag->tag_len) == 2) - ppp_max_payload = ntohs(*(uint16_t *)tag->tag_data); + ppp_max_payload = pppoe_read_u16(tag->tag_data); break; } } diff --git a/accel-pppd/ctrl/pppoe/tr101.c b/accel-pppd/ctrl/pppoe/tr101.c index bb8b845a..e7aa96dc 100644 --- a/accel-pppd/ctrl/pppoe/tr101.c +++ b/accel-pppd/ctrl/pppoe/tr101.c @@ -30,6 +30,14 @@ #define ACCESS_LOOP_ENCAP 0x90 #define IFW_SESSION 0xFE +static uint32_t tr101_read_u32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohl(value); +} + static int tr101_send_request(struct pppoe_tag *tr101, struct rad_packet_t *pack, int type) { uint8_t *ptr = (uint8_t *)tr101->tag_data + 4; @@ -75,85 +83,85 @@ static int tr101_send_request(struct pppoe_tag *tr101, struct rad_packet_t *pack case OPT_ACTUAL_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_ACTUAL_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Downstream", tr101_read_u32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream", tr101_read_u32(ptr))) return -1; break; case OPT_ATT_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_ATT_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Downstream", tr101_read_u32(ptr))) return -1; break; case OPT_MAX_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_MAX_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Downstream", tr101_read_u32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_UP_LP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream-Low-Power", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream-Low-Power", tr101_read_u32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_DOWN_LP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream-Low-Power", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream-Low-Power", tr101_read_u32(ptr))) return -1; break; case OPT_MAX_INTERL_DELAY_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_ACTUAL_INTERL_DELAY_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Upstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Upstream", tr101_read_u32(ptr))) return -1; break; case OPT_MAX_INTER_DELAY_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Downstream", tr101_read_u32(ptr))) return -1; break; case OPT_ACTUAL_INTER_DELAY_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Downstream", ntohl(*(uint32_t *)ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Downstream", tr101_read_u32(ptr))) return -1; break; case ACCESS_LOOP_ENCAP: -- cgit v1.2.3 From e0c63e6259bb4a75468e5680fb2d10934c2d035b Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:50:12 +0300 Subject: radius: harden packet integer decoding Serialize integer fields via memcpy, restrict attribute parsing to the RADIUS header's declared packet length, reject undersized packet lengths, and reject truncated attribute headers. --- accel-pppd/radius/packet.c | 59 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/accel-pppd/radius/packet.c b/accel-pppd/radius/packet.c index c7e91349..782ecffe 100644 --- a/accel-pppd/radius/packet.c +++ b/accel-pppd/radius/packet.c @@ -35,6 +35,34 @@ static mempool_t packet_pool; static mempool_t attr_pool; static mempool_t buf_pool; +static uint16_t radius_read_u16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohs(value); +} + +static uint32_t radius_read_u32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohl(value); +} + +static void radius_write_u16(void *ptr, uint16_t value) +{ + value = htons(value); + memcpy(ptr, &value, sizeof(value)); +} + +static void radius_write_u32(void *ptr, uint32_t value) +{ + value = htonl(value); + memcpy(ptr, &value, sizeof(value)); +} + struct rad_packet_t *rad_packet_alloc(int code) { struct rad_packet_t *pack; @@ -111,20 +139,20 @@ int rad_packet_build(struct rad_packet_t *pack, uint8_t *RA) *ptr = pack->code; ptr++; *ptr = pack->id; ptr++; - *(uint16_t*)ptr = htons(pack->len); ptr+= 2; + radius_write_u16(ptr, pack->len); ptr += 2; memcpy(ptr, RA, 16); ptr+=16; list_for_each_entry(attr, &pack->attrs, entry) { if (attr->vendor) { *ptr = 26; ptr++; *ptr = attr->len + 2 + 6; ptr++; - *(uint32_t *)ptr = htonl(attr->vendor->id); ptr+=4; + radius_write_u32(ptr, attr->vendor->id); ptr += 4; } *ptr = attr->attr->id; ptr++; *ptr = attr->len + 2; ptr++; switch(attr->attr->type) { case ATTR_TYPE_INTEGER: - *(uint32_t*)ptr = htonl(attr->val.integer); + radius_write_u32(ptr, attr->val.integer); break; case ATTR_TYPE_OCTETS: case ATTR_TYPE_STRING: @@ -136,7 +164,7 @@ int rad_packet_build(struct rad_packet_t *pack, uint8_t *RA) memcpy(ptr, &attr->val, attr->len); break; case ATTR_TYPE_DATE: - *(uint32_t*)ptr = htonl(attr->val.date); + radius_write_u32(ptr, attr->val.date); break; case ATTR_TYPE_IPV6PREFIX: ptr[0] = 0; @@ -204,17 +232,22 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) pack->code = *ptr; ptr++; pack->id = *ptr; ptr++; - pack->len = ntohs(*(uint16_t*)ptr); ptr += 2; + pack->len = radius_read_u16(ptr); ptr += 2; - if (pack->len > n) { + if (pack->len < 20 || pack->len > n) { log_ppp_warn("radius:packet: short packet received %i, expected %i\n", pack->len, n); goto out_err; } ptr += 16; - n -= 20; + n = pack->len - 20; while (n>0) { + if (n < 2) { + log_ppp_warn("radius:packet: truncated attribute header received\n"); + goto out_err; + } + id = *ptr; ptr++; len = *ptr - 2; ptr++; if (len < 0) { @@ -230,7 +263,7 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) log_ppp_warn("radius:packet: vendor attribute too short (%i)\n", len); goto out_err; } - vendor_id = ntohl(*(uint32_t *)ptr); + vendor_id = radius_read_u32(ptr); vendor = rad_dict_find_vendor_id(vendor_id); if (vendor) { if (len < 4 + vendor->tag + vendor->len) { @@ -240,14 +273,14 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) ptr += 4; if (vendor->tag == 2) - id = (uint16_t)ntohs(*(uint16_t *)ptr); + id = radius_read_u16(ptr); else id = *ptr; ptr += vendor->tag; if (vendor->len == 2) - len = (uint16_t)ntohs(*(uint16_t *)ptr); + len = radius_read_u16(ptr); else len = *ptr; @@ -300,15 +333,15 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) break; } if (len == 4) - attr->val.integer = ntohl(*(uint32_t*)ptr); + attr->val.integer = radius_read_u32(ptr); else if (len == 2) - attr->val.integer = ntohs(*(uint16_t*)ptr); + attr->val.integer = radius_read_u16(ptr); else if (len == 1) attr->val.integer = *ptr; break; case ATTR_TYPE_DATE: if (len == 4) - attr->val.integer = ntohl(*(uint32_t*)ptr); + attr->val.integer = radius_read_u32(ptr); else log_ppp_warn("radius:packet: attribute %s has invalid length %i (must be 4)\n", da->name, len); break; -- cgit v1.2.3 From cbbae583e584f83957acd99440fc67343d46b800 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:50:26 +0300 Subject: backup: restore scalar fields alignment-safely Use memcpy for scalar backup headers and restored session, pool, and RADIUS values because variable-length tags do not guarantee native integer alignment. --- accel-pppd/backup/backup_file.c | 8 +++++--- accel-pppd/extra/ippool.c | 4 ++-- accel-pppd/radius/backup.c | 40 ++++++++++++++++++++++++++++++---------- accel-pppd/session_backup.c | 11 ++++++++--- 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/accel-pppd/backup/backup_file.c b/accel-pppd/backup/backup_file.c index 472694f0..eaaf87d2 100644 --- a/accel-pppd/backup/backup_file.c +++ b/accel-pppd/backup/backup_file.c @@ -96,7 +96,7 @@ static int fs_commit(struct backup_data *d) ptr = (uint8_t *)(tag + 1); *ptr = tag->id; ptr++; *ptr = tag->internal ? 1 : 0; ptr++; - *(uint16_t *)ptr = tag->size; + memcpy(ptr, &tag->size, sizeof(tag->size)); MD5_Update(&md5, tag + 1, 4 + tag->size); iov[i].iov_base = tag + 1; iov[i].iov_len = 4 + tag->size; @@ -278,14 +278,16 @@ static void restore_session(const char *fn, int internal) } if (!internal && ptr[1]) { - ptr += 4 + *(uint16_t *)(ptr + 2); + uint16_t tag_size; + memcpy(&tag_size, ptr + 2, sizeof(tag_size)); + ptr += 4 + tag_size; continue; } tag = fs_alloc_tag(d, 0); tag->id = *ptr; ptr++; tag->internal = (*ptr & 0x01) ? 1 : 0; ptr ++; - tag->size = *(uint16_t *)ptr; ptr += 2; + memcpy(&tag->size, ptr, sizeof(tag->size)); ptr += 2; tag->data = ptr; ptr += tag->size; list_add_tail(&tag->entry, &mod->tag_list); diff --git a/accel-pppd/extra/ippool.c b/accel-pppd/extra/ippool.c index e935bb4c..28aecd95 100644 --- a/accel-pppd/extra/ippool.c +++ b/accel-pppd/extra/ippool.c @@ -736,10 +736,10 @@ static int session_restore(struct ap_session *ses, struct backup_mod *m) list_for_each_entry(tag, &m->tag_list, entry) { switch (tag->id) { case SES_TAG_IPV4_ADDR: - addr = *(in_addr_t *)tag->data; + memcpy(&addr, tag->data, sizeof(addr)); break; case SES_TAG_IPV4_PEER_ADDR: - peer_addr = *(in_addr_t *)tag->data; + memcpy(&peer_addr, tag->data, sizeof(peer_addr)); break; } } diff --git a/accel-pppd/radius/backup.c b/accel-pppd/radius/backup.c index 6d4db636..73825336 100644 --- a/accel-pppd/radius/backup.c +++ b/accel-pppd/radius/backup.c @@ -22,6 +22,27 @@ #define RAD_TAG_ACCT_SERVER_ADDR 9 #define RAD_TAG_ACCT_SERVER_PORT 10 #define RAD_TAG_IDLE_TIMEOUT 11 + +static uint16_t backup_read_u16(const void *ptr) +{ + uint16_t value; + memcpy(&value, ptr, sizeof(value)); + return value; +} + +static uint32_t backup_read_u32(const void *ptr) +{ + uint32_t value; + memcpy(&value, ptr, sizeof(value)); + return value; +} + +static uint64_t backup_read_u64(const void *ptr) +{ + uint64_t value; + memcpy(&value, ptr, sizeof(value)); + return value; +} #define RAD_TAG_ACCT_USERNAME 12 @@ -94,10 +115,10 @@ static void restore_ipv4_addr(struct ap_session *ses) list_for_each_entry(tag, &m->tag_list, entry) { switch (tag->id) { case SES_TAG_IPV4_ADDR: - ses->ipv4->addr = *(in_addr_t *)tag->data; + ses->ipv4->addr = backup_read_u32(tag->data); break; case SES_TAG_IPV4_PEER_ADDR: - ses->ipv4->peer_addr = *(in_addr_t *)tag->data; + ses->ipv4->peer_addr = backup_read_u32(tag->data); break; } } @@ -121,16 +142,16 @@ void radius_restore_session(struct ap_session *ses, struct radius_pd_t *rpd) list_for_each_entry(tag, &m->tag_list, entry) { switch (tag->id) { case RAD_TAG_INTERIM_INTERVAL: - rpd->acct_interim_interval = *(uint32_t *)tag->data; + rpd->acct_interim_interval = backup_read_u32(tag->data); break; case RAD_TAG_INTERIM_JITTER: - rpd->acct_interim_jitter = *(uint32_t *)tag->data; + rpd->acct_interim_jitter = backup_read_u32(tag->data); break; case RAD_TAG_SESSION_TIMEOUT: - rpd->session_timeout.expire_tv.tv_sec = *(uint64_t *)tag->data - ses->start_time; + rpd->session_timeout.expire_tv.tv_sec = backup_read_u64(tag->data) - ses->start_time; break; case RAD_TAG_IDLE_TIMEOUT: - rpd->idle_timeout.period = (*(uint32_t *)tag->data) * 1000; + rpd->idle_timeout.period = backup_read_u32(tag->data) * 1000; break; case RAD_TAG_IPV4_ADDR: ses->ipv4 = &rpd->ipv4_addr; @@ -150,16 +171,16 @@ void radius_restore_session(struct ap_session *ses, struct radius_pd_t *rpd) rpd->attr_state_len = tag->size; break; case RAD_TAG_TERMINATION_ACTION: - rpd->termination_action = *(uint32_t *)tag->data; + rpd->termination_action = backup_read_u32(tag->data); break; case RAD_TAG_ACCT_USERNAME: rpd->acct_username = _strndup(tag->data, tag->size); break; case RAD_TAG_ACCT_SERVER_ADDR: - acct_addr = *(in_addr_t *)tag->data; + acct_addr = backup_read_u32(tag->data); break; case RAD_TAG_ACCT_SERVER_PORT: - acct_port = *(uint16_t *)tag->data; + acct_port = backup_read_u16(tag->data); break; } } @@ -182,4 +203,3 @@ static void init(void) } DEFINE_INIT(100, init); - diff --git a/accel-pppd/session_backup.c b/accel-pppd/session_backup.c index dea5b600..2873136f 100644 --- a/accel-pppd/session_backup.c +++ b/accel-pppd/session_backup.c @@ -59,6 +59,8 @@ static int session_save(struct ap_session *ses, struct backup_mod *m) static int session_restore(struct ap_session *ses, struct backup_mod *m) { struct backup_tag *t; + time_t start_time; + uint32_t ifindex; list_for_each_entry(t, &m->tag_list, entry) { switch(t->id) { @@ -79,11 +81,14 @@ static int session_restore(struct ap_session *ses, struct backup_mod *m) ses->ifname[t->size] = 0; break; case SES_TAG_START_TIME: - ses->start_time = *(time_t *)t->data; + memcpy(&start_time, t->data, sizeof(start_time)); + ses->start_time = start_time; break; case SES_TAG_IFINDEX: - if (ses->backup->internal) - ses->ifindex = *(uint32_t *)t->data; + if (ses->backup->internal) { + memcpy(&ifindex, t->data, sizeof(ifindex)); + ses->ifindex = ifindex; + } break; /*case PPP_TAG_FD: ses->fd = *(int *)t->data; -- cgit v1.2.3 From 004ec5adc7e5702c518583caef437d304932beb5 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:50:35 +0300 Subject: ipv6: avoid integer access through address bytes Copy interface IDs and prefix words between aligned temporaries and byte arrays instead of casting IPv6 address storage to uint64_t pointers. --- accel-pppd/extra/ipv6pool.c | 13 ++++++++++--- accel-pppd/ifcfg.c | 4 ++-- accel-pppd/ipdb.c | 11 ++++++++--- accel-pppd/ppp/ipv6cp_opt_intfid.c | 9 +++++---- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/accel-pppd/extra/ipv6pool.c b/accel-pppd/extra/ipv6pool.c index 9a763445..05b84fe8 100644 --- a/accel-pppd/extra/ipv6pool.c +++ b/accel-pppd/extra/ipv6pool.c @@ -293,11 +293,18 @@ static void add_prefix(struct ip6_pool *pool, const char *_val) /* end = start | hostmask(mask) (matches the original generator) */ memcpy(&end, &start, sizeof(end)); - if (mask > 64) - *(uint64_t *)(end.s6_addr + 8) = htobe64(be64toh(*(uint64_t *)(end.s6_addr + 8)) | ((1llu << (128 - mask)) - 1)); + if (mask > 64) { + uint64_t value; + memcpy(&value, end.s6_addr + 8, sizeof(value)); + value = htobe64(be64toh(value) | ((1llu << (128 - mask)) - 1)); + memcpy(end.s6_addr + 8, &value, sizeof(value)); + } else { + uint64_t value; memset(end.s6_addr + 8, 0xff, 8); - *(uint64_t *)end.s6_addr = htobe64(be64toh(*(uint64_t *)end.s6_addr) | ((1llu << (64 - mask)) - 1)); + memcpy(&value, end.s6_addr, sizeof(value)); + value = htobe64(be64toh(value) | ((1llu << (64 - mask)) - 1)); + memcpy(end.s6_addr, &value, sizeof(value)); } { diff --git a/accel-pppd/ifcfg.c b/accel-pppd/ifcfg.c index 4e4a381a..66f5cc6b 100644 --- a/accel-pppd/ifcfg.c +++ b/accel-pppd/ifcfg.c @@ -225,7 +225,7 @@ void __export ap_session_ifdown(struct ap_session *ses) if (ses->ctrl->ppp) { ifr6.ifr6_addr.s6_addr32[0] = htonl(0xfe800000); - *(uint64_t *)(ifr6.ifr6_addr.s6_addr + 8) = ses->ipv6->intf_id; + memcpy(ifr6.ifr6_addr.s6_addr + 8, &ses->ipv6->intf_id, sizeof(ses->ipv6->intf_id)); ifr6.ifr6_prefixlen = 64; net->sock6_ioctl(SIOCDIFADDR, &ifr6); } @@ -376,4 +376,4 @@ int __export ap_session_vrf(struct ap_session *ses, const char *vrf_name, int le return 0; } -#endif \ No newline at end of file +#endif diff --git a/accel-pppd/ipdb.c b/accel-pppd/ipdb.c index 8fc08063..264f67b3 100644 --- a/accel-pppd/ipdb.c +++ b/accel-pppd/ipdb.c @@ -75,18 +75,23 @@ void __export ipdb_put_ipv6_prefix(struct ap_session *ses, struct ipv6db_prefix_ void __export build_ip6_addr(struct ipv6db_addr_t *a, uint64_t intf_id, struct in6_addr *addr) { + uint64_t value; + memcpy(addr, &a->addr, sizeof(*addr)); if (a->prefix_len == 128) return; if (a->prefix_len <= 64) - *(uint64_t *)(addr->s6_addr + 8) = intf_id; - else + memcpy(addr->s6_addr + 8, &intf_id, sizeof(intf_id)); + else { /* prefix_len 65..127 means a shift of up to 63 bits: a plain * int literal 1 is undefined behavior for shifts >= 31, so the * host bits mask must be built from a 64-bit constant */ - *(uint64_t *)(addr->s6_addr + 8) |= intf_id & htobe64((UINT64_C(1) << (128 - a->prefix_len)) - 1); + memcpy(&value, addr->s6_addr + 8, sizeof(value)); + value |= intf_id & htobe64((UINT64_C(1) << (128 - a->prefix_len)) - 1); + memcpy(addr->s6_addr + 8, &value, sizeof(value)); + } } diff --git a/accel-pppd/ppp/ipv6cp_opt_intfid.c b/accel-pppd/ppp/ipv6cp_opt_intfid.c index cb33f024..5de88932 100644 --- a/accel-pppd/ppp/ipv6cp_opt_intfid.c +++ b/accel-pppd/ppp/ipv6cp_opt_intfid.c @@ -284,12 +284,14 @@ static void ipaddr_print(void (*print)(const char *fmt,...), struct ipv6cp_optio { struct ipaddr_option_t *ipaddr_opt = container_of(opt, typeof(*ipaddr_opt), opt); struct ipv6cp_opt64_t *opt64 = (struct ipv6cp_opt64_t *)ptr; - struct in6_addr a; + struct in6_addr a = {}; + uint64_t intf_id; if (ptr) - *(uint64_t *)(a.s6_addr + 8) = opt64->val; + intf_id = opt64->val; else - *(uint64_t *)(a.s6_addr + 8) = ipaddr_opt->ppp->ses.ipv6->intf_id; + intf_id = ipaddr_opt->ppp->ses.ipv6->intf_id; + memcpy(a.s6_addr + 8, &intf_id, sizeof(intf_id)); print("", ntohs(a.s6_addr16[4]), ntohs(a.s6_addr16[5]), ntohs(a.s6_addr16[6]), ntohs(a.s6_addr16[7])); } @@ -376,4 +378,3 @@ static void init() } DEFINE_INIT(5, init); - -- cgit v1.2.3 From c12e1242c0ab98255661f84be3eb37148c00cb38 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 08:50:43 +0300 Subject: memdebug: access tail canaries alignment-safely Allocation sizes are arbitrary, so store and load the trailing uint64_t canary with memcpy instead of dereferencing a potentially unaligned pointer. --- accel-pppd/memdebug.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/accel-pppd/memdebug.c b/accel-pppd/memdebug.c index 5353e572..b3488aaa 100644 --- a/accel-pppd/memdebug.c +++ b/accel-pppd/memdebug.c @@ -42,6 +42,19 @@ struct mem_t static LIST_HEAD(mem_list); static spinlock_t mem_list_lock; +static uint64_t get_tail_magic(const struct mem_t *mem) +{ + uint64_t magic; + + memcpy(&magic, mem->data + mem->size, sizeof(magic)); + return magic; +} + +static void set_tail_magic(struct mem_t *mem) +{ + memcpy(mem->data + mem->size, &mem->magic2, sizeof(mem->magic2)); +} + static struct mem_t *_md_malloc(size_t size, const char *fname, int line) { struct mem_t *mem = malloc(sizeof(*mem) + size + 8); @@ -54,7 +67,7 @@ static struct mem_t *_md_malloc(size_t size, const char *fname, int line) mem->size = size; mem->magic1 = MAGIC1; mem->magic2 = (uint64_t)random() * (uint64_t)random(); - *(uint64_t*)(mem->data + size) = mem->magic2; + set_tail_magic(mem); spin_lock(&mem_list_lock); list_add_tail(&mem->entry, &mem_list); @@ -84,7 +97,7 @@ void __export md_free(void *ptr, const char *fname, int line) abort(); } - if (mem->magic2 != *(uint64_t*)(mem->data + mem->size)) { + if (mem->magic2 != get_tail_magic(mem)) { printf("memory corruption:\nmalloc(%zu) at %s:%i\nfree at %s:%i\n", mem->size, mem->fname, mem->line, fname, line); abort(); @@ -113,7 +126,7 @@ void __export *md_realloc(void *ptr, size_t size, const char *fname, int line) abort(); } - if (mem->magic2 != *(uint64_t*)(mem->data + mem->size)) { + if (mem->magic2 != get_tail_magic(mem)) { printf("memory corruption:\nmalloc(%zu) at %s:%i\nfree at %s:%i\n", mem->size, mem->fname, mem->line, fname, line); abort(); @@ -217,7 +230,7 @@ static void siginfo2(int num) spin_lock(&mem_list_lock); list_for_each_entry(mem, &mem_list, entry) { - if (mem->magic1 != MAGIC1 || mem->magic2 != *(uint64_t*)(mem->data + mem->size)) + if (mem->magic1 != MAGIC1 || mem->magic2 != get_tail_magic(mem)) printf("%s:%i %lu\n", mem->fname, mem->line, (long unsigned)mem->size); } spin_unlock(&mem_list_lock); @@ -233,7 +246,7 @@ void __export md_check(void *ptr) if (mem->magic1 != MAGIC1) abort(); - if (mem->magic2 != *(uint64_t*)(mem->data + mem->size)) + if (mem->magic2 != get_tail_magic(mem)) abort(); } -- cgit v1.2.3 From 7d4f8524f57ba0ac77e47171bebfc0370df667b1 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 09:08:47 +0300 Subject: utils: centralize unaligned integer accessors --- accel-pppd/ctrl/l2tp/packet.c | 68 +++++------------------------------ accel-pppd/ctrl/l2tp/packet_test.c | 2 +- accel-pppd/ctrl/pppoe/pppoe.c | 26 +++----------- accel-pppd/ctrl/pppoe/tr101.c | 37 ++++++++----------- accel-pppd/ipv6/dhcpv6.c | 4 +-- accel-pppd/ipv6/dhcpv6_packet.c | 18 +++------- accel-pppd/ppp/ccp_mppe.c | 4 +-- accel-pppd/ppp/ppp_lcp.c | 26 +++----------- accel-pppd/radius/backup.c | 39 ++++++-------------- accel-pppd/radius/packet.c | 51 +++++++------------------- accel-pppd/utils.h | 73 ++++++++++++++++++++++++++++++++++++++ 11 files changed, 137 insertions(+), 211 deletions(-) diff --git a/accel-pppd/ctrl/l2tp/packet.c b/accel-pppd/ctrl/l2tp/packet.c index 0a4113a0..f134666d 100644 --- a/accel-pppd/ctrl/l2tp/packet.c +++ b/accel-pppd/ctrl/l2tp/packet.c @@ -111,58 +111,6 @@ 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) { size_t indx; @@ -223,7 +171,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 = unaligned_ntohs(p1); + orig_attr_len = u_read_be16(p1); if (orig_attr_len <= MD5_DIGEST_LENGTH - sizeof(uint16_t)) { /* Enough bytes decoded already, no need to decode padding */ @@ -271,7 +219,7 @@ out: 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); + orig_attr_len = u_read_be16(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" @@ -502,17 +450,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 = unaligned_ntohs(orig_avp_val); + attr->val.uint16 = u_read_be16(orig_avp_val); break; case ATTR_TYPE_INT32: if (orig_avp_len != sizeof(*avp) + 4) goto out_err_len; - attr->val.uint32 = unaligned_ntohl(orig_avp_val); + attr->val.uint32 = u_read_be32(orig_avp_val); break; case ATTR_TYPE_INT64: if (orig_avp_len != sizeof(*avp) + 8) goto out_err_len; - attr->val.uint64 = unaligned_be64toh(orig_avp_val); + attr->val.uint64 = u_read_be64(orig_avp_val); break; case ATTR_TYPE_OCTETS: attr->val.octets = _malloc(attr->length); @@ -589,13 +537,13 @@ int l2tp_packet_send(int sock, struct l2tp_packet_t *pack) else switch (attr->attr->type) { case ATTR_TYPE_INT16: - unaligned_htons(avp->val, attr->val.int16); + u_write_be16(avp->val, attr->val.int16); break; case ATTR_TYPE_INT32: - unaligned_htonl(avp->val, attr->val.int32); + u_write_be32(avp->val, attr->val.int32); break; case ATTR_TYPE_INT64: - unaligned_htobe64(avp->val, attr->val.uint64); + u_write_be64(avp->val, attr->val.uint64); break; case ATTR_TYPE_STRING: case ATTR_TYPE_OCTETS: diff --git a/accel-pppd/ctrl/l2tp/packet_test.c b/accel-pppd/ctrl/l2tp/packet_test.c index 9f962407..a6c9a182 100644 --- a/accel-pppd/ctrl/l2tp/packet_test.c +++ b/accel-pppd/ctrl/l2tp/packet_test.c @@ -4,7 +4,7 @@ * 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 \ + * -I accel-pppd -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 diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c index 00f962af..bd92cbf8 100644 --- a/accel-pppd/ctrl/pppoe/pppoe.c +++ b/accel-pppd/ctrl/pppoe/pppoe.c @@ -554,22 +554,6 @@ static void print_tag_octets(struct pppoe_tag *tag) log_info2("%02x", (uint8_t)tag->tag_data[i]); } -static uint16_t pppoe_read_u16(const void *ptr) -{ - uint16_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohs(value); -} - -static uint32_t pppoe_read_u32(const void *ptr) -{ - uint32_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohl(value); -} - static void print_tag_u16(struct pppoe_tag *tag) { if (ntohs(tag->tag_len) != sizeof(uint16_t)) { @@ -577,7 +561,7 @@ static void print_tag_u16(struct pppoe_tag *tag) return; } - log_info2("%i", pppoe_read_u16(tag->tag_data)); + log_info2("%i", u_read_be16(tag->tag_data)); } static void print_packet(const char *ifname, const char *op, uint8_t *pack) @@ -654,7 +638,7 @@ static void print_packet(const char *ifname, const char *op, uint8_t *pack) if (ntohs(tag->tag_len) < 4) log_info2(" "); else - log_info2(" ", pppoe_read_u32(tag->tag_data)); + log_info2(" ", u_read_be32(tag->tag_data)); break; case TAG_RELAY_SESSION_ID: log_info2(" tag_len) == 2) - ppp_max_payload = pppoe_read_u16(tag->tag_data); + ppp_max_payload = u_read_be16(tag->tag_data); break; } } @@ -1248,14 +1232,14 @@ static void pppoe_recv_PADR(struct pppoe_serv_t *serv, uint8_t *pack, int size) case TAG_VENDOR_SPECIFIC: if (ntohs(tag->tag_len) < 4) continue; - vendor_id = pppoe_read_u32(tag->tag_data); + vendor_id = u_read_be32(tag->tag_data); if (vendor_id == VENDOR_ADSL_FORUM) if (conf_tr101) tr101_tag = tag; break; case TAG_PPP_MAX_PAYLOAD: if (ntohs(tag->tag_len) == 2) - ppp_max_payload = pppoe_read_u16(tag->tag_data); + ppp_max_payload = u_read_be16(tag->tag_data); break; } } diff --git a/accel-pppd/ctrl/pppoe/tr101.c b/accel-pppd/ctrl/pppoe/tr101.c index e7aa96dc..06aeff86 100644 --- a/accel-pppd/ctrl/pppoe/tr101.c +++ b/accel-pppd/ctrl/pppoe/tr101.c @@ -8,6 +8,7 @@ #include "log.h" #include "radius.h" #include "memdebug.h" +#include "utils.h" #include "pppoe.h" @@ -30,14 +31,6 @@ #define ACCESS_LOOP_ENCAP 0x90 #define IFW_SESSION 0xFE -static uint32_t tr101_read_u32(const void *ptr) -{ - uint32_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohl(value); -} - static int tr101_send_request(struct pppoe_tag *tr101, struct rad_packet_t *pack, int type) { uint8_t *ptr = (uint8_t *)tr101->tag_data + 4; @@ -83,85 +76,85 @@ static int tr101_send_request(struct pppoe_tag *tr101, struct rad_packet_t *pack case OPT_ACTUAL_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Upstream", u_read_be32(ptr))) return -1; break; case OPT_ACTUAL_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Data-Rate-Downstream", u_read_be32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream", u_read_be32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream", u_read_be32(ptr))) return -1; break; case OPT_ATT_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Upstream", u_read_be32(ptr))) return -1; break; case OPT_ATT_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Attainable-Data-Rate-Downstream", u_read_be32(ptr))) return -1; break; case OPT_MAX_DATA_RATE_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Upstream", u_read_be32(ptr))) return -1; break; case OPT_MAX_DATA_RATE_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Data-Rate-Downstream", u_read_be32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_UP_LP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream-Low-Power", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Upstream-Low-Power", u_read_be32(ptr))) return -1; break; case OPT_MIN_DATA_RATE_DOWN_LP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream-Low-Power", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Minimum-Data-Rate-Downstream-Low-Power", u_read_be32(ptr))) return -1; break; case OPT_MAX_INTERL_DELAY_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Upstream", u_read_be32(ptr))) return -1; break; case OPT_ACTUAL_INTERL_DELAY_UP: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Upstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Upstream", u_read_be32(ptr))) return -1; break; case OPT_MAX_INTER_DELAY_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Maximum-Interleaving-Delay-Downstream", u_read_be32(ptr))) return -1; break; case OPT_ACTUAL_INTER_DELAY_DOWN: if (len != 4) goto inval; - if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Downstream", tr101_read_u32(ptr))) + if (rad_packet_add_int(pack, "ADSL-Forum", "Actual-Interleaving-Delay-Downstream", u_read_be32(ptr))) return -1; break; case ACCESS_LOOP_ENCAP: diff --git a/accel-pppd/ipv6/dhcpv6.c b/accel-pppd/ipv6/dhcpv6.c index 1d565a76..a0b159cf 100644 --- a/accel-pppd/ipv6/dhcpv6.c +++ b/accel-pppd/ipv6/dhcpv6.c @@ -23,6 +23,7 @@ #include "ipv6_dns.h" #include "events.h" #include "iputils.h" +#include "utils.h" #include "dhcpv6.h" @@ -229,8 +230,7 @@ static void insert_oro(struct dhcpv6_packet *reply, struct dhcpv6_option *opt) struct in6_addr dns[MAX_DNS_COUNT]; for (i = ntohs(opt->hdr->len) / 2, ptr = opt->hdr->data; i; i--, ptr += sizeof(code)) { - memcpy(&code, ptr, sizeof(code)); - code = ntohs(code); + code = u_read_be16(ptr); if (code == D6_OPTION_DNS_SERVERS) { dns_count = ipv6_dns_get(reply->ses, conf_dns, conf_dns_count, dns, MAX_DNS_COUNT); diff --git a/accel-pppd/ipv6/dhcpv6_packet.c b/accel-pppd/ipv6/dhcpv6_packet.c index 1bf297af..f1c168b6 100644 --- a/accel-pppd/ipv6/dhcpv6_packet.c +++ b/accel-pppd/ipv6/dhcpv6_packet.c @@ -4,6 +4,7 @@ #include "log.h" #include "memdebug.h" +#include "utils.h" #include "dhcpv6.h" @@ -34,14 +35,6 @@ static void print_dnssl(struct dhcpv6_option *opt, void (*print)(const char *fmt static void print_ia_prefix(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)); static void print_aftr_gw(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)); -static uint16_t dhcpv6_read_u16(const void *ptr) -{ - uint16_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohs(value); -} - static struct dict_option known_options[] = { { D6_OPTION_CLIENTID, "Client-ID", 1, sizeof(uint16_t), 0, print_clientid }, { D6_OPTION_SERVERID, "Server-ID", 0, sizeof(uint16_t), 0, print_clientid }, @@ -497,7 +490,7 @@ static void print_oro(struct dhcpv6_option *opt, void (*print)(const char *fmt, else print(" "); - code = dhcpv6_read_u16(ptr); + code = u_read_be16(ptr); for (dopt = known_options; dopt->code; dopt++) { if (code == dopt->code) break; @@ -531,13 +524,10 @@ static void print_uint8(struct dhcpv6_option *opt, void (*print)(const char *fmt static void print_time(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)) { - uint16_t val; - - if (ntohs(opt->hdr->len) < sizeof(val)) + if (ntohs(opt->hdr->len) < sizeof(uint16_t)) return; - memcpy(&val, opt->hdr->data, sizeof(val)); - print(" %u", ntohs(val)); + print(" %u", u_read_be16(opt->hdr->data)); } static void print_ipv6addr(struct dhcpv6_option *opt, void (*print)(const char *fmt, ...)) diff --git a/accel-pppd/ppp/ccp_mppe.c b/accel-pppd/ppp/ccp_mppe.c index c2ffd9a0..5042c8a0 100644 --- a/accel-pppd/ppp/ccp_mppe.c +++ b/accel-pppd/ppp/ccp_mppe.c @@ -10,6 +10,7 @@ #include "ppp_ccp.h" #include "log.h" #include "events.h" +#include "utils.h" #include "memdebug.h" @@ -102,12 +103,11 @@ static int setup_mppe_key(int fd, int transmit, uint8_t *key) { struct ppp_option_data data; uint8_t buf[6 + 16]; - uint32_t bits = htonl(MPPE_S | MPPE_H); memset(buf, 0, sizeof(buf)); buf[0] = CI_MPPE; buf[1] = 6; - memcpy(buf + 2, &bits, sizeof(bits)); + u_write_be32(buf + 2, MPPE_S | MPPE_H); if (key) memcpy(buf + 6, key, 16); diff --git a/accel-pppd/ppp/ppp_lcp.c b/accel-pppd/ppp/ppp_lcp.c index fb0bb8bb..b44674f4 100644 --- a/accel-pppd/ppp/ppp_lcp.c +++ b/accel-pppd/ppp/ppp_lcp.c @@ -47,22 +47,6 @@ static void send_term_req(struct ppp_fsm_t *fsm); static void send_term_ack(struct ppp_fsm_t *fsm); static void lcp_recv(struct ppp_handler_t*); -static uint16_t lcp_read_u16(const void *ptr) -{ - uint16_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohs(value); -} - -static uint32_t lcp_read_u32(const void *ptr) -{ - uint32_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohl(value); -} - static void lcp_options_init(struct ppp_lcp_t *lcp) { struct lcp_option_t *lopt; @@ -627,7 +611,7 @@ static void lcp_recv_echo_repl(struct ppp_lcp_t *lcp, uint8_t *data, int size) if (conf_ppp_verbose) log_ppp_debug("recv [LCP EchoRep id=%x]\n", lcp->fsm.recv_id); } else { - magic = lcp_read_u32(data); + magic = u_read_be32(data); if (conf_ppp_verbose) log_ppp_debug("recv [LCP EchoRep id=%x ]\n", lcp->fsm.recv_id, magic); @@ -881,7 +865,7 @@ static void lcp_recv(struct ppp_handler_t*h) break; } if (conf_ppp_verbose) - log_ppp_debug("recv [LCP EchoReq id=%x ]\n", hdr->id, lcp_read_u32(hdr + 1)); + log_ppp_debug("recv [LCP EchoReq id=%x ]\n", hdr->id, u_read_be32(hdr + 1)); send_echo_reply(lcp); break; case ECHOREP: @@ -897,11 +881,11 @@ static void lcp_recv(struct ppp_handler_t*h) log_ppp_warn("LCP: short ProtoRej received\n"); break; } - log_ppp_info2("recv [LCP ProtoRej id=%x <%04x>]\n", hdr->id, lcp_read_u16(hdr + 1)); + log_ppp_info2("recv [LCP ProtoRej id=%x <%04x>]\n", hdr->id, u_read_be16(hdr + 1)); } if (len < PPP_HDRLEN + 2 || buf_len < (int)(sizeof(*hdr) + 2)) break; - ppp_recv_proto_rej(lcp->ppp, lcp_read_u16(hdr + 1)); + ppp_recv_proto_rej(lcp->ppp, u_read_be16(hdr + 1)); break; case DISCARDREQ: if (conf_ppp_verbose) { @@ -909,7 +893,7 @@ static void lcp_recv(struct ppp_handler_t*h) log_ppp_warn("LCP: short DiscardReq received\n"); break; } - log_ppp_info2("recv [LCP DiscardReq id=%x ]\n", hdr->id, lcp_read_u32(hdr + 1)); + log_ppp_info2("recv [LCP DiscardReq id=%x ]\n", hdr->id, u_read_be32(hdr + 1)); } break; case IDENT: diff --git a/accel-pppd/radius/backup.c b/accel-pppd/radius/backup.c index 73825336..bfca0e49 100644 --- a/accel-pppd/radius/backup.c +++ b/accel-pppd/radius/backup.c @@ -6,6 +6,7 @@ #include "log.h" #include "memdebug.h" +#include "utils.h" #include "backup.h" #include "ap_session_backup.h" @@ -23,26 +24,6 @@ #define RAD_TAG_ACCT_SERVER_PORT 10 #define RAD_TAG_IDLE_TIMEOUT 11 -static uint16_t backup_read_u16(const void *ptr) -{ - uint16_t value; - memcpy(&value, ptr, sizeof(value)); - return value; -} - -static uint32_t backup_read_u32(const void *ptr) -{ - uint32_t value; - memcpy(&value, ptr, sizeof(value)); - return value; -} - -static uint64_t backup_read_u64(const void *ptr) -{ - uint64_t value; - memcpy(&value, ptr, sizeof(value)); - return value; -} #define RAD_TAG_ACCT_USERNAME 12 @@ -115,10 +96,10 @@ static void restore_ipv4_addr(struct ap_session *ses) list_for_each_entry(tag, &m->tag_list, entry) { switch (tag->id) { case SES_TAG_IPV4_ADDR: - ses->ipv4->addr = backup_read_u32(tag->data); + ses->ipv4->addr = u_read_native32(tag->data); break; case SES_TAG_IPV4_PEER_ADDR: - ses->ipv4->peer_addr = backup_read_u32(tag->data); + ses->ipv4->peer_addr = u_read_native32(tag->data); break; } } @@ -142,16 +123,16 @@ void radius_restore_session(struct ap_session *ses, struct radius_pd_t *rpd) list_for_each_entry(tag, &m->tag_list, entry) { switch (tag->id) { case RAD_TAG_INTERIM_INTERVAL: - rpd->acct_interim_interval = backup_read_u32(tag->data); + rpd->acct_interim_interval = u_read_native32(tag->data); break; case RAD_TAG_INTERIM_JITTER: - rpd->acct_interim_jitter = backup_read_u32(tag->data); + rpd->acct_interim_jitter = u_read_native32(tag->data); break; case RAD_TAG_SESSION_TIMEOUT: - rpd->session_timeout.expire_tv.tv_sec = backup_read_u64(tag->data) - ses->start_time; + rpd->session_timeout.expire_tv.tv_sec = u_read_native64(tag->data) - ses->start_time; break; case RAD_TAG_IDLE_TIMEOUT: - rpd->idle_timeout.period = backup_read_u32(tag->data) * 1000; + rpd->idle_timeout.period = u_read_native32(tag->data) * 1000; break; case RAD_TAG_IPV4_ADDR: ses->ipv4 = &rpd->ipv4_addr; @@ -171,16 +152,16 @@ void radius_restore_session(struct ap_session *ses, struct radius_pd_t *rpd) rpd->attr_state_len = tag->size; break; case RAD_TAG_TERMINATION_ACTION: - rpd->termination_action = backup_read_u32(tag->data); + rpd->termination_action = u_read_native32(tag->data); break; case RAD_TAG_ACCT_USERNAME: rpd->acct_username = _strndup(tag->data, tag->size); break; case RAD_TAG_ACCT_SERVER_ADDR: - acct_addr = backup_read_u32(tag->data); + acct_addr = u_read_native32(tag->data); break; case RAD_TAG_ACCT_SERVER_PORT: - acct_port = backup_read_u16(tag->data); + acct_port = u_read_native16(tag->data); break; } } diff --git a/accel-pppd/radius/packet.c b/accel-pppd/radius/packet.c index 782ecffe..4a0ab244 100644 --- a/accel-pppd/radius/packet.c +++ b/accel-pppd/radius/packet.c @@ -24,6 +24,7 @@ #include "mempool.h" #include "radius_p.h" #include "attr_defs.h" +#include "utils.h" #include "memdebug.h" @@ -35,34 +36,6 @@ static mempool_t packet_pool; static mempool_t attr_pool; static mempool_t buf_pool; -static uint16_t radius_read_u16(const void *ptr) -{ - uint16_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohs(value); -} - -static uint32_t radius_read_u32(const void *ptr) -{ - uint32_t value; - - memcpy(&value, ptr, sizeof(value)); - return ntohl(value); -} - -static void radius_write_u16(void *ptr, uint16_t value) -{ - value = htons(value); - memcpy(ptr, &value, sizeof(value)); -} - -static void radius_write_u32(void *ptr, uint32_t value) -{ - value = htonl(value); - memcpy(ptr, &value, sizeof(value)); -} - struct rad_packet_t *rad_packet_alloc(int code) { struct rad_packet_t *pack; @@ -139,20 +112,20 @@ int rad_packet_build(struct rad_packet_t *pack, uint8_t *RA) *ptr = pack->code; ptr++; *ptr = pack->id; ptr++; - radius_write_u16(ptr, pack->len); ptr += 2; + u_write_be16(ptr, pack->len); ptr += 2; memcpy(ptr, RA, 16); ptr+=16; list_for_each_entry(attr, &pack->attrs, entry) { if (attr->vendor) { *ptr = 26; ptr++; *ptr = attr->len + 2 + 6; ptr++; - radius_write_u32(ptr, attr->vendor->id); ptr += 4; + u_write_be32(ptr, attr->vendor->id); ptr += 4; } *ptr = attr->attr->id; ptr++; *ptr = attr->len + 2; ptr++; switch(attr->attr->type) { case ATTR_TYPE_INTEGER: - radius_write_u32(ptr, attr->val.integer); + u_write_be32(ptr, attr->val.integer); break; case ATTR_TYPE_OCTETS: case ATTR_TYPE_STRING: @@ -164,7 +137,7 @@ int rad_packet_build(struct rad_packet_t *pack, uint8_t *RA) memcpy(ptr, &attr->val, attr->len); break; case ATTR_TYPE_DATE: - radius_write_u32(ptr, attr->val.date); + u_write_be32(ptr, attr->val.date); break; case ATTR_TYPE_IPV6PREFIX: ptr[0] = 0; @@ -232,7 +205,7 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) pack->code = *ptr; ptr++; pack->id = *ptr; ptr++; - pack->len = radius_read_u16(ptr); ptr += 2; + pack->len = u_read_be16(ptr); ptr += 2; if (pack->len < 20 || pack->len > n) { log_ppp_warn("radius:packet: short packet received %i, expected %i\n", pack->len, n); @@ -263,7 +236,7 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) log_ppp_warn("radius:packet: vendor attribute too short (%i)\n", len); goto out_err; } - vendor_id = radius_read_u32(ptr); + vendor_id = u_read_be32(ptr); vendor = rad_dict_find_vendor_id(vendor_id); if (vendor) { if (len < 4 + vendor->tag + vendor->len) { @@ -273,14 +246,14 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) ptr += 4; if (vendor->tag == 2) - id = radius_read_u16(ptr); + id = u_read_be16(ptr); else id = *ptr; ptr += vendor->tag; if (vendor->len == 2) - len = radius_read_u16(ptr); + len = u_read_be16(ptr); else len = *ptr; @@ -333,15 +306,15 @@ int rad_packet_recv(int fd, struct rad_packet_t **p, struct sockaddr_in *addr) break; } if (len == 4) - attr->val.integer = radius_read_u32(ptr); + attr->val.integer = u_read_be32(ptr); else if (len == 2) - attr->val.integer = radius_read_u16(ptr); + attr->val.integer = u_read_be16(ptr); else if (len == 1) attr->val.integer = *ptr; break; case ATTR_TYPE_DATE: if (len == 4) - attr->val.integer = radius_read_u32(ptr); + attr->val.integer = u_read_be32(ptr); else log_ppp_warn("radius:packet: attribute %s has invalid length %i (must be 4)\n", da->name, len); break; diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h index 63c1db0d..7c62422c 100644 --- a/accel-pppd/utils.h +++ b/accel-pppd/utils.h @@ -1,13 +1,86 @@ #ifndef __UTILS_H #define __UTILS_H +#include #include #include +#include #ifndef min #define min(x, y) ((x) < (y) ? (x) : (y)) #endif +/* + * Fixed-size memcpy() lets the compiler emit efficient unaligned accesses on + * architectures that support them without imposing alignment or aliasing + * requirements on callers. + */ +static inline uint16_t u_read_be16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohs(value); +} + +static inline uint32_t u_read_be32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return ntohl(value); +} + +static inline uint64_t u_read_be64(const void *ptr) +{ + uint64_t value; + + memcpy(&value, ptr, sizeof(value)); + return be64toh(value); +} + +static inline void u_write_be16(void *ptr, uint16_t value) +{ + value = htons(value); + memcpy(ptr, &value, sizeof(value)); +} + +static inline void u_write_be32(void *ptr, uint32_t value) +{ + value = htonl(value); + memcpy(ptr, &value, sizeof(value)); +} + +static inline void u_write_be64(void *ptr, uint64_t value) +{ + value = htobe64(value); + memcpy(ptr, &value, sizeof(value)); +} + +static inline uint16_t u_read_native16(const void *ptr) +{ + uint16_t value; + + memcpy(&value, ptr, sizeof(value)); + return value; +} + +static inline uint32_t u_read_native32(const void *ptr) +{ + uint32_t value; + + memcpy(&value, ptr, sizeof(value)); + return value; +} + +static inline uint64_t u_read_native64(const void *ptr) +{ + uint64_t value; + + memcpy(&value, ptr, sizeof(value)); + return value; +} + char *u_ip6str(const struct in6_addr *addr, char *buf); char *u_ip4str(const struct in_addr *addr, char *buf); -- cgit v1.2.3 From d22666a84cc38b8018d839483db5df125fc6728b Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 09:31:53 +0300 Subject: ppp: bound IPCP and IPV6CP packets by the received size ipcp_recv() and ipv6cp_recv() only rejected packets whose header length field was below PPP_HEADERLEN. The upper bound was missing, so the option walker was handed a size derived purely from the peer chosen hdr->len. ppp->buf is a fixed 8192 byte mempool block and ppp->buf_size holds the number of bytes actually read, so a short frame declaring hdr->len 0xffff made the option loop run off the end of the block: it kept fetching option headers from whatever followed in the heap and stored pointers to them in the ropt list. The read is harmless in itself but easily reaches unmapped memory and kills the daemon. Reject any packet whose declared length does not fit in what was received, the way lcp_recv() and ccp_recv() already do. The two byte slack accounts for the protocol field that precedes the header inside the buffer, and the existing buf_size >= PPP_HEADERLEN + 2 test just above keeps the subtraction from underflowing. --- accel-pppd/ppp/ppp_ipcp.c | 2 +- accel-pppd/ppp/ppp_ipv6cp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/ppp/ppp_ipcp.c b/accel-pppd/ppp/ppp_ipcp.c index b67bfa44..a919883a 100644 --- a/accel-pppd/ppp/ppp_ipcp.c +++ b/accel-pppd/ppp/ppp_ipcp.c @@ -695,7 +695,7 @@ static void ipcp_recv(struct ppp_handler_t*h) } hdr = (struct ipcp_hdr_t *)ipcp->ppp->buf; - if (ntohs(hdr->len) < PPP_HEADERLEN) { + if (ntohs(hdr->len) < PPP_HEADERLEN || ntohs(hdr->len) > ipcp->ppp->buf_size - 2) { log_ppp_warn("IPCP: short packet received\n"); return; } diff --git a/accel-pppd/ppp/ppp_ipv6cp.c b/accel-pppd/ppp/ppp_ipv6cp.c index 755e8903..5f08e46c 100644 --- a/accel-pppd/ppp/ppp_ipv6cp.c +++ b/accel-pppd/ppp/ppp_ipv6cp.c @@ -699,7 +699,7 @@ static void ipv6cp_recv(struct ppp_handler_t*h) } hdr = (struct ipv6cp_hdr_t *)ipv6cp->ppp->buf; - if (ntohs(hdr->len) < PPP_HEADERLEN) { + if (ntohs(hdr->len) < PPP_HEADERLEN || ntohs(hdr->len) > ipv6cp->ppp->buf_size - 2) { log_ppp_warn("IPV6CP: short packet received\n"); return; } -- cgit v1.2.3 From 4654c4a9c083780f5e151ee064e69a357c48d364 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 1 Sep 2026 09:32:30 +0300 Subject: ppp: say why a malformed ConfReq drops the session The option walkers of lcp_recv_conf_req() and its IPCP, IPV6CP and CCP counterparts bail out with *_OPT_FAIL as soon as an option header does not fit in the remaining bytes or carries an impossible length, and the caller turns that into ap_session_terminate(TERM_USER_ERROR). The bail out happens before the "recv [LCP ConfReq id=..." line is emitted, so the session simply disappeared with nothing in the log to point at the peer. The ConfAck, ConfNak and ConfRej walkers break out of their loop instead and at least close the line they had already started. Log the offending length, option id and the number of bytes left before returning, so a peer sending malformed options can be told apart from the other reasons a session ends with TERM_USER_ERROR. --- accel-pppd/ppp/ppp_ccp.c | 9 +++++++-- accel-pppd/ppp/ppp_ipcp.c | 9 +++++++-- accel-pppd/ppp/ppp_ipv6cp.c | 9 +++++++-- accel-pppd/ppp/ppp_lcp.c | 9 +++++++-- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/accel-pppd/ppp/ppp_ccp.c b/accel-pppd/ppp/ppp_ccp.c index 4dda10f7..2082ca3f 100644 --- a/accel-pppd/ppp/ppp_ccp.c +++ b/accel-pppd/ppp/ppp_ccp.c @@ -387,13 +387,18 @@ static int ccp_recv_conf_req(struct ppp_ccp_t *ccp, uint8_t *data, int size) ccp->ropt_len = size; while (size > 0) { - if (size < sizeof(*hdr)) + if (size < sizeof(*hdr)) { + log_ppp_warn("CCP: ConfReq: truncated option header (%i bytes left)\n", size); return CCP_OPT_FAIL; + } hdr = (struct ccp_opt_hdr_t *)data; - if (hdr->len < sizeof(*hdr) || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + log_ppp_warn("CCP: ConfReq: invalid length %i of option %i (%i bytes left)\n", + hdr->len, hdr->id, size); return CCP_OPT_FAIL; + } ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); diff --git a/accel-pppd/ppp/ppp_ipcp.c b/accel-pppd/ppp/ppp_ipcp.c index a919883a..2f75542a 100644 --- a/accel-pppd/ppp/ppp_ipcp.c +++ b/accel-pppd/ppp/ppp_ipcp.c @@ -391,13 +391,18 @@ static int ipcp_recv_conf_req(struct ppp_ipcp_t *ipcp, uint8_t *data, int size) ipcp->ropt_len = size; while (size > 0) { - if (size < sizeof(*hdr)) + if (size < sizeof(*hdr)) { + log_ppp_warn("IPCP: ConfReq: truncated option header (%i bytes left)\n", size); return IPCP_OPT_FAIL; + } hdr = (struct ipcp_opt_hdr_t *)data; - if (hdr->len < sizeof(*hdr) || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + log_ppp_warn("IPCP: ConfReq: invalid length %i of option %i (%i bytes left)\n", + hdr->len, hdr->id, size); return IPCP_OPT_FAIL; + } ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); diff --git a/accel-pppd/ppp/ppp_ipv6cp.c b/accel-pppd/ppp/ppp_ipv6cp.c index 5f08e46c..370f1d55 100644 --- a/accel-pppd/ppp/ppp_ipv6cp.c +++ b/accel-pppd/ppp/ppp_ipv6cp.c @@ -395,13 +395,18 @@ static int ipv6cp_recv_conf_req(struct ppp_ipv6cp_t *ipv6cp, uint8_t *data, int ipv6cp->ropt_len = size; while (size > 0) { - if (size < sizeof(*hdr)) + if (size < sizeof(*hdr)) { + log_ppp_warn("IPV6CP: ConfReq: truncated option header (%i bytes left)\n", size); return IPV6CP_OPT_FAIL; + } hdr = (struct ipv6cp_opt_hdr_t *)data; - if (hdr->len < sizeof(*hdr) || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + log_ppp_warn("IPV6CP: ConfReq: invalid length %i of option %i (%i bytes left)\n", + hdr->len, hdr->id, size); return IPV6CP_OPT_FAIL; + } ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); diff --git a/accel-pppd/ppp/ppp_lcp.c b/accel-pppd/ppp/ppp_lcp.c index b44674f4..2424ca94 100644 --- a/accel-pppd/ppp/ppp_lcp.c +++ b/accel-pppd/ppp/ppp_lcp.c @@ -370,13 +370,18 @@ static int lcp_recv_conf_req(struct ppp_lcp_t *lcp, uint8_t *data, int size) lcp->ropt_len = size; while (size > 0) { - if (size < sizeof(*hdr)) + if (size < sizeof(*hdr)) { + log_ppp_warn("LCP: ConfReq: truncated option header (%i bytes left)\n", size); return LCP_OPT_FAIL; + } hdr = (struct lcp_opt_hdr_t *)data; - if (hdr->len < sizeof(*hdr) || hdr->len > size) + if (hdr->len < sizeof(*hdr) || hdr->len > size) { + log_ppp_warn("LCP: ConfReq: invalid length %i of option %i (%i bytes left)\n", + hdr->len, hdr->id, size); return LCP_OPT_FAIL; + } ropt = _malloc(sizeof(*ropt)); memset(ropt, 0, sizeof(*ropt)); -- cgit v1.2.3