From 315b7edbc8281c822cbdf3c844e4924ee1ca4d33 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Thu, 9 Feb 2023 19:51:04 +0530 Subject: [PATCH 1/4] ACL: Add support for NOC CAT --- matter/src/acl.rs | 185 +++++++++++++++++++++++++++++++--- matter/src/data_model/core.rs | 23 +++-- 2 files changed, 189 insertions(+), 19 deletions(-) diff --git a/matter/src/acl.rs b/matter/src/acl.rs index d46d46d..2260062 100644 --- a/matter/src/acl.rs +++ b/matter/src/acl.rs @@ -15,7 +15,10 @@ * limitations under the License. */ -use std::sync::{Arc, Mutex, MutexGuard, RwLock}; +use std::{ + fmt::Display, + sync::{Arc, Mutex, MutexGuard, RwLock}, +}; use crate::{ data_model::objects::{Access, Privilege}, @@ -24,6 +27,7 @@ use crate::{ interaction_model::messages::GenericPath, sys::Psm, tlv::{FromTLV, TLVElement, TLVList, TLVWriter, TagType, ToTLV}, + transport::session::MAX_CAT_IDS_PER_NOC, utils::writebuf::WriteBuf, }; use log::error; @@ -67,12 +71,94 @@ impl ToTLV for AuthMode { } } +/// An accessor can have as many identities: one node id and Upto MAX_CAT_IDS_PER_NOC +const MAX_ACCESSOR_SUBJECTS: usize = 1 + MAX_CAT_IDS_PER_NOC; +/// The CAT Prefix used in Subjects +pub const NOC_CAT_SUBJECT_PREFIX: u64 = 0xFFFF_FFFD_0000_0000; +const NOC_CAT_ID_MASK: u64 = 0xFFFF_0000; +const NOC_CAT_VERSION_MASK: u64 = 0xFFFF; + +fn is_noc_cat(id: u64) -> bool { + (id & NOC_CAT_SUBJECT_PREFIX) == NOC_CAT_SUBJECT_PREFIX +} + +fn get_noc_cat_id(id: u64) -> u64 { + (id & NOC_CAT_ID_MASK) >> 16 +} + +fn get_noc_cat_version(id: u64) -> u64 { + id & NOC_CAT_VERSION_MASK +} + +pub fn gen_noc_cat(id: u16, version: u16) -> u64 { + NOC_CAT_SUBJECT_PREFIX | ((id as u64) << 16) | version as u64 +} + +pub struct AccessorSubjects([u64; MAX_ACCESSOR_SUBJECTS]); + +impl AccessorSubjects { + pub fn new(id: u64) -> Self { + let mut a = Self(Default::default()); + a.0[0] = id; + a + } + + pub fn add(&mut self, subject: u64) -> Result<(), Error> { + for (i, val) in self.0.iter().enumerate() { + if *val == 0 { + self.0[i] = subject; + return Ok(()); + } + } + Err(Error::NoSpace) + } + + /// Match the match_subject with any of the current subjects + /// If a NOC CAT is specified, CAT aware matching is also performed + pub fn matches(&self, acl_subject: u64) -> bool { + for v in self.0.iter() { + if *v == 0 { + continue; + } + + if *v == acl_subject { + return true; + } else { + // NOC CAT match + if is_noc_cat(*v) + && is_noc_cat(acl_subject) + && (get_noc_cat_id(*v) == get_noc_cat_id(acl_subject)) + && (get_noc_cat_version(*v) >= get_noc_cat_version(acl_subject)) + { + return true; + } + } + } + + false + } +} + +impl Display for AccessorSubjects { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error> { + write!(f, "[")?; + for i in self.0 { + if is_noc_cat(i) { + write!(f, "CAT({} - {})", get_noc_cat_id(i), get_noc_cat_version(i))?; + } else if i != 0 { + write!(f, "{}, ", i)?; + } + } + write!(f, "]") + } +} + /// The Accessor Object pub struct Accessor { /// The fabric index of the accessor pub fab_idx: u8, - /// Accessor's identified: could be node-id, NoC CAT, group id - id: u64, + /// Accessor's subject: could be node-id, NoC CAT, group id + subjects: AccessorSubjects, /// The Authmode of this session auth_mode: AuthMode, // TODO: Is this the right place for this though, or should we just use a global-acl-handle-get @@ -80,10 +166,15 @@ pub struct Accessor { } impl Accessor { - pub fn new(fab_idx: u8, id: u64, auth_mode: AuthMode, acl_mgr: Arc) -> Self { + pub fn new( + fab_idx: u8, + subjects: AccessorSubjects, + auth_mode: AuthMode, + acl_mgr: Arc, + ) -> Self { Self { fab_idx, - id, + subjects, auth_mode, acl_mgr, } @@ -215,7 +306,7 @@ impl AclEntry { let mut entries_exist = false; for i in self.subjects.iter().flatten() { entries_exist = true; - if accessor.id == *i { + if accessor.subjects.matches(*i) { allow = true; } } @@ -467,8 +558,8 @@ impl AclMgr { } } error!( - "ACL Disallow for src id {} fab idx {}", - req.accessor.id, req.accessor.fab_idx + "ACL Disallow for subjects {} fab idx {}", + req.accessor.subjects, req.accessor.fab_idx ); error!("{}", self); false @@ -490,6 +581,7 @@ impl std::fmt::Display for AclMgr { #[allow(clippy::bool_assert_comparison)] mod tests { use crate::{ + acl::{gen_noc_cat, AccessorSubjects}, data_model::objects::{Access, Privilege}, interaction_model::messages::GenericPath, }; @@ -501,7 +593,7 @@ mod tests { fn test_basic_empty_subject_target() { let am = Arc::new(AclMgr::new_with(false).unwrap()); am.erase_all(); - let accessor = Accessor::new(2, 112233, AuthMode::Case, am.clone()); + let accessor = Accessor::new(2, AccessorSubjects::new(112233), AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); let mut req = AccessReq::new(&accessor, &path, Access::READ); req.set_target_perms(Access::RWVA); @@ -529,7 +621,7 @@ mod tests { fn test_subject() { let am = Arc::new(AclMgr::new_with(false).unwrap()); am.erase_all(); - let accessor = Accessor::new(2, 112233, AuthMode::Case, am.clone()); + let accessor = Accessor::new(2, AccessorSubjects::new(112233), AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); let mut req = AccessReq::new(&accessor, &path, Access::READ); req.set_target_perms(Access::RWVA); @@ -547,11 +639,79 @@ mod tests { assert_eq!(req.allow(), true); } + #[test] + fn test_cat() { + let am = Arc::new(AclMgr::new_with(false).unwrap()); + am.erase_all(); + + let allow_cat = 0xABCD; + let disallow_cat = 0xCAFE; + let v2 = 2; + let v3 = 3; + // Accessor has nodeif and CAT 0xABCD_0002 + let mut subjects = AccessorSubjects::new(112233); + subjects.add(gen_noc_cat(allow_cat, v2)).unwrap(); + + let accessor = Accessor::new(2, subjects, AuthMode::Case, am.clone()); + let path = GenericPath::new(Some(1), Some(1234), None); + let mut req = AccessReq::new(&accessor, &path, Access::READ); + req.set_target_perms(Access::RWVA); + + // Deny for CAT id mismatch + let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); + new.add_subject(gen_noc_cat(disallow_cat, v2)).unwrap(); + am.add(new).unwrap(); + assert_eq!(req.allow(), false); + + // Deny of CAT version mismatch + let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); + new.add_subject(gen_noc_cat(allow_cat, v3)).unwrap(); + am.add(new).unwrap(); + assert_eq!(req.allow(), false); + + // Allow for CAT match + let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); + new.add_subject(gen_noc_cat(allow_cat, v2)).unwrap(); + am.add(new).unwrap(); + assert_eq!(req.allow(), true); + } + + #[test] + fn test_cat_version() { + let am = Arc::new(AclMgr::new_with(false).unwrap()); + am.erase_all(); + + let allow_cat = 0xABCD; + let disallow_cat = 0xCAFE; + let v2 = 2; + let v3 = 3; + // Accessor has nodeif and CAT 0xABCD_0003 + let mut subjects = AccessorSubjects::new(112233); + subjects.add(gen_noc_cat(allow_cat, v3)).unwrap(); + + let accessor = Accessor::new(2, subjects, AuthMode::Case, am.clone()); + let path = GenericPath::new(Some(1), Some(1234), None); + let mut req = AccessReq::new(&accessor, &path, Access::READ); + req.set_target_perms(Access::RWVA); + + // Deny for CAT id mismatch + let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); + new.add_subject(gen_noc_cat(disallow_cat, v2)).unwrap(); + am.add(new).unwrap(); + assert_eq!(req.allow(), false); + + // Allow for CAT match and version more than ACL version + let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); + new.add_subject(gen_noc_cat(allow_cat, v2)).unwrap(); + am.add(new).unwrap(); + assert_eq!(req.allow(), true); + } + #[test] fn test_target() { let am = Arc::new(AclMgr::new_with(false).unwrap()); am.erase_all(); - let accessor = Accessor::new(2, 112233, AuthMode::Case, am.clone()); + let accessor = Accessor::new(2, AccessorSubjects::new(112233), AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); let mut req = AccessReq::new(&accessor, &path, Access::READ); req.set_target_perms(Access::RWVA); @@ -612,7 +772,8 @@ mod tests { fn test_privilege() { let am = Arc::new(AclMgr::new_with(false).unwrap()); am.erase_all(); - let accessor = Accessor::new(2, 112233, AuthMode::Case, am.clone()); + + let accessor = Accessor::new(2, AccessorSubjects::new(112233), AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); // Create an Exact Match ACL with View privilege diff --git a/matter/src/data_model/core.rs b/matter/src/data_model/core.rs index 8cddaf4..d2f4059 100644 --- a/matter/src/data_model/core.rs +++ b/matter/src/data_model/core.rs @@ -23,7 +23,7 @@ use super::{ system_model::descriptor::DescriptorCluster, }; use crate::{ - acl::{AccessReq, Accessor, AclMgr, AuthMode}, + acl::{AccessReq, Accessor, AccessorSubjects, AclMgr, AuthMode}, error::*, fabric::FabricMgr, interaction_model::{ @@ -219,14 +219,23 @@ impl DataModel { fn sess_to_accessor(&self, sess: &Session) -> Accessor { match sess.get_session_mode() { - SessionMode::Case(c) => Accessor::new( - c, - sess.get_peer_node_id().unwrap_or_default(), - AuthMode::Case, + SessionMode::Case(c) => { + let subject = AccessorSubjects::new(sess.get_peer_node_id().unwrap_or_default()); + Accessor::new(c, subject, AuthMode::Case, self.acl_mgr.clone()) + } + SessionMode::Pase => Accessor::new( + 0, + AccessorSubjects::new(1), + AuthMode::Pase, + self.acl_mgr.clone(), + ), + + SessionMode::PlainText => Accessor::new( + 0, + AccessorSubjects::new(1), + AuthMode::Invalid, self.acl_mgr.clone(), ), - SessionMode::Pase => Accessor::new(0, 1, AuthMode::Pase, self.acl_mgr.clone()), - SessionMode::PlainText => Accessor::new(0, 1, AuthMode::Invalid, self.acl_mgr.clone()), } } From 725d19187e882f4df09cd61919e06534cfd97057 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Thu, 9 Feb 2023 15:45:25 +0530 Subject: [PATCH 2/4] Session: Include NoC CAT fields, and populate them from the CASE exchange --- matter/src/cert/mod.rs | 16 +++++++++++++++ matter/src/data_model/core.rs | 2 +- matter/src/data_model/sdm/failsafe.rs | 13 ++++++++---- matter/src/data_model/sdm/noc.rs | 8 ++++---- matter/src/secure_channel/case.rs | 11 ++++++++-- matter/src/transport/session.rs | 29 +++++++++++++++++++++++++-- matter/tests/common/im_engine.rs | 4 ++-- 7 files changed, 68 insertions(+), 15 deletions(-) diff --git a/matter/src/cert/mod.rs b/matter/src/cert/mod.rs index b3f9338..93e5e40 100644 --- a/matter/src/cert/mod.rs +++ b/matter/src/cert/mod.rs @@ -318,6 +318,18 @@ impl DistNames { } }) } + + fn u64_arr(&self, match_id: DnTags, output: &mut [u64]) { + let mut out_index = 0; + for (_, val) in self.dn.iter().filter(|(id, _)| *id == match_id as u8) { + if let DistNameValue::Uint(a) = val { + if out_index < output.len() { + output[out_index] = *a; + out_index += 1; + } + } + } + } } const PRINTABLE_STR_THRESHOLD: u8 = 0x80; @@ -543,6 +555,10 @@ impl Cert { self.subject.u64(DnTags::NodeId).ok_or(Error::NoNodeId) } + pub fn get_cat_ids(&self, output: &mut [u64]) { + self.subject.u64_arr(DnTags::NocCat, output) + } + pub fn get_fabric_id(&self) -> Result { self.subject.u64(DnTags::FabricId).ok_or(Error::NoFabricId) } diff --git a/matter/src/data_model/core.rs b/matter/src/data_model/core.rs index d2f4059..49ca031 100644 --- a/matter/src/data_model/core.rs +++ b/matter/src/data_model/core.rs @@ -221,7 +221,7 @@ impl DataModel { match sess.get_session_mode() { SessionMode::Case(c) => { let subject = AccessorSubjects::new(sess.get_peer_node_id().unwrap_or_default()); - Accessor::new(c, subject, AuthMode::Case, self.acl_mgr.clone()) + Accessor::new(c.fab_idx, subject, AuthMode::Case, self.acl_mgr.clone()) } SessionMode::Pase => Accessor::new( 0, diff --git a/matter/src/data_model/sdm/failsafe.rs b/matter/src/data_model/sdm/failsafe.rs index 12a5d11..cd3c2be 100644 --- a/matter/src/data_model/sdm/failsafe.rs +++ b/matter/src/data_model/sdm/failsafe.rs @@ -89,10 +89,15 @@ impl FailSafe { match c.noc_state { NocState::NocNotRecvd => return Err(Error::Invalid), NocState::AddNocRecvd(idx) | NocState::UpdateNocRecvd(idx) => { - if SessionMode::Case(idx) != session_mode { - error!( - "Received disarm in separate session from previous Add/Update NOC" - ); + if let SessionMode::Case(c) = session_mode { + if c.fab_idx != idx { + error!( + "Received disarm in separate session from previous Add/Update NOC" + ); + return Err(Error::Invalid); + } + } else { + error!("Received disarm in a non-CASE session"); return Err(Error::Invalid); } } diff --git a/matter/src/data_model/sdm/noc.rs b/matter/src/data_model/sdm/noc.rs index 2d3beeb..f6bb99f 100644 --- a/matter/src/data_model/sdm/noc.rs +++ b/matter/src/data_model/sdm/noc.rs @@ -248,11 +248,11 @@ impl NocCluster { .map_err(|_| IMStatusCode::InvalidDataType)?; let (result, fab_idx) = - if let SessionMode::Case(fab_idx) = cmd_req.trans.session.get_session_mode() { - if self.fabric_mgr.set_label(fab_idx, label).is_err() { - (NocStatus::LabelConflict, fab_idx) + if let SessionMode::Case(c) = cmd_req.trans.session.get_session_mode() { + if self.fabric_mgr.set_label(c.fab_idx, label).is_err() { + (NocStatus::LabelConflict, c.fab_idx) } else { - (NocStatus::Ok, fab_idx) + (NocStatus::Ok, c.fab_idx) } } else { // Update Fabric Label not allowed diff --git a/matter/src/secure_channel/case.rs b/matter/src/secure_channel/case.rs index 33c8e47..58a5593 100644 --- a/matter/src/secure_channel/case.rs +++ b/matter/src/secure_channel/case.rs @@ -33,7 +33,7 @@ use crate::{ network::Address, proto_demux::{ProtoCtx, ResponseRequired}, queue::{Msg, WorkQ}, - session::{CloneData, SessionMode}, + session::{CaseDetails, CloneData, NocCatIds, SessionMode}, }, utils::writebuf::WriteBuf, }; @@ -155,6 +155,8 @@ impl Case { } // Only now do we add this message to the TT Hash + let mut peer_catids: NocCatIds = Default::default(); + initiator_noc.get_cat_ids(&mut peer_catids); case_session.tt_hash.update(ctx.rx.as_borrow_slice())?; let clone_data = Case::get_session_clone_data( fabric.ipk.op_key(), @@ -162,6 +164,7 @@ impl Case { initiator_noc.get_node_id()?, ctx.exch_ctx.sess.get_peer_addr(), &case_session, + &peer_catids, )?; // Queue a transport mgr request to add a new session WorkQ::get()?.sync_send(Msg::NewSession(clone_data))?; @@ -281,6 +284,7 @@ impl Case { peer_nodeid: u64, peer_addr: Address, case_session: &CaseSession, + peer_catids: &NocCatIds, ) -> Result { let mut session_keys = [0_u8; 3 * crypto::SYMM_KEY_LEN_BYTES]; Case::get_session_keys( @@ -296,7 +300,10 @@ impl Case { case_session.peer_sessid, case_session.local_sessid, peer_addr, - SessionMode::Case(case_session.local_fabric_idx as u8), + SessionMode::Case(CaseDetails::new( + case_session.local_fabric_idx as u8, + peer_catids, + )), ); clone_data.dec_key.copy_from_slice(&session_keys[0..16]); diff --git a/matter/src/transport/session.rs b/matter/src/transport/session.rs index db0c845..da8d7cd 100644 --- a/matter/src/transport/session.rs +++ b/matter/src/transport/session.rs @@ -37,12 +37,28 @@ use super::{ packet::{Packet, PacketPool}, }; +pub const MAX_CAT_IDS_PER_NOC: usize = 3; const MATTER_AES128_KEY_SIZE: usize = 16; +#[derive(Debug, Default, Copy, Clone, PartialEq)] +pub struct CaseDetails { + pub fab_idx: u8, + pub cat_ids: [u64; MAX_CAT_IDS_PER_NOC], +} + +impl CaseDetails { + pub fn new(fab_idx: u8, cat_ids: &[u64; MAX_CAT_IDS_PER_NOC]) -> Self { + Self { + fab_idx, + cat_ids: *cat_ids, + } + } +} + #[derive(Debug, PartialEq, Copy, Clone)] pub enum SessionMode { // The Case session will capture the local fabric index - Case(u8), + Case(CaseDetails), Pase, PlainText, } @@ -53,6 +69,8 @@ impl Default for SessionMode { } } +pub type NocCatIds = [u64; MAX_CAT_IDS_PER_NOC]; + #[derive(Debug)] pub struct Session { peer_addr: Address, @@ -188,9 +206,16 @@ impl Session { self.peer_nodeid } + pub fn get_peer_cat_ids(&self) -> Option<&NocCatIds> { + match &self.mode { + SessionMode::Case(a) => Some(&a.cat_ids), + _ => None, + } + } + pub fn get_local_fabric_idx(&self) -> Option { match self.mode { - SessionMode::Case(a) => Some(a), + SessionMode::Case(a) => Some(a.fab_idx), _ => None, } } diff --git a/matter/tests/common/im_engine.rs b/matter/tests/common/im_engine.rs index 561c014..29e1c76 100644 --- a/matter/tests/common/im_engine.rs +++ b/matter/tests/common/im_engine.rs @@ -32,7 +32,6 @@ use matter::{ secure_channel::pake::PaseMgr, tlv::{TLVWriter, TagType, ToTLV}, transport::packet::Packet, - transport::proto_demux::HandleProto, transport::{ exchange::{self, Exchange, ExchangeCtx}, network::Address, @@ -40,6 +39,7 @@ use matter::{ proto_demux::ProtoCtx, session::{CloneData, SessionMgr, SessionMode}, }, + transport::{proto_demux::HandleProto, session::CaseDetails}, utils::writebuf::WriteBuf, }; use std::{ @@ -146,7 +146,7 @@ impl ImEngine { std::net::IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 5542, )), - SessionMode::Case(1), + SessionMode::Case(CaseDetails::new(1, &Default::default())), ); let sess_idx = sess_mgr.clone_session(&clone_data).unwrap(); let sess = sess_mgr.get_session_handle(sess_idx); From 0149d30a0cc09c0ac1d74d8aa296d73a2db70c55 Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Fri, 10 Feb 2023 20:22:28 +0530 Subject: [PATCH 3/4] CASE: Extract CAT IDs from current session and add them to the Accessor --- matter/src/data_model/core.rs | 8 ++- matter/tests/common/im_engine.rs | 10 ++- matter/tests/data_model/acl_and_dataver.rs | 75 ++++++++++++++++++++-- 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/matter/src/data_model/core.rs b/matter/src/data_model/core.rs index 49ca031..7ea9659 100644 --- a/matter/src/data_model/core.rs +++ b/matter/src/data_model/core.rs @@ -220,7 +220,13 @@ impl DataModel { fn sess_to_accessor(&self, sess: &Session) -> Accessor { match sess.get_session_mode() { SessionMode::Case(c) => { - let subject = AccessorSubjects::new(sess.get_peer_node_id().unwrap_or_default()); + let mut subject = + AccessorSubjects::new(sess.get_peer_node_id().unwrap_or_default()); + for i in c.cat_ids { + if i != 0 { + let _ = subject.add(i); + } + } Accessor::new(c.fab_idx, subject, AuthMode::Case, self.acl_mgr.clone()) } SessionMode::Pase => Accessor::new( diff --git a/matter/tests/common/im_engine.rs b/matter/tests/common/im_engine.rs index 29e1c76..f91433c 100644 --- a/matter/tests/common/im_engine.rs +++ b/matter/tests/common/im_engine.rs @@ -37,7 +37,7 @@ use matter::{ network::Address, packet::PacketPool, proto_demux::ProtoCtx, - session::{CloneData, SessionMgr, SessionMode}, + session::{CloneData, NocCatIds, SessionMgr, SessionMode}, }, transport::{proto_demux::HandleProto, session::CaseDetails}, utils::writebuf::WriteBuf, @@ -69,6 +69,7 @@ pub struct ImInput<'a> { action: OpCode, data: &'a dyn ToTLV, peer_id: u64, + cat_ids: NocCatIds, } pub const IM_ENGINE_PEER_ID: u64 = 445566; @@ -78,12 +79,17 @@ impl<'a> ImInput<'a> { action, data, peer_id: IM_ENGINE_PEER_ID, + cat_ids: Default::default(), } } pub fn set_peer_node_id(&mut self, peer: u64) { self.peer_id = peer; } + + pub fn set_cat_ids(&mut self, cat_ids: &NocCatIds) { + self.cat_ids = *cat_ids; + } } impl ImEngine { @@ -146,7 +152,7 @@ impl ImEngine { std::net::IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 5542, )), - SessionMode::Case(CaseDetails::new(1, &Default::default())), + SessionMode::Case(CaseDetails::new(1, &input.cat_ids)), ); let sess_idx = sess_mgr.clone_session(&clone_data).unwrap(); let sess = sess_mgr.get_session_handle(sess_idx); diff --git a/matter/tests/data_model/acl_and_dataver.rs b/matter/tests/data_model/acl_and_dataver.rs index 3dd2f60..d692c6a 100644 --- a/matter/tests/data_model/acl_and_dataver.rs +++ b/matter/tests/data_model/acl_and_dataver.rs @@ -16,7 +16,7 @@ */ use matter::{ - acl::{AclEntry, AuthMode, Target}, + acl::{gen_noc_cat, AclEntry, AuthMode, Target}, data_model::{ objects::{AttrValue, EncodeValue, Privilege}, system_model::access_control, @@ -30,6 +30,7 @@ use matter::{ messages::{msg, GenericPath}, }, tlv::{self, ElementType, FromTLV, TLVArray, TLVElement, TLVWriter, TagType}, + transport::session::NocCatIds, }; use crate::{ @@ -77,6 +78,7 @@ fn gen_read_reqs_output<'a>( fn handle_write_reqs( im: &mut ImEngine, peer_node_id: u64, + peer_cat_ids: Option<&NocCatIds>, input: &[AttrData], expected: &[AttrStatus], ) { @@ -85,6 +87,9 @@ fn handle_write_reqs( let mut input = ImInput::new(OpCode::WriteRequest, &write_req); input.set_peer_node_id(peer_node_id); + if let Some(cat_ids) = peer_cat_ids { + input.set_cat_ids(cat_ids); + } let (_, out_buf) = im.process(&input, &mut out_buf); tlv::print_tlv_list(out_buf); @@ -263,7 +268,7 @@ fn wc_write_attribute() { // Test 1: Wildcard write to an attribute without permission should return // no error - handle_write_reqs(&mut im, peer, input0, &[]); + handle_write_reqs(&mut im, peer, None, input0, &[]); { let node = im.dm.node.read().unwrap(); let echo = node.get_cluster(0, echo_cluster::ID).unwrap(); @@ -287,6 +292,7 @@ fn wc_write_attribute() { handle_write_reqs( &mut im, peer, + None, input0, &[AttrStatus::new(&ep0_att, IMStatusCode::Sucess, 0)], ); @@ -307,6 +313,7 @@ fn wc_write_attribute() { handle_write_reqs( &mut im, peer, + None, input1, &[ AttrStatus::new(&ep0_att, IMStatusCode::Sucess, 0), @@ -350,7 +357,7 @@ fn exact_write_attribute() { // Test 1: Exact write to an attribute without permission should return // Unsupported Access Error - handle_write_reqs(&mut im, peer, input, expected_fail); + handle_write_reqs(&mut im, peer, None, input, expected_fail); assert_eq!( AttrValue::Uint16(ATTR_WRITE_DEFAULT_VALUE), read_cluster_id_write_attr(&im, 0) @@ -363,7 +370,62 @@ fn exact_write_attribute() { // Test 1: Exact write to an attribute with permission should grant // access - handle_write_reqs(&mut im, peer, input, expected_success); + handle_write_reqs(&mut im, peer, None, input, expected_success); + assert_eq!(AttrValue::Uint16(val0), read_cluster_id_write_attr(&im, 0)); +} + +#[test] +/// Ensure that an write attribute without a wildcard returns an error when the +/// ACL disallows the access, and returns success once access is granted to the CAT ID +/// The Accessor CAT version is one more than that in the ACL +fn exact_write_attribute_noc_cat() { + let _ = env_logger::try_init(); + let val0 = 10; + let attr_data0 = |tag, t: &mut TLVWriter| { + let _ = t.u16(tag, val0); + }; + + let ep0_att = GenericPath::new( + Some(0), + Some(echo_cluster::ID), + Some(echo_cluster::Attributes::AttWrite as u32), + ); + + let input = &[AttrData::new( + None, + AttrPath::new(&ep0_att), + EncodeValue::Closure(&attr_data0), + )]; + let expected_fail = &[AttrStatus::new( + &ep0_att, + IMStatusCode::UnsupportedAccess, + 0, + )]; + let expected_success = &[AttrStatus::new(&ep0_att, IMStatusCode::Sucess, 0)]; + + let peer = 98765; + /* CAT in NOC is 1 more, in version, than that in ACL */ + let noc_cat = gen_noc_cat(0xABCD, 2); + let cat_in_acl = gen_noc_cat(0xABCD, 1); + let cat_ids = [noc_cat, 0, 0]; + let mut im = ImEngine::new(); + + // Test 1: Exact write to an attribute without permission should return + // Unsupported Access Error + handle_write_reqs(&mut im, peer, Some(&cat_ids), input, expected_fail); + assert_eq!( + AttrValue::Uint16(ATTR_WRITE_DEFAULT_VALUE), + read_cluster_id_write_attr(&im, 0) + ); + + // Add ACL to allow our peer to access any endpoint + let mut acl = AclEntry::new(1, Privilege::ADMIN, AuthMode::Case); + acl.add_subject(cat_in_acl).unwrap(); + im.acl_mgr.add(acl).unwrap(); + + // Test 1: Exact write to an attribute with permission should grant + // access + handle_write_reqs(&mut im, peer, Some(&cat_ids), input, expected_success); assert_eq!(AttrValue::Uint16(val0), read_cluster_id_write_attr(&im, 0)); } @@ -399,6 +461,7 @@ fn insufficient_perms_write() { handle_write_reqs( &mut im, peer, + None, input0, &[AttrStatus::new( &ep0_att, @@ -466,6 +529,7 @@ fn write_with_runtime_acl_add() { handle_write_reqs( &mut im, peer, + None, // write to echo-cluster attribute, write to acl attribute, write to echo-cluster attribute &[input0, acl_input, input0], &[ @@ -623,6 +687,7 @@ fn test_write_data_ver() { handle_write_reqs( &mut im, peer, + None, input_correct_dataver, &[AttrStatus::new(&ep0_attwrite, IMStatusCode::Sucess, 0)], ); @@ -638,6 +703,7 @@ fn test_write_data_ver() { handle_write_reqs( &mut im, peer, + None, input_correct_dataver, &[AttrStatus::new( &ep0_attwrite, @@ -660,6 +726,7 @@ fn test_write_data_ver() { handle_write_reqs( &mut im, peer, + None, input_correct_dataver, &[AttrStatus::new(&ep0_attwrite, IMStatusCode::Sucess, 0)], ); From 707d3700c0271b09cfa279b33e3953898b1b25fa Mon Sep 17 00:00:00 2001 From: Kedar Sovani Date: Sun, 12 Feb 2023 10:14:47 +0530 Subject: [PATCH 4/4] NOC CAT: The CAT IDs in the NoC are only 32-bit, account for that --- matter/src/acl.rs | 30 ++++++++++++++-------- matter/src/cert/mod.rs | 9 ++++--- matter/src/data_model/core.rs | 2 +- matter/src/transport/session.rs | 8 +++--- matter/tests/data_model/acl_and_dataver.rs | 2 +- 5 files changed, 30 insertions(+), 21 deletions(-) diff --git a/matter/src/acl.rs b/matter/src/acl.rs index 2260062..8625a7b 100644 --- a/matter/src/acl.rs +++ b/matter/src/acl.rs @@ -90,8 +90,10 @@ fn get_noc_cat_version(id: u64) -> u64 { id & NOC_CAT_VERSION_MASK } -pub fn gen_noc_cat(id: u16, version: u16) -> u64 { - NOC_CAT_SUBJECT_PREFIX | ((id as u64) << 16) | version as u64 +/// Generate CAT that is embeddedable in the NoC +/// This only generates the 32-bit CAT ID +pub fn gen_noc_cat(id: u16, version: u16) -> u32 { + ((id as u32) << 16) | version as u32 } pub struct AccessorSubjects([u64; MAX_ACCESSOR_SUBJECTS]); @@ -103,10 +105,10 @@ impl AccessorSubjects { a } - pub fn add(&mut self, subject: u64) -> Result<(), Error> { + pub fn add_catid(&mut self, subject: u32) -> Result<(), Error> { for (i, val) in self.0.iter().enumerate() { if *val == 0 { - self.0[i] = subject; + self.0[i] = NOC_CAT_SUBJECT_PREFIX | (subject as u64); return Ok(()); } } @@ -287,6 +289,10 @@ impl AclEntry { Ok(()) } + pub fn add_subject_catid(&mut self, cat_id: u32) -> Result<(), Error> { + self.add_subject(NOC_CAT_SUBJECT_PREFIX | cat_id as u64) + } + pub fn add_target(&mut self, target: Target) -> Result<(), Error> { let index = self .targets @@ -650,7 +656,7 @@ mod tests { let v3 = 3; // Accessor has nodeif and CAT 0xABCD_0002 let mut subjects = AccessorSubjects::new(112233); - subjects.add(gen_noc_cat(allow_cat, v2)).unwrap(); + subjects.add_catid(gen_noc_cat(allow_cat, v2)).unwrap(); let accessor = Accessor::new(2, subjects, AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); @@ -659,19 +665,20 @@ mod tests { // Deny for CAT id mismatch let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); - new.add_subject(gen_noc_cat(disallow_cat, v2)).unwrap(); + new.add_subject_catid(gen_noc_cat(disallow_cat, v2)) + .unwrap(); am.add(new).unwrap(); assert_eq!(req.allow(), false); // Deny of CAT version mismatch let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); - new.add_subject(gen_noc_cat(allow_cat, v3)).unwrap(); + new.add_subject_catid(gen_noc_cat(allow_cat, v3)).unwrap(); am.add(new).unwrap(); assert_eq!(req.allow(), false); // Allow for CAT match let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); - new.add_subject(gen_noc_cat(allow_cat, v2)).unwrap(); + new.add_subject_catid(gen_noc_cat(allow_cat, v2)).unwrap(); am.add(new).unwrap(); assert_eq!(req.allow(), true); } @@ -687,7 +694,7 @@ mod tests { let v3 = 3; // Accessor has nodeif and CAT 0xABCD_0003 let mut subjects = AccessorSubjects::new(112233); - subjects.add(gen_noc_cat(allow_cat, v3)).unwrap(); + subjects.add_catid(gen_noc_cat(allow_cat, v3)).unwrap(); let accessor = Accessor::new(2, subjects, AuthMode::Case, am.clone()); let path = GenericPath::new(Some(1), Some(1234), None); @@ -696,13 +703,14 @@ mod tests { // Deny for CAT id mismatch let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); - new.add_subject(gen_noc_cat(disallow_cat, v2)).unwrap(); + new.add_subject_catid(gen_noc_cat(disallow_cat, v2)) + .unwrap(); am.add(new).unwrap(); assert_eq!(req.allow(), false); // Allow for CAT match and version more than ACL version let mut new = AclEntry::new(2, Privilege::VIEW, AuthMode::Case); - new.add_subject(gen_noc_cat(allow_cat, v2)).unwrap(); + new.add_subject_catid(gen_noc_cat(allow_cat, v2)).unwrap(); am.add(new).unwrap(); assert_eq!(req.allow(), true); } diff --git a/matter/src/cert/mod.rs b/matter/src/cert/mod.rs index 93e5e40..cc6bd31 100644 --- a/matter/src/cert/mod.rs +++ b/matter/src/cert/mod.rs @@ -319,12 +319,13 @@ impl DistNames { }) } - fn u64_arr(&self, match_id: DnTags, output: &mut [u64]) { + fn u32_arr(&self, match_id: DnTags, output: &mut [u32]) { let mut out_index = 0; for (_, val) in self.dn.iter().filter(|(id, _)| *id == match_id as u8) { if let DistNameValue::Uint(a) = val { if out_index < output.len() { - output[out_index] = *a; + // CatIds are actually just 32-bit + output[out_index] = *a as u32; out_index += 1; } } @@ -555,8 +556,8 @@ impl Cert { self.subject.u64(DnTags::NodeId).ok_or(Error::NoNodeId) } - pub fn get_cat_ids(&self, output: &mut [u64]) { - self.subject.u64_arr(DnTags::NocCat, output) + pub fn get_cat_ids(&self, output: &mut [u32]) { + self.subject.u32_arr(DnTags::NocCat, output) } pub fn get_fabric_id(&self) -> Result { diff --git a/matter/src/data_model/core.rs b/matter/src/data_model/core.rs index 7ea9659..55a0e9e 100644 --- a/matter/src/data_model/core.rs +++ b/matter/src/data_model/core.rs @@ -224,7 +224,7 @@ impl DataModel { AccessorSubjects::new(sess.get_peer_node_id().unwrap_or_default()); for i in c.cat_ids { if i != 0 { - let _ = subject.add(i); + let _ = subject.add_catid(i); } } Accessor::new(c.fab_idx, subject, AuthMode::Case, self.acl_mgr.clone()) diff --git a/matter/src/transport/session.rs b/matter/src/transport/session.rs index da8d7cd..bba2149 100644 --- a/matter/src/transport/session.rs +++ b/matter/src/transport/session.rs @@ -38,16 +38,18 @@ use super::{ }; pub const MAX_CAT_IDS_PER_NOC: usize = 3; +pub type NocCatIds = [u32; MAX_CAT_IDS_PER_NOC]; + const MATTER_AES128_KEY_SIZE: usize = 16; #[derive(Debug, Default, Copy, Clone, PartialEq)] pub struct CaseDetails { pub fab_idx: u8, - pub cat_ids: [u64; MAX_CAT_IDS_PER_NOC], + pub cat_ids: NocCatIds, } impl CaseDetails { - pub fn new(fab_idx: u8, cat_ids: &[u64; MAX_CAT_IDS_PER_NOC]) -> Self { + pub fn new(fab_idx: u8, cat_ids: &NocCatIds) -> Self { Self { fab_idx, cat_ids: *cat_ids, @@ -69,8 +71,6 @@ impl Default for SessionMode { } } -pub type NocCatIds = [u64; MAX_CAT_IDS_PER_NOC]; - #[derive(Debug)] pub struct Session { peer_addr: Address, diff --git a/matter/tests/data_model/acl_and_dataver.rs b/matter/tests/data_model/acl_and_dataver.rs index d692c6a..34eb234 100644 --- a/matter/tests/data_model/acl_and_dataver.rs +++ b/matter/tests/data_model/acl_and_dataver.rs @@ -420,7 +420,7 @@ fn exact_write_attribute_noc_cat() { // Add ACL to allow our peer to access any endpoint let mut acl = AclEntry::new(1, Privilege::ADMIN, AuthMode::Case); - acl.add_subject(cat_in_acl).unwrap(); + acl.add_subject_catid(cat_in_acl).unwrap(); im.acl_mgr.add(acl).unwrap(); // Test 1: Exact write to an attribute with permission should grant