This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/10054 ) Change subject: fix unaligned access in build_ipcp_pco() ...................................................................... fix unaligned access in build_ipcp_pco() IPCP data can begin at any byte location in the pco_req->v array. Casting to a 'struct ipcp_hdr' pointer could lead to unaligned access. Parse IPCP data with u_int8_t pointers instead to avoid this problem. Add some length checks while here. pco_contains_proto() and ipcp_contains_option() now receive the minimum size of the data the caller is looking for, and only return pointers to items of sufficient size. Also fix an inifinite loop in ipcp_contains_option() by refusing IPCP options with length small than 2. Previously, a zero length option would trigger an infinite loop in the parser. Change-Id: Ia1410abb216831864042f95679330f4508e1af3d Related: OS#3194 --- M ggsn/ggsn.c 1 file changed, 31 insertions(+), 20 deletions(-) Approvals: Jenkins Builder: Verified Pau Espin Pedrol: Looks good to me, but someone else must approve Harald Welte: Looks good to me, approved diff --git a/ggsn/ggsn.c b/ggsn/ggsn.c index 6d879c0..30584ef 100644 --- a/ggsn/ggsn.c +++ b/ggsn/ggsn.c @@ -413,16 +413,19 @@ } __attribute__ ((packed)); /* determine if IPCP contains given option */ -static struct ipcp_option_hdr *ipcp_contains_option(struct ipcp_hdr *ipcp, enum ipcp_options opt) +static uint8_t *ipcp_contains_option(uint8_t *ipcp, size_t ipcp_len, enum ipcp_options opt, size_t opt_minlen) { - uint8_t *cur = ipcp->options; + uint8_t *cur_opt = ipcp + sizeof(struct ipcp_hdr); /* iterate over Options and check if protocol contained */ - while (cur + 2 <= ((uint8_t *)ipcp) + ntohs(ipcp->len)) { - struct ipcp_option_hdr *cur_opt = (struct ipcp_option_hdr *) cur; - if (cur_opt->type == opt) + while (cur_opt + 2 <= ipcp + ipcp_len) { + uint8_t type = cur_opt[0]; + uint8_t len = cur_opt[1]; /* length value includes 2 bytes type/length */ + if (len < 2) + return NULL; + if (type == opt && len >= 2 + opt_minlen) return cur_opt; - cur += cur_opt->len; + cur_opt += len; } return NULL; } @@ -460,7 +463,7 @@ }; /* determine if PCO contains given protocol */ -static uint8_t *pco_contains_proto(struct ul255_t *pco, uint16_t prot) +static uint8_t *pco_contains_proto(struct ul255_t *pco, uint16_t prot, size_t prot_minlen) { uint8_t *cur = pco->v + 1; @@ -468,7 +471,7 @@ while (cur + 3 <= pco->v + pco->l) { uint16_t cur_prot = osmo_load16be(cur); uint8_t cur_len = cur[2]; - if (cur_prot == prot) + if (cur_prot == prot && cur_len >= prot_minlen) return cur; cur += cur_len + 3; } @@ -500,35 +503,45 @@ } /* construct an IPCP PCO response from request*/ -static int build_ipcp_pco(struct apn_ctx *apn, struct pdp_t *pdp, struct msgb *msg) +static void build_ipcp_pco(struct apn_ctx *apn, struct pdp_t *pdp, struct msgb *msg) { const struct in46_addr *dns1 = &apn->v4.cfg.dns[0]; const struct in46_addr *dns2 = &apn->v4.cfg.dns[1]; - struct ipcp_hdr *ipcp; + uint8_t *ipcp; + uint16_t ipcp_len; uint8_t *len1, *len2, *pco_ipcp; uint8_t *start = msg->tail; unsigned int len_appended; + ptrdiff_t consumed; + size_t remain; - if (!(pco_ipcp = pco_contains_proto(&pdp->pco_req, PCO_P_IPCP))) - return 0; - ipcp = (struct ipcp_hdr*) (pco_ipcp + 3); /* 2=type + 1=len */ + /* pco_contains_proto() returns a potentially unaligned pointer into pco_req->v (see OS#3194) */ + if (!(pco_ipcp = pco_contains_proto(&pdp->pco_req, PCO_P_IPCP, sizeof(struct ipcp_hdr)))) + return; + + ipcp = (pco_ipcp + 3); /* 2=type + 1=len */ + consumed = (ipcp - &pdp->pco_req.v[0]); + remain = sizeof(pdp->pco_req.v) - consumed; + ipcp_len = osmo_load16be(ipcp + 2); /* 1=code + 1=id */ + if (remain < 0 || remain < ipcp_len) + return; /* Three byte T16L header */ msgb_put_u16(msg, 0x8021); /* IPCP */ len1 = msgb_put(msg, 1); /* Length of contents: delay */ msgb_put_u8(msg, 0x02); /* ACK */ - msgb_put_u8(msg, ipcp->id); /* ID: Needs to match request */ + msgb_put_u8(msg, ipcp[1]); /* ID: Needs to match request */ msgb_put_u8(msg, 0x00); /* Length MSB */ len2 = msgb_put(msg, 1); /* Length LSB: delay */ - if (dns1->len == 4 && ipcp_contains_option(ipcp, IPCP_OPT_PRIMARY_DNS)) { + if (dns1->len == 4 && ipcp_contains_option(ipcp, ipcp_len, IPCP_OPT_PRIMARY_DNS, 4)) { msgb_put_u8(msg, 0x81); /* DNS1 Tag */ msgb_put_u8(msg, 2 + dns1->len);/* DNS1 Length, incl. TL */ msgb_put_u32(msg, ntohl(dns1->v4.s_addr)); } - if (dns2->len == 4 && ipcp_contains_option(ipcp, IPCP_OPT_SECONDARY_DNS)) { + if (dns2->len == 4 && ipcp_contains_option(ipcp, ipcp_len, IPCP_OPT_SECONDARY_DNS, 4)) { msgb_put_u8(msg, 0x83); /* DNS2 Tag */ msgb_put_u8(msg, 2 + dns2->len);/* DNS2 Length, incl. TL */ msgb_put_u32(msg, ntohl(dns2->v4.s_addr)); @@ -538,8 +551,6 @@ len_appended = msg->tail - start; *len1 = len_appended - 3; *len2 = len_appended - 3; - - return 0; } /* process one PCO request from a MS/UE, putting together the proper responses */ @@ -555,7 +566,7 @@ if (peer_v4) build_ipcp_pco(apn, pdp, msg); - if (pco_contains_proto(&pdp->pco_req, PCO_P_DNS_IPv6_ADDR)) { + if (pco_contains_proto(&pdp->pco_req, PCO_P_DNS_IPv6_ADDR, 0)) { for (i = 0; i < ARRAY_SIZE(apn->v6.cfg.dns); i++) { struct in46_addr *i46a = &apn->v6.cfg.dns[i]; if (i46a->len != 16) @@ -564,7 +575,7 @@ } } - if (pco_contains_proto(&pdp->pco_req, PCO_P_DNS_IPv4_ADDR)) { + if (pco_contains_proto(&pdp->pco_req, PCO_P_DNS_IPv4_ADDR, 0)) { for (i = 0; i < ARRAY_SIZE(apn->v4.cfg.dns); i++) { struct in46_addr *i46a = &apn->v4.cfg.dns[i]; if (i46a->len != 4) -- To view, visit https://gerrit.osmocom.org/10054 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia1410abb216831864042f95679330f4508e1af3d Gerrit-Change-Number: 10054 Gerrit-PatchSet: 4 Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180721/ba35359a/attachment.htm>