<p>Harald Welte <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/9511">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
Harald Welte: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bsc-nat: Avoid heap-use-after-free on bsc auth failure<br><br>Previous to this patch, if ipaccess_auth_bsc() failed finding the<br>requested auth token, it would call bsc_close_connection() on it.<br>However, it would not report callers that the bsc conn was closed.<br><br>Since ipaccess_auth_bsc is called in the following path:<br>[osmo_wqueue_bfd_cb->ipaccess_bsc_read_cb->forward_sccp_to_msc->ipaccess_auth_bsc]<br>It needs to notify the lower layers (wqueue) that the conn/osmo_fd has been<br>freed an it should avoid keep using/forwarding it again.<br><br>This patch fixes this issue by moving the conn closing one layer down<br>the stack (from ipaccess_auth_bsc to forward_sccp_to_msc), and in there<br>we now close the conn and provide required information to the callers.<br><br>Fixes following Asan report:<br>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.<br>=================================================================<br>==18946==ERROR: AddressSanitizer: heap-use-after-free on address 0x616001f8b81c at pc 0x7ffff6067540 bp 0x7fffffffe170 sp 0x7fffffffe168<br>READ of size 4 at 0x616001f8b81c thread T0<br> #0 0x7ffff606753f in osmo_wqueue_bfd_cb libosmocore/src/write_queue.c:65<br> #1 0x7ffff605206b in osmo_fd_disp_fds libosmocore/src/select.c:217<br> #2 0x7ffff6052305 in osmo_select_main libosmocore/src/select.c:257<br> #3 0x421c8e in main openbsc/openbsc/src/osmo-bsc_nat/bsc_nat.c:1714<br> #4 0x7ffff47ffb44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)<br> #5 0x406438 (/bin/osmo-bsc_nat+0x406438)<br><br>Fixes: SYS#4250<br>Change-Id: Ifb39a045b98bc2043a98a9787fc61cbcddc368e0<br>---<br>M openbsc/src/osmo-bsc_nat/bsc_nat.c<br>1 file changed, 32 insertions(+), 19 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c</span><br><span>index fb2ec83..e912f60 100644</span><br><span>--- a/openbsc/src/osmo-bsc_nat/bsc_nat.c</span><br><span>+++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c</span><br><span>@@ -958,21 +958,23 @@</span><br><span> talloc_free(connection);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static void bsc_maybe_close(struct bsc_connection *bsc)</span><br><span style="color: hsl(120, 100%, 40%);">+/* Returns true if bsc_close_connection() was called, false otherwise */</span><br><span style="color: hsl(120, 100%, 40%);">+static bool bsc_maybe_close(struct bsc_connection *bsc)</span><br><span> {</span><br><span> struct nat_sccp_connection *sccp;</span><br><span> if (!bsc->nat->blocked)</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> </span><br><span> /* are there any connections left */</span><br><span> llist_for_each_entry(sccp, &bsc->nat->sccp_connections, list_entry)</span><br><span> if (sccp->bsc == bsc)</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> </span><br><span> /* nothing left, close the BSC */</span><br><span> LOGP(DNAT, LOGL_NOTICE, "Cleaning up BSC %d in blocking mode.\n",</span><br><span> bsc->cfg ? bsc->cfg->nr : -1);</span><br><span> bsc_close_connection(bsc);</span><br><span style="color: hsl(120, 100%, 40%);">+ return true;</span><br><span> }</span><br><span> </span><br><span> static void ipaccess_close_bsc(void *data)</span><br><span>@@ -1021,7 +1023,8 @@</span><br><span> return osmo_constant_time_cmp(vec.res, key, 8) == 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc)</span><br><span style="color: hsl(120, 100%, 40%);">+/* Returns true if connection was successfully authenticated, false otherwise. */</span><br><span style="color: hsl(120, 100%, 40%);">+static bool ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc)</span><br><span> {</span><br><span> struct bsc_config *conf;</span><br><span> const char *token = (const char *) TLVP_VAL(tvp, IPAC_IDTAG_UNITNAME);</span><br><span>@@ -1032,21 +1035,19 @@</span><br><span> if (bsc->cfg) {</span><br><span> LOGP(DNAT, LOGL_ERROR, "Reauth on fd %d bsc nr %d\n",</span><br><span> bsc->write_queue.bfd.fd, bsc->cfg->nr);</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return true;</span><br><span> }</span><br><span> </span><br><span> if (len <= 0) {</span><br><span> LOGP(DNAT, LOGL_ERROR, "Token with length zero on fd: %d\n",</span><br><span> bsc->write_queue.bfd.fd);</span><br><span style="color: hsl(0, 100%, 40%);">- bsc_close_connection(bsc);</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> }</span><br><span> </span><br><span> if (token[len - 1] != '\0') {</span><br><span> LOGP(DNAT, LOGL_ERROR, "Token not null terminated on fd: %d\n",</span><br><span> bsc->write_queue.bfd.fd);</span><br><span style="color: hsl(0, 100%, 40%);">- bsc_close_connection(bsc);</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> }</span><br><span> </span><br><span> /*</span><br><span>@@ -1061,8 +1062,7 @@</span><br><span> LOGP(DNAT, LOGL_ERROR,</span><br><span> "No bsc found for token '%s' len %d on fd: %d.\n", token,</span><br><span> bsc->write_queue.bfd.fd, len);</span><br><span style="color: hsl(0, 100%, 40%);">- bsc_close_connection(bsc);</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> }</span><br><span> </span><br><span> /* We have set a key and expect it to be present */</span><br><span>@@ -1070,8 +1070,7 @@</span><br><span> LOGP(DNAT, LOGL_ERROR,</span><br><span> "Wrong key for bsc nr %d fd: %d.\n", conf->nr,</span><br><span> bsc->write_queue.bfd.fd);</span><br><span style="color: hsl(0, 100%, 40%);">- bsc_close_connection(bsc);</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(120, 100%, 40%);">+ return false;</span><br><span> }</span><br><span> </span><br><span> rate_ctr_inc(&conf->stats.ctrg->ctr[BCFG_CTR_NET_RECONN]);</span><br><span>@@ -1081,6 +1080,7 @@</span><br><span> LOGP(DNAT, LOGL_NOTICE, "Authenticated bsc nr: %d on fd %d\n",</span><br><span> conf->nr, bsc->write_queue.bfd.fd);</span><br><span> start_ping_pong(bsc);</span><br><span style="color: hsl(120, 100%, 40%);">+ return true;</span><br><span> }</span><br><span> </span><br><span> static void handle_con_stats(struct nat_sccp_connection *con)</span><br><span>@@ -1098,7 +1098,14 @@</span><br><span> rate_ctr_inc(&ctrg->ctr[id]);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg)</span><br><span style="color: hsl(120, 100%, 40%);">+/*!</span><br><span style="color: hsl(120, 100%, 40%);">+ * Forward messages to msc and verify received authentication messages.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] bsc Pointer to bsc_connection structure from which the message was received.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] msg The msg received to be forwarded to the msc.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[out] bsc_conn_closed Whether bsc_close_connection(bsc) was called inside the function.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \returns 0 on success, -1 on error.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg, bool *bsc_conn_closed)</span><br><span> {</span><br><span> int con_filter = 0;</span><br><span> char *imsi = NULL;</span><br><span>@@ -1107,6 +1114,7 @@</span><br><span> int con_type;</span><br><span> struct bsc_nat_parsed *parsed;</span><br><span> struct bsc_filter_reject_cause cause;</span><br><span style="color: hsl(120, 100%, 40%);">+ *bsc_conn_closed = false;</span><br><span> </span><br><span> /* Parse and filter messages */</span><br><span> parsed = bsc_nat_parse(msg);</span><br><span>@@ -1216,7 +1224,7 @@</span><br><span> con_filter = con->con_local;</span><br><span> }</span><br><span> remove_sccp_src_ref(bsc, msg, parsed);</span><br><span style="color: hsl(0, 100%, 40%);">- bsc_maybe_close(bsc);</span><br><span style="color: hsl(120, 100%, 40%);">+ *bsc_conn_closed = bsc_maybe_close(bsc);</span><br><span> break;</span><br><span> case SCCP_MSG_TYPE_UDT:</span><br><span> /* simply forward everything */</span><br><span>@@ -1277,8 +1285,12 @@</span><br><span> "message with malformed TLVs\n");</span><br><span> return ret;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME))</span><br><span style="color: hsl(0, 100%, 40%);">- ipaccess_auth_bsc(&tvp, bsc);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ipaccess_auth_bsc(&tvp, bsc)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ bsc_close_connection(bsc);</span><br><span style="color: hsl(120, 100%, 40%);">+ *bsc_conn_closed = true;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> }</span><br><span> }</span><br><span> </span><br><span>@@ -1296,6 +1308,7 @@</span><br><span> struct msgb *msg = NULL;</span><br><span> struct ipaccess_head *hh;</span><br><span> struct ipaccess_head_ext *hh_ext;</span><br><span style="color: hsl(120, 100%, 40%);">+ bool fd_closed = false;</span><br><span> int ret;</span><br><span> </span><br><span> ret = ipa_msg_recv_buffered(bfd->fd, &msg, &bsc->pending_msg);</span><br><span>@@ -1344,8 +1357,8 @@</span><br><span> </span><br><span> /* FIXME: Currently no PONG is sent to the BSC */</span><br><span> /* FIXME: Currently no ID ACK is sent to the BSC */</span><br><span style="color: hsl(0, 100%, 40%);">- forward_sccp_to_msc(bsc, msg);</span><br><span style="color: hsl(0, 100%, 40%);">- return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ forward_sccp_to_msc(bsc, msg, &fd_closed);</span><br><span style="color: hsl(120, 100%, 40%);">+ return fd_closed ? -EBADF : 0;</span><br><span> </span><br><span> close_fd:</span><br><span> bsc_close_connection(bsc);</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/9511">change 9511</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/9511"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: openbsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ifb39a045b98bc2043a98a9787fc61cbcddc368e0 </div>
<div style="display:none"> Gerrit-Change-Number: 9511 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>