Multiple instances of session ID allocator create session ID collisions at shared Matter port #12821
Assignees
Labels
Comments
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 9, 2021
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 22, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 23, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 23, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 30, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Dec 31, 2021
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Jan 4, 2022
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
This was referenced Jan 14, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Jan 27, 2022
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Feb 3, 2022
Add a central session ID allocator in mSystemState so aware consumers can avoid collision.
|
@msandstedt I believe #16233 addresses the critical problem here, but we might still want to move the id allocator into the SessionManager.... |
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Mar 31, 2022
Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outsanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate sesion table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Mar 31, 2022
Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outsanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate sesion table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Mar 31, 2022
Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outsanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate sesion table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Mar 31, 2022
Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this issue
Mar 31, 2022
Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass
kghost
pushed a commit
that referenced
this issue
Apr 4, 2022
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: #7835, #12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
chencheung
pushed a commit
to chencheung/connectedhomeip
that referenced
this issue
Apr 6, 2022
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
chencheung
pushed a commit
to chencheung/connectedhomeip
that referenced
this issue
Apr 6, 2022
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
andrei-menzopol
pushed a commit
to andrei-menzopol/connectedhomeip
that referenced
this issue
Apr 14, 2022
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
|
Fixed with #16895. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
Each controller object contains its own session ID allocator. However, all controllers (or commissioners) vended from the controller factory multiplex traffic through a single session manager.
This breaks session ID matching because the multiple allocators can have collisions. This also isn't spec compliant and cannot work when multiplexing secure channel traffic through a single port.
Proposed Solution
We will also need cleanup around session ID persistence.
The text was updated successfully, but these errors were encountered: