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(-) (limited to 'accel-pppd') 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