search

project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.49k stars 2.01k forks source link

Audit all consumers of AsSecureSession #13397

Open bzbarsky-apple opened 2 years ago

bzbarsky-apple commented 2 years ago

Problem

Reading over #13330 there are various places where it's calling AsSecureSession where it's clearly not safe in the sense that we may well have a group session. The instances identified in #13396 are some examples. Another example is InteractionModelEngine::OnReadInitialRequest (where an invalid message could come in, with a group session and a read/subscribe message type... but invalid messages should not cause us to crash!). Another example is WriteClient::GetSourceNodeId (which maybe should not even exist?). Another one is emberAfGeneralCommissioningClusterCommissioningCompleteCallback which could get triggered by someone groupcasting CommissioningComplete (again, they should not, but we should not crash). Same for EmberAfClusterCommand::SourceNodeId.

A number of these used to work correctly before the refactor of #13330, not just in the sense of not crashing but in the sense of working for both unicast and group sessions, and the new code very much does not do that.

Proposed Solution

Audit all callsites of AsSecureSession. For each one, either document why it's safe, make it clear that it's safe by checking GetSessionType or better a new IsSecureSession accessor, or fix the code to not assume a secure session, depending on what the actual expectations for the relevant callsite are.

@andreilitvin @msandstedt @jepenven-silabs

mlepage-google commented 2 years ago

Another case that is OK to call AsSecureSession is when running commands that (per spec) require Administer privilege, which can only be granted via PASE/CASE and not Group auth mode. There are some examples of this in OperationalCredentialsCluster commands such as AddNOC. For these, we can probably just document/comment that assumption as they are audited.

turon commented 2 years ago

Removing v1_triage_split_4, as v1 must be reliable (not assert/crash easily due to corner cases).

jepenven-silabs commented 2 years ago

Removing from Group project since it's more of a general issue.

bzbarsky-apple commented 2 years ago

I think this audit still needs to happen, sadly...

woody-apple commented 2 years ago

Issue Scrub: Moving to maintainability.