n0k0 has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/43187?usp=email ) Change subject: gtp: clamp GSN-Address copy to sockaddr_in size ...................................................................... gtp: clamp GSN-Address copy to sockaddr_in size gtp_data_req() builds an IPv4 destination sockaddr_in and does memcpy(&addr.sin_addr, pdp->gsnru.v, pdp->gsnru.l); gtp_gpdu_ind() checks the GPDU source with memcmp(&peer->sin_addr, pdp->gsnru.v, pdp->gsnru.l). Both use the wire GSN-Address length as the count into the 4-byte sin_addr, so a GSN-Address longer than 4 bytes (e.g. a 16-byte IPv6 address) writes up to 4 bytes past the sockaddr_in on the stack, or reads up to 12 bytes past sin_addr. Both sites already carried a 'TODO range check' comment. This is the AF_INET user-plane path, where the GSN address is a 4-byte IPv4 address, so copy and compare exactly sizeof(sin_addr) bytes. Change-Id: If49f1929645c0ba8d19f3c9aa95f57d0a432ad53 --- M gtp/gtp.c 1 file changed, 8 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-ggsn refs/changes/87/43187/1 diff --git a/gtp/gtp.c b/gtp/gtp.c index fa65575..7a439d9 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -3280,7 +3280,10 @@ } /* If the GPDU was not from the peer GSN tell him to delete context */ - if (memcmp(&peer->sin_addr, pdp->gsnru.v, pdp->gsnru.l)) { /* TODO Range? */ + /* Compare only the IPv4 address bytes; peer->sin_addr is 4 bytes, so + * bound the compare to sizeof(sin_addr) rather than the (attacker-influenced) + * gsnru length to avoid reading past sin_addr. */ + if (memcmp(&peer->sin_addr, pdp->gsnru.v, sizeof(peer->sin_addr))) { rate_ctr_inc2(gsn->ctrg, GSN_CTR_ERR_UNKNOWN_PDP); GTP_LOGPKG(LOGL_ERROR, peer, pack, len, "Unknown GSN peer %s\n", inet_ntoa(peer->sin_addr)); return gtp_error_ind_resp(gsn, version, peer, fd, pack, len); @@ -3778,7 +3781,10 @@ #if defined(__FreeBSD__) || defined(__APPLE__) addr.sin_len = sizeof(addr); #endif - memcpy(&addr.sin_addr, pdp->gsnru.v, pdp->gsnru.l); /* TODO range check */ + /* gsnru is the IPv4 GSN user-plane address for this AF_INET path; copy + * exactly sizeof(sin_addr) bytes so an over-long (e.g. 16-byte IPv6) + * address cannot write past addr on the stack. */ + memcpy(&addr.sin_addr, pdp->gsnru.v, sizeof(addr.sin_addr)); /* prepare msghdr */ memset(&msgh, 0, sizeof(msgh)); -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/43187?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: If49f1929645c0ba8d19f3c9aa95f57d0a432ad53 Gerrit-Change-Number: 43187 Gerrit-PatchSet: 1 Gerrit-Owner: n0k0 <osmocom@hacky.software>