-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audit all consumers of AsSecureSession #13397
Comments
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. |
Removing |
Removing from Group project since it's more of a general issue. |
I think this audit still needs to happen, sadly... |
Issue Scrub: Moving to maintainability. |
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 isInteractionModelEngine::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 isWriteClient::GetSourceNodeId
(which maybe should not even exist?). Another one isemberAfGeneralCommissioningClusterCommissioningCompleteCallback
which could get triggered by someone groupcasting CommissioningComplete (again, they should not, but we should not crash). Same forEmberAfClusterCommand::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 checkingGetSessionType
or better a newIsSecureSession
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
The text was updated successfully, but these errors were encountered: