From d3e15ad21e06a1cf20af900b8ff7ffd4dba785c4 Mon Sep 17 00:00:00 2001 From: Pau Espin Pedrol Date: Fri, 8 Jun 2018 13:10:13 +0200 Subject: bsc-nat: Avoid heap-use-after-free on bsc auth failure Previous to this patch, if ipaccess_auth_bsc() failed finding the requested auth token, it would call bsc_close_connection() on it. However, it would not report callers that the bsc conn was closed. Since ipaccess_auth_bsc is called in the following path: [osmo_wqueue_bfd_cb->ipaccess_bsc_read_cb->forward_sccp_to_msc->ipaccess_auth_bsc] It needs to notify the lower layers (wqueue) that the conn/osmo_fd has been freed an it should avoid keep using/forwarding it again. This patch fixes this issue by moving the conn closing one layer down the stack (from ipaccess_auth_bsc to forward_sccp_to_msc), and in there we now close the conn and provide required information to the callers. Fixes following Asan report: Unit_Name='foobar' <0015> openbsc/openbsc/src/osmo-bsc_nat/bsc_nat.c:1061 No bsc found for token 'foobar' len 6 on fd: 11. ================================================================= ==18946==ERROR: AddressSanitizer: heap-use-after-free on address 0x616001f8b81c at pc 0x7ffff6067540 bp 0x7fffffffe170 sp 0x7fffffffe168 READ of size 4 at 0x616001f8b81c thread T0 #0 0x7ffff606753f in osmo_wqueue_bfd_cb libosmocore/src/write_queue.c:65 #1 0x7ffff605206b in osmo_fd_disp_fds libosmocore/src/select.c:217 #2 0x7ffff6052305 in osmo_select_main libosmocore/src/select.c:257 #3 0x421c8e in main openbsc/openbsc/src/osmo-bsc_nat/bsc_nat.c:1714 #4 0x7ffff47ffb44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44) #5 0x406438 (/bin/osmo-bsc_nat+0x406438) Fixes: SYS#4250 Change-Id: Ifb39a045b98bc2043a98a9787fc61cbcddc368e0 --- openbsc/src/osmo-bsc_nat/bsc_nat.c | 51 ++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 19 deletions(-) (limited to 'openbsc/src/osmo-bsc_nat') diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index fb2ec8347..e912f60b4 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -958,21 +958,23 @@ void bsc_close_connection(struct bsc_connection *connection) talloc_free(connection); } -static void bsc_maybe_close(struct bsc_connection *bsc) +/* Returns true if bsc_close_connection() was called, false otherwise */ +static bool bsc_maybe_close(struct bsc_connection *bsc) { struct nat_sccp_connection *sccp; if (!bsc->nat->blocked) - return; + return false; /* are there any connections left */ llist_for_each_entry(sccp, &bsc->nat->sccp_connections, list_entry) if (sccp->bsc == bsc) - return; + return false; /* nothing left, close the BSC */ LOGP(DNAT, LOGL_NOTICE, "Cleaning up BSC %d in blocking mode.\n", bsc->cfg ? bsc->cfg->nr : -1); bsc_close_connection(bsc); + return true; } static void ipaccess_close_bsc(void *data) @@ -1021,7 +1023,8 @@ static int verify_key(struct bsc_connection *conn, struct bsc_config *conf, cons return osmo_constant_time_cmp(vec.res, key, 8) == 0; } -static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc) +/* Returns true if connection was successfully authenticated, false otherwise. */ +static bool ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc) { struct bsc_config *conf; const char *token = (const char *) TLVP_VAL(tvp, IPAC_IDTAG_UNITNAME); @@ -1032,21 +1035,19 @@ static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc if (bsc->cfg) { LOGP(DNAT, LOGL_ERROR, "Reauth on fd %d bsc nr %d\n", bsc->write_queue.bfd.fd, bsc->cfg->nr); - return; + return true; } if (len <= 0) { LOGP(DNAT, LOGL_ERROR, "Token with length zero on fd: %d\n", bsc->write_queue.bfd.fd); - bsc_close_connection(bsc); - return; + return false; } if (token[len - 1] != '\0') { LOGP(DNAT, LOGL_ERROR, "Token not null terminated on fd: %d\n", bsc->write_queue.bfd.fd); - bsc_close_connection(bsc); - return; + return false; } /* @@ -1061,8 +1062,7 @@ static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc LOGP(DNAT, LOGL_ERROR, "No bsc found for token '%s' len %d on fd: %d.\n", token, bsc->write_queue.bfd.fd, len); - bsc_close_connection(bsc); - return; + return false; } /* We have set a key and expect it to be present */ @@ -1070,8 +1070,7 @@ static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc LOGP(DNAT, LOGL_ERROR, "Wrong key for bsc nr %d fd: %d.\n", conf->nr, bsc->write_queue.bfd.fd); - bsc_close_connection(bsc); - return; + return false; } rate_ctr_inc(&conf->stats.ctrg->ctr[BCFG_CTR_NET_RECONN]); @@ -1081,6 +1080,7 @@ static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc LOGP(DNAT, LOGL_NOTICE, "Authenticated bsc nr: %d on fd %d\n", conf->nr, bsc->write_queue.bfd.fd); start_ping_pong(bsc); + return true; } static void handle_con_stats(struct nat_sccp_connection *con) @@ -1098,7 +1098,14 @@ static void handle_con_stats(struct nat_sccp_connection *con) rate_ctr_inc(&ctrg->ctr[id]); } -static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg) +/*! + * Forward messages to msc and verify received authentication messages. + * \param[in] bsc Pointer to bsc_connection structure from which the message was received. + * \param[in] msg The msg received to be forwarded to the msc. + * \param[out] bsc_conn_closed Whether bsc_close_connection(bsc) was called inside the function. + * \returns 0 on success, -1 on error. + */ +static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg, bool *bsc_conn_closed) { int con_filter = 0; char *imsi = NULL; @@ -1107,6 +1114,7 @@ static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg) int con_type; struct bsc_nat_parsed *parsed; struct bsc_filter_reject_cause cause; + *bsc_conn_closed = false; /* Parse and filter messages */ parsed = bsc_nat_parse(msg); @@ -1216,7 +1224,7 @@ static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg) con_filter = con->con_local; } remove_sccp_src_ref(bsc, msg, parsed); - bsc_maybe_close(bsc); + *bsc_conn_closed = bsc_maybe_close(bsc); break; case SCCP_MSG_TYPE_UDT: /* simply forward everything */ @@ -1277,8 +1285,12 @@ exit: "message with malformed TLVs\n"); return ret; } - if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) - ipaccess_auth_bsc(&tvp, bsc); + if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) { + if (!ipaccess_auth_bsc(&tvp, bsc)) { + bsc_close_connection(bsc); + *bsc_conn_closed = true; + } + } } } @@ -1296,6 +1308,7 @@ static int ipaccess_bsc_read_cb(struct osmo_fd *bfd) struct msgb *msg = NULL; struct ipaccess_head *hh; struct ipaccess_head_ext *hh_ext; + bool fd_closed = false; int ret; ret = ipa_msg_recv_buffered(bfd->fd, &msg, &bsc->pending_msg); @@ -1344,8 +1357,8 @@ static int ipaccess_bsc_read_cb(struct osmo_fd *bfd) /* FIXME: Currently no PONG is sent to the BSC */ /* FIXME: Currently no ID ACK is sent to the BSC */ - forward_sccp_to_msc(bsc, msg); - return 0; + forward_sccp_to_msc(bsc, msg, &fd_closed); + return fd_closed ? -EBADF : 0; close_fd: bsc_close_connection(bsc); -- cgit v1.2.3