Skip to content
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

Open
bzbarsky-apple opened this issue Jan 8, 2022 · 5 comments
Open

Audit all consumers of AsSecureSession #13397

bzbarsky-apple opened this issue Jan 8, 2022 · 5 comments

Comments

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Jan 8, 2022

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
Copy link
Contributor

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
Copy link
Contributor

turon commented Jan 31, 2022

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

@jepenven-silabs
Copy link
Contributor

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

@bzbarsky-apple
Copy link
Contributor Author

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

@franck-apple franck-apple added the p1 priority 1 work label Oct 24, 2022
@woody-apple woody-apple added maintainability and removed p1 priority 1 work V1.0 crash labels Nov 2, 2022
@woody-apple
Copy link
Contributor

Issue Scrub: Moving to maintainability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants