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/9511 )
Change subject: bsc-nat: Avoid heap-use-after-free on bsc auth failure
......................................................................
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
---
M openbsc/src/osmo-bsc_nat/bsc_nat.c
1 file changed, 32 insertions(+), 19 deletions(-)
Approvals:
Jenkins Builder: Verified
Harald Welte: Looks good to me, approved
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c
index fb2ec83..e912f60 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c
@@ -958,21 +958,23 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
"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 @@
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 @@
/* 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);
--
To view, visit https://gerrit.osmocom.org/9511
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifb39a045b98bc2043a98a9787fc61cbcddc368e0
Gerrit-Change-Number: 9511
Gerrit-PatchSet: 1
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180608/255a86ed/attachment.htm>