Change in osmo-bsc[master]: fix inter-BSC-in handover encryption

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/27249 ) Change subject: fix inter-BSC-in handover encryption ...................................................................... fix inter-BSC-in handover encryption In the field we saw Handover Requests without any Chosen Encryption Algorithm IE, and osmo-bsc completely failed on those. This made me understand my mistake from when I wrote this handover code. So far, from a BSSMAP Handover Request, we (I) used only the Chosen Encryption Algorithm IE to pick the encryption to use on the target lchan. That is very wrong. Instead, figure out the intersection of permitted algorithms MSC & BSC, and pick the best of those. Which means, actually, completely ignore the Chosen Encryption Algorithm IE. In the message, the permitted algorithms are passed as a bitmask. The current code using gsm0808_dec_encrypt_info() passes this on as an array. In order to select_best_cipher(), I could convert that array back to a bitmask. Instead pass the bitmask on from message decoding alongside the struct gsm0808_encrypt_info in req->ei_as_bitmask. In handover_end(), change the condition so that we can also pass HO_RESULT_FAIL_RR_HO_FAIL to emit a Handover Failure. Related: SYS#5839 Change-Id: Iffedc981b60d309ed2e5decd5efedee07a757b53 --- M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c 3 files changed, 26 insertions(+), 8 deletions(-) Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h index 07d141d..4fe97a2 100644 --- a/include/osmocom/bsc/gsm_data.h +++ b/include/osmocom/bsc/gsm_data.h @@ -270,6 +270,8 @@ struct gsm0808_channel_type ct; struct gsm0808_speech_codec_list scl; struct gsm0808_encrypt_info ei; + /* The same information as in 'ei' but as the handy bitmask as on the wire. */ + uint8_t ei_as_bitmask; bool kc128_present; uint8_t kc128[16]; struct gsm_classmark classmark; @@ -1457,4 +1459,6 @@ enum rsl_cmod_spd chan_mode_to_rsl_cmod_spd(enum gsm48_chan_mode chan_mode); +int select_best_cipher(uint8_t msc_mask, uint8_t bsc_mask); + #endif /* _GSM_DATA_H */ diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c index 37e7417..487135b 100644 --- a/src/osmo-bsc/handover_fsm.c +++ b/src/osmo-bsc/handover_fsm.c @@ -490,6 +490,7 @@ LOG_HO(conn, LOGL_ERROR, "Failed to parse Encryption Information IE\n"); return false; } + req->ei_as_bitmask = *e->val; if ((e = TLVP_GET(tp, GSM0808_IE_KC_128))) { if (e->len != 16) { @@ -630,6 +631,7 @@ int match_idx; struct osmo_fsm_inst *fi; struct channel_mode_and_rate ch_mode_rate = {}; + int chosen_a5_n; handover_fsm_alloc(conn); @@ -717,16 +719,28 @@ .msc_assigned_cic = req->msc_assigned_cic, }; - if (req->chosen_encr_alg) { - info.encr.alg_id = req->chosen_encr_alg; - if (info.encr.alg_id > 1 && !req->ei.key_len) { - ho_fail(HO_RESULT_ERROR, "Chosen Encryption Algorithm (Serving) reflects A5/%u" - " but there is no key (Encryption Information)", info.encr.alg_id - 1); + /* Figure out the encryption algorithm */ + chosen_a5_n = select_best_cipher(req->ei_as_bitmask, bsc_gsmnet->a5_encryption_mask); + if (chosen_a5_n < 0) { + ho_fail(HO_RESULT_FAIL_RR_HO_FAIL, + "There is no A5 encryption mode that both BSC and MSC permit: MSC 0x%x & BSC 0x%x = 0\n", + req->ei_as_bitmask, bsc_gsmnet->a5_encryption_mask); + return; + } + if (chosen_a5_n > 0 && !req->ei.key_len) { + /* There is no key. Is A5/0 permitted? */ + if ((req->ei_as_bitmask & bsc_gsmnet->a5_encryption_mask & 0x1) == 0x1) { + chosen_a5_n = 0; + } else { + ho_fail(HO_RESULT_ERROR, + "Encryption is required, but there is no key (Encryption Information)"); return; } } - if (req->ei.key_len) { + /* Put encryption info in the chan activation info */ + info.encr.alg_id = ALG_A5_NR_TO_RSL(chosen_a5_n); + if (chosen_a5_n > 0) { if (req->ei.key_len > sizeof(info.encr.key)) { ho_fail(HO_RESULT_ERROR, "Encryption Information IE key length is too large: %u\n", req->ei.key_len); @@ -965,7 +979,7 @@ result = bsc_tx_bssmap_ho_complete(conn, ho->new_lchan); } /* Not 'else': above checks may still result in HO_RESULT_ERROR. */ - if (result == HO_RESULT_ERROR) { + if (result != HO_RESULT_OK) { /* Return a BSSMAP Handover Failure, as described in 3GPP TS 48.008 3.1.5.2.2 * "Handover Resource Allocation Failure" */ bsc_tx_bssmap_ho_failure(conn); diff --git a/src/osmo-bsc/osmo_bsc_bssap.c b/src/osmo-bsc/osmo_bsc_bssap.c index b18627c..8acf293 100644 --- a/src/osmo-bsc/osmo_bsc_bssap.c +++ b/src/osmo-bsc/osmo_bsc_bssap.c @@ -394,7 +394,7 @@ } /* select the best cipher permitted by the intersection of both masks */ -static int select_best_cipher(uint8_t msc_mask, uint8_t bsc_mask) +int select_best_cipher(uint8_t msc_mask, uint8_t bsc_mask) { /* A5/7 ... A5/3: We assume higher is better, * but: A5/1 is better than A5/2, which is better than A5/0 */ 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/27249 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Iffedc981b60d309ed2e5decd5efedee07a757b53 Gerrit-Change-Number: 27249 Gerrit-PatchSet: 4 Gerrit-Owner: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged