From 65006583c1c9f77444c4ca9f8107a4abae8ccdcc Mon Sep 17 00:00:00 2001 From: Harald Welte Date: Tue, 3 May 2022 16:22:37 +0200 Subject: [PATCH] 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 --- asn1/RSPRO.asn | 1 + include/osmocom/rspro/ResultCode.h | 1 + src/rspro/ResultCode.c | 12 +++--- src/server/rspro_server.c | 60 ++++++++++++++++++++++++++---- 4 files changed, 61 insertions(+), 13 deletions(-) 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 @@ ResultCode ::= ENUMERATED { 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 @@ typedef enum ResultCode { 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 @@ static const asn_INTEGER_enum_map_t asn_MAP_ResultCode_value2enum_1[] = { { 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 unsigned int asn_MAP_ResultCode_enum2value_1[] = { 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..7ce99c9 100644 --- a/src/server/rspro_server.c +++ b/src/server/rspro_server.c @@ -76,6 +76,7 @@ enum remsim_server_client_fsm_state { 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_init(struct osmo_fsm_inst *fi, uint32_t event, void *data) 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 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void * 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 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void * 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 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void * 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 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void * 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 @@ static void clnt_st_established(struct osmo_fsm_inst *fi, uint32_t event, void * "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_ConnectBankRes(&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 @@ static int server_client_fsm_timer_cb(struct osmo_fsm_inst *fi) 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 @@ static const struct osmo_fsm_state server_client_fsm_states[] = { .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 @@ static const struct osmo_fsm_state server_client_fsm_states[] = { .action = clnt_st_connected_bankd, .onenter = clnt_st_connected_bankd_onenter, }, + [CLNTC_ST_REJECTED] = { + .name = "REJECTED", + /* no events permitted, no action required */ + } };