From 366dfca7ffc4dd2100521cd021ffd549bb6f294f Mon Sep 17 00:00:00 2001 From: Andre Puschmann Date: Sun, 23 Jan 2022 11:27:55 +0100 Subject: [PATCH] gnb,rrc: refactor AS security updates refactor security updater to extra function and make sure the config is applied to all active RBs, also DRBs --- srsgnb/hdr/stack/rrc/rrc_nr_ue.h | 3 + srsgnb/src/stack/rrc/rrc_nr_ue.cc | 83 ++++++++++++++++-------- srsgnb/src/stack/rrc/test/rrc_nr_test.cc | 1 + 3 files changed, 59 insertions(+), 28 deletions(-) diff --git a/srsgnb/hdr/stack/rrc/rrc_nr_ue.h b/srsgnb/hdr/stack/rrc/rrc_nr_ue.h index dabfead70..4240ed82a 100644 --- a/srsgnb/hdr/stack/rrc/rrc_nr_ue.h +++ b/srsgnb/hdr/stack/rrc/rrc_nr_ue.h @@ -108,6 +108,9 @@ private: /// Update MAC based on ASN1 message int update_mac(const asn1::rrc_nr::cell_group_cfg_s& cell_group_config, bool is_config_complete); + /// Update AS security config on active RB + int update_as_security(uint32_t lcid, bool enable_integrity, bool enable_ciphering); + int pack_rrc_reconfiguration(asn1::dyn_octstring& packed_rrc_reconfig); int pack_secondary_cell_group_cfg(asn1::dyn_octstring& packed_secondary_cell_config); diff --git a/srsgnb/src/stack/rrc/rrc_nr_ue.cc b/srsgnb/src/stack/rrc/rrc_nr_ue.cc index ea8ad540b..8c411f15f 100644 --- a/srsgnb/src/stack/rrc/rrc_nr_ue.cc +++ b/srsgnb/src/stack/rrc/rrc_nr_ue.cc @@ -1108,20 +1108,8 @@ void rrc_nr::ue::handle_rrc_setup_complete(const asn1::rrc_nr::rrc_setup_complet /// TS 38.331, SecurityModeCommand void rrc_nr::ue::send_security_mode_command(srsran::unique_byte_buffer_t nas_pdu) { - // FIXME: Currently we are using the PDCP-LTE, so we need to convert from nr_as_security_cfg to as_security_config. - // When we start using PDCP-NR we can avoid this step. - srsran::nr_as_security_config_t tmp_cnfg = sec_ctx.get_as_sec_cfg(); - srsran::as_security_config_t pdcp_cnfg = {}; - pdcp_cnfg.k_rrc_int = tmp_cnfg.k_nr_rrc_int; - pdcp_cnfg.k_rrc_enc = tmp_cnfg.k_nr_rrc_enc; - pdcp_cnfg.k_up_int = tmp_cnfg.k_nr_up_int; - pdcp_cnfg.k_up_enc = tmp_cnfg.k_nr_up_enc; - pdcp_cnfg.integ_algo = (srsran::INTEGRITY_ALGORITHM_ID_ENUM)tmp_cnfg.integ_algo; - pdcp_cnfg.cipher_algo = (srsran::CIPHERING_ALGORITHM_ID_ENUM)tmp_cnfg.cipher_algo; - - // Setup SRB1 security/integrity. Encryption is set on completion - parent->pdcp->config_security(rnti, srb_to_lcid(srsran::nr_srb::srb1), pdcp_cnfg); - parent->pdcp->enable_integrity(rnti, srb_to_lcid(srsran::nr_srb::srb1)); + // apply selected security config and enable integrity on SRB1 before generating security mode command + update_as_security(srb_to_lcid(srsran::nr_srb::srb1), true, false); if (nas_pdu != nullptr) { nas_pdu_queue.push_back(std::move(nas_pdu)); @@ -1139,11 +1127,58 @@ void rrc_nr::ue::send_security_mode_command(srsran::unique_byte_buffer_t nas_pdu } } +/** + * @brief Internal helper to update the security configuration of a PDCP bearer + * + * If no valid AS security config is present (yet) the method doesn't modify the + * PDCP config and returns SRSRAN_ERROR. In some cases, however, + * for example during RRC Setup, this is in fact the expected behaviour as + * AS security isn't established yet. + * + * @param lcid Logical channel ID of the bearer + * @param enable_integrity Whether to enable integrity protection for the bearer + * @param enable_ciphering Whether to enable ciphering for the bearer + * @return int SRSRAN_SUCCESS if a valid AS security config was found and the security was configured + */ +int rrc_nr::ue::update_as_security(uint32_t lcid, bool enable_integrity = true, bool enable_ciphering = true) +{ + if (not sec_ctx.is_as_sec_cfg_valid()) { + parent->logger.error("Invalid AS security configuration. Skipping configuration for lcid=%d", lcid); + return SRSRAN_ERROR; + } + + // FIXME: Currently we are using the PDCP-LTE, so we need to convert from nr_as_security_cfg to as_security_config. + // When we start using PDCP-NR we can avoid this step. + srsran::nr_as_security_config_t tmp_cnfg = sec_ctx.get_as_sec_cfg(); + srsran::as_security_config_t pdcp_cnfg = {}; + pdcp_cnfg.k_rrc_int = tmp_cnfg.k_nr_rrc_int; + pdcp_cnfg.k_rrc_enc = tmp_cnfg.k_nr_rrc_enc; + pdcp_cnfg.k_up_int = tmp_cnfg.k_nr_up_int; + pdcp_cnfg.k_up_enc = tmp_cnfg.k_nr_up_enc; + pdcp_cnfg.integ_algo = (srsran::INTEGRITY_ALGORITHM_ID_ENUM)tmp_cnfg.integ_algo; + pdcp_cnfg.cipher_algo = (srsran::CIPHERING_ALGORITHM_ID_ENUM)tmp_cnfg.cipher_algo; + + // configure algorithm and keys + parent->pdcp->config_security(rnti, lcid, pdcp_cnfg); + + if (enable_integrity) { + parent->pdcp->enable_integrity(rnti, lcid); + } + + if (enable_ciphering) { + parent->pdcp->enable_encryption(rnti, lcid); + } + + return SRSRAN_SUCCESS; +} + /// TS 38.331, SecurityModeComplete void rrc_nr::ue::handle_security_mode_complete(const asn1::rrc_nr::security_mode_complete_s& msg) { parent->logger.info("SecurityModeComplete transaction ID: %d", msg.rrc_transaction_id); - parent->pdcp->enable_encryption(rnti, srb_to_lcid(srsran::nr_srb::srb1)); + + // finally, also enable ciphering on SRB1 + update_as_security(srb_to_lcid(srsran::nr_srb::srb1), false, true); send_rrc_reconfiguration(); // Note: Skip UE capabilities @@ -1357,20 +1392,8 @@ int rrc_nr::ue::update_pdcp_bearers(const asn1::rrc_nr::radio_bearer_cfg_s& radi } parent->pdcp->add_bearer(rnti, rlc_bearer->lc_ch_id, pdcp_cnfg); - // enable security config if (sec_ctx.is_as_sec_cfg_valid()) { - srsran::nr_as_security_config_t tmp_cnfg = sec_ctx.get_as_sec_cfg(); - srsran::as_security_config_t pdcp_cnfg = {}; - pdcp_cnfg.k_rrc_int = tmp_cnfg.k_nr_rrc_int; - pdcp_cnfg.k_rrc_enc = tmp_cnfg.k_nr_rrc_enc; - pdcp_cnfg.k_up_int = tmp_cnfg.k_nr_up_int; - pdcp_cnfg.k_up_enc = tmp_cnfg.k_nr_up_enc; - pdcp_cnfg.integ_algo = (srsran::INTEGRITY_ALGORITHM_ID_ENUM)tmp_cnfg.integ_algo; - pdcp_cnfg.cipher_algo = (srsran::CIPHERING_ALGORITHM_ID_ENUM)tmp_cnfg.cipher_algo; - - // Setup SRB1 security/integrity. Encryption is set on completion - parent->pdcp->config_security(rnti, srb_to_lcid(srsran::nr_srb::srb1), pdcp_cnfg); - parent->pdcp->enable_integrity(rnti, srb_to_lcid(srsran::nr_srb::srb1)); + update_as_security(rlc_bearer->lc_ch_id); } } @@ -1390,6 +1413,10 @@ int rrc_nr::ue::update_pdcp_bearers(const asn1::rrc_nr::radio_bearer_cfg_s& radi return SRSRAN_ERROR; } parent->pdcp->add_bearer(rnti, rlc_bearer->lc_ch_id, pdcp_cnfg); + + if (sec_ctx.is_as_sec_cfg_valid()) { + update_as_security(rlc_bearer->lc_ch_id); + } } return SRSRAN_SUCCESS; diff --git a/srsgnb/src/stack/rrc/test/rrc_nr_test.cc b/srsgnb/src/stack/rrc/test/rrc_nr_test.cc index d7a29392c..88b61800e 100644 --- a/srsgnb/src/stack/rrc/test/rrc_nr_test.cc +++ b/srsgnb/src/stack/rrc/test/rrc_nr_test.cc @@ -170,6 +170,7 @@ void test_rrc_sa_connection() SRSRAN_SUCCESS); TESTASSERT_SUCCESS(rrc_obj.add_user(0x4601, 0)); + TESTASSERT_SUCCESS(rrc_obj.ue_set_security_cfg_key(0x4601, {})); test_rrc_nr_connection_establishment(task_sched, rrc_obj, rlc_obj, mac_obj, ngap_obj, 0x4601); test_rrc_nr_info_transfer(task_sched, rrc_obj, pdcp_obj, ngap_obj, 0x4601);