connectedhomeip Audit all consumers of AsSecureSession

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

Jan 08 '22 07:01 bzbarsky-apple

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.