Change in osmo-remsim[master]: server: Detect duplicate client/bankd connection; drop new ones

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-remsim/+/28027 ) Change subject: server: Detect duplicate client/bankd connection; drop new ones ...................................................................... server: Detect duplicate client/bankd connection; drop new ones we're dropping the current (new) connection as we don't really know which is the "right" one. Dropping the new gives the old connection time to timeout, or to continue to operate. If we were to drop the old connection, this could interrupt a perfectly working connection and opens some kind of DoS. Related: OS#5527 Change-Id: I00387dbc19d689415470e2f08df08a47a78b81c0 --- M asn1/RSPRO.asn M include/osmocom/rspro/ResultCode.h M src/rspro/ResultCode.c M src/server/rspro_server.c 4 files changed, 61 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-remsim refs/changes/27/28027/1 diff --git a/asn1/RSPRO.asn b/asn1/RSPRO.asn index d49345d..7cba1b9 100644 --- a/asn1/RSPRO.asn +++ b/asn1/RSPRO.asn @@ -87,6 +87,7 @@ illegalSlotId (3), unsupportedProtocolVersion (4), unknownSlotmap (5), + identityInUse (6), -- no card is present in given slot cardNotPresent (100), diff --git a/include/osmocom/rspro/ResultCode.h b/include/osmocom/rspro/ResultCode.h index 1342990..9a2cbd2 100644 --- a/include/osmocom/rspro/ResultCode.h +++ b/include/osmocom/rspro/ResultCode.h @@ -25,6 +25,7 @@ ResultCode_illegalSlotId = 3, ResultCode_unsupportedProtocolVersion = 4, ResultCode_unknownSlotmap = 5, + ResultCode_identityInUse = 6, ResultCode_cardNotPresent = 100, ResultCode_cardUnresponsive = 101, ResultCode_cardTransmissionError = 102 diff --git a/src/rspro/ResultCode.c b/src/rspro/ResultCode.c index f60bf0c..76ca2fa 100644 --- a/src/rspro/ResultCode.c +++ b/src/rspro/ResultCode.c @@ -89,15 +89,17 @@ { 3, 13, "illegalSlotId" }, { 4, 26, "unsupportedProtocolVersion" }, { 5, 14, "unknownSlotmap" }, + { 6, 13, "identityInUse" }, { 100, 14, "cardNotPresent" }, { 101, 16, "cardUnresponsive" }, { 102, 21, "cardTransmissionError" } /* This list is extensible */ }; static const unsigned int asn_MAP_ResultCode_enum2value_1[] = { - 6, /* cardNotPresent(100) */ - 8, /* cardTransmissionError(102) */ - 7, /* cardUnresponsive(101) */ + 7, /* cardNotPresent(100) */ + 9, /* cardTransmissionError(102) */ + 8, /* cardUnresponsive(101) */ + 6, /* identityInUse(6) */ 2, /* illegalBankId(2) */ 1, /* illegalClientId(1) */ 3, /* illegalSlotId(3) */ @@ -109,8 +111,8 @@ static const asn_INTEGER_specifics_t asn_SPC_ResultCode_specs_1 = { asn_MAP_ResultCode_value2enum_1, /* "tag" => N; sorted by tag */ asn_MAP_ResultCode_enum2value_1, /* N => "tag"; sorted by N */ - 9, /* Number of elements in the maps */ - 10, /* Extensions before this member */ + 10, /* Number of elements in the maps */ + 11, /* Extensions before this member */ 1, /* Strict enumeration */ 0, /* Native long size */ 0 diff --git a/src/server/rspro_server.c b/src/server/rspro_server.c index c4d4d79..b753993 100644 --- a/src/server/rspro_server.c +++ b/src/server/rspro_server.c @@ -76,6 +76,7 @@ CLNTC_ST_WAIT_CONF_RES, /* waiting for ConfigClientRes */ CLNTC_ST_CONNECTED_BANKD, CLNTC_ST_CONNECTED_CLIENT, + CLNTC_ST_REJECTED, }; enum remsim_server_client_event { @@ -119,6 +120,7 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void *data) { struct rspro_client_conn *conn = fi->priv; + struct rspro_client_conn *previous_conn; const RsproPDU_t *pdu = data; const ConnectClientReq_t *cclreq = NULL; const ConnectBankReq_t *cbreq = NULL; @@ -141,11 +143,6 @@ return; } - /* reparent us from srv->connections to srv->clients */ - pthread_rwlock_wrlock(&conn->srv->rwlock); - llist_del(&conn->list); - llist_add_tail(&conn->list, &conn->srv->clients); - pthread_rwlock_unlock(&conn->srv->rwlock); if (!cclreq->clientSlot) { #if 0 @@ -161,7 +158,6 @@ osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL); #endif } else { - /* FIXME: check for unique-ness */ rspro2client_slot(&conn->client.slot, cclreq->clientSlot); osmo_fsm_inst_update_id_f(fi, "C%u:%u", conn->client.slot.client_id, conn->client.slot.slot_nr); @@ -169,6 +165,30 @@ conn->client.slot.client_id, conn->client.slot.slot_nr); LOGPFSML(fi, LOGL_INFO, "Client connected from %s:%s\n", ip_str, port_str); + + /* check for unique-ness */ + previous_conn = client_conn_by_slot(conn->srv, &conn->client.slot); + if (previous_conn && previous_conn != conn) { + /* we're dropping the current (new) connection as we don't really know which + * is the "right" one. Dropping the new gives the old connection time to + * timeout, or to continue to operate. If we were to drop the old + * connection, this could interrupt a perfectly working connection and opens + * some kind of DoS. */ + LOGPFSML(fi, LOGL_ERROR, "New client connection from %s:%s, but we already " + "have a connection from %s:%u. Dropping new connection.\n", + ip_str, port_str, previous_conn->peer->addr, previous_conn->peer->port); + resp = rspro_gen_ConnectClientRes(&conn->srv->comp_id, ResultCode_identityInUse); + client_conn_send(conn, resp); + osmo_fsm_inst_state_chg(fi, CLNTC_ST_REJECTED, 1, 2); + return; + } + + /* reparent us from srv->connections to srv->clients */ + pthread_rwlock_wrlock(&conn->srv->rwlock); + llist_del(&conn->list); + llist_add_tail(&conn->list, &conn->srv->clients); + pthread_rwlock_unlock(&conn->srv->rwlock); + resp = rspro_gen_ConnectClientRes(&conn->srv->comp_id, ResultCode_ok); client_conn_send(conn, resp); osmo_fsm_inst_state_chg(fi, CLNTC_ST_CONNECTED_CLIENT, 0, 0); @@ -183,7 +203,6 @@ osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL); return; } - /* FIXME: check for unique-ness */ conn->bank.bank_id = cbreq->bankId; conn->bank.num_slots = cbreq->numberOfSlots; osmo_fsm_inst_update_id_f(fi, "B%u", conn->bank.bank_id); @@ -196,6 +215,23 @@ "as they must be able to reach the bankd!\n", ip_str); } + /* check for unique-ness */ + previous_conn = bankd_conn_by_id(conn->srv, conn->bank.bank_id); + if (previous_conn && previous_conn != conn) { + /* we're dropping the current (new) connection as we don't really know which + * is the "right" one. Dropping the new gives the old connection time to + * timeout, or to continue to operate. If we were to drop the old + * connection, this could interrupt a perfectly working connection and opens + * some kind of DoS. */ + LOGPFSML(fi, LOGL_ERROR, "New bankd connection from %s:%s, but we already " + "have a connection from %s:%u. Dropping new connection.\n", + ip_str, port_str, previous_conn->peer->addr, previous_conn->peer->port); + resp = rspro_gen_ConnectClientRes(&conn->srv->comp_id, ResultCode_identityInUse); + client_conn_send(conn, resp); + osmo_fsm_inst_state_chg(fi, CLNTC_ST_REJECTED, 1, 2); + return; + } + /* reparent us from srv->connections to srv->banks */ pthread_rwlock_wrlock(&conn->srv->rwlock); llist_del(&conn->list); @@ -445,6 +481,10 @@ case 1: /* No ClientConnectReq received:disconnect */ return 1; /* ask core to terminate FSM */ + case 2: + /* Timeout after rejecting client */ + LOGPFSML(fi, LOGL_NOTICE, "Closing connection of rejected peer\n"); + return 1; /* ask core to terminate FSM */ default: OSMO_ASSERT(0); } @@ -471,7 +511,7 @@ .name = "ESTABLISHED", .in_event_mask = S(CLNTC_E_CLIENT_CONN) | S(CLNTC_E_BANK_CONN), .out_state_mask = S(CLNTC_ST_CONNECTED_CLIENT) | S(CLNTC_ST_WAIT_CONF_RES) | - S(CLNTC_ST_CONNECTED_BANKD), + S(CLNTC_ST_CONNECTED_BANKD) | S(CLNTC_ST_REJECTED), .action = clnt_st_established, }, [CLNTC_ST_WAIT_CONF_RES] = { @@ -493,6 +533,10 @@ .action = clnt_st_connected_bankd, .onenter = clnt_st_connected_bankd_onenter, }, + [CLNTC_ST_REJECTED] = { + .name = "REJECTED", + /* no events permitted, no action required */ + } }; -- To view, visit https://gerrit.osmocom.org/c/osmo-remsim/+/28027 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-remsim Gerrit-Branch: master Gerrit-Change-Id: I00387dbc19d689415470e2f08df08a47a78b81c0 Gerrit-Change-Number: 28027 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange