Missing CASESession interlocks with NOC cluster

[tcarmelveilleux]

Problem

When UpdateNOC, RemoveFabric or RemoveTrustedRootCertificate are done, there is fundamental change to the credentials that may impact security properties.

This is because of two reasons:

1. CASESession holds a long term FabricInfo * that, in initiator cases, may possibly be no longer valid, and in responder cases, may relate to a fabric no longer valid, but where a Sigma3 reception may arise
2. CASESession does not attempt to validate a source of truth for invariants of the CASE session establishment remaining true for the duration of establishment, and this would be quite hard to do.

This could lead to unforeseen privilege escalation, or privilege loss, or cross-fabric session establishment.

Proposed Solution

To address this problem, suggest:

• Holding FabricTable * and FabricIndex rather than a FabricInfo * in session state, and always using look-up of fabric by index via a const FabricInfo * GetFabricInfo() const that is always used ephemerally and without keeping references to FabricInfo * over the session establishment --> this would reduce corners/edges of where FabricInfo object is long held but context has changed.
• Adding event handling to FabricTable and FabricTableDelegate for FabricTableHasChanged(FabricIndex index, const FabricInfo * newFabricInfo), where newFabricInfo * is null on deletion, to assist the following:
• On the following events, we should invalidate all pending CASE session establishment states for a given fabric, and attempt to send an error status report:
  • Removal of a given fabric (whether via RemoveFabric or RemoveTrustedRootCertificate), that is, whenever FabricTable FabricTableDelegate::FabricTableHasChanged() is called.
    • This is a separate item than invalidating all currently established session contexts that also must occur, but which are managed by other data structures in SessionManager.
  • On UpdateNOC succeeding, but before CommissioningComplete is received
    • This is a separate item than invalidating all currently established CASE session contexts except the one currently established that also must occur, but which are managed by other data structures in SessionManager.
  • On CommissioningComplete after successful UpdateNOC

FYI @bzbarsky-apple @msandstedt @emargolis @balducci-apple

[msandstedt]

Agreed with the proposal to store fabric table + fabric index. That is simple and will result in 'correct' behavior. Perhaps there is also a fancy c++ trick we can employ to prevent code from storing FabricInfo *.

As to eviction / cancelation during session establishment, I think this can be relatively simple as we are now allocating session slots in the secure session table at the beginning of session establishment. This can just be one more case where the session manager can reclaim the session table entry, and the code in CASESession / PASESession can just be mindful that it, like all other consumers of session references, may lose its reference.

This has implications for ongoing discussions between @kghost , @mrjerryjohns and myself. There had been an idea that during session establishment, sessions should not be evictable. This apparently will not be the case. We should just choose not to evict these to make room for new session establishment requests. But for fabric removal or NOC changes, it does seem very clear that the session manager will need to pull sessions out from under CASESession.

[balducci-apple]

I can't speak to the specific implementation details but idea of invalidating pending CASE session establishments when those scenarios arise is something I agree with.

[tcarmelveilleux]

Adding spec label because of the following constraints of AddNOC and UpdateNOC:

. If the current secure session was established with <<ref_CASE,CASE>>, subsequent configuration of the newly installed Fabric requires the opening of a new CASE session from the Administrator from the Fabric just installed. This Administrator is the one listed in the <<ref_cmd_AddNOC_CaseAdminSubject,CaseAdminSubject>> argument.

and

.. The <<Operational-Discovery,operational discovery>> service record SHALL immediately reflect the new Operational Identifier.

.. All internal data reflecting the prior operational identifier of the Node within the Fabric SHALL be revoked and removed, to an outcome equivalent to the disappearance of the prior Node, except for the ongoing <<ref_CASE,CASE>> session context, which SHALL temporarily remain valid until the NOCResponse has been successfully delivered or until the next transport-layer error, so that the response can be received by the Administrator invoking the command.

[kghost]

during session establishment, sessions should not be evictable

This may not be true. We can add changes that evict that session by handling OnSessionReleased in PairingSession class.

Holding FabricTable * and FabricIndex rather than a FabricInfo * in session state,

I'm not clear how it helps. Both index and info are pointer like objects, there is not fundimental different between them. The ideal solution should prevent dangling index from happening, we should release the pairing session when the fabric is removed.

[tehampson]

I'm not clear how it helps. Both index and info are pointer like objects, there is not fundimental different between them. The ideal solution should prevent dangling index from happening, we should release the pairing session when the fabric is removed.

FabricIndex does not correlate to a specific spot in the FabricTable; you can have FabricIndex of 32 be in spot fabricTable.mStates[0]. With that said FabricIndex does wrap around, how much of a concern that is I don't know at all. If for all intensive purposes it can be ignored then we can use FabricIndex, but if someone can think of a corner case where someone could do some sort of privilege escalation/privilege loss/cross-fabric session establishment with some sequence of events maybe we should store compress fabric ID and fabric table in CaseSession and use FindFabricWithCompressedId(CompressedFabricId fabricId)? I don't know how expensive computing the CompressedFabricId, so just leaving this comment to start the discussion on that, while I continue to read through code and understand the life of a CASESession

[bzbarsky-apple]

CompressedFabricId is not unique, so we should not be using it as a key.

Fabric index can wrap around, but ideally things that are referencing a fabric index will get notified on fabric removal and deal. Certainly deal before a new fabric can be commissioned.