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

CASESessionManager now can mix up node ids from different fabrics #12867

Closed
bzbarsky-apple opened this issue Dec 10, 2021 · 1 comment · Fixed by #13074
Closed

CASESessionManager now can mix up node ids from different fabrics #12867

bzbarsky-apple opened this issue Dec 10, 2021 · 1 comment · Fixed by #13074
Assignees

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

The fabric association was removed from CASESessionManager in #12766, but:

  1. There are now callsites where we have a PeerId, we extract its NodeId and pass it in and reconstitute a PeerId.
  2. There are still places where CASESessionManager uses bare node ids (e.g. FindExistingSession!) and these can now collide across fabrics, allowing something that should be talking via one fabric to talk via another fabric, possibly to a compeletely different destination. I would be shocked if there were not a security bug in here.

Proposed Solution

Change all CASESessionManager APIs to take const PeerId & instead of a NodeId. Drop the FabricInfo arguments. Do the fabric->GetPeerIdForNode(nodeId) in the consumers that don't already have a PeerId.

@msandstedt

@msandstedt
Copy link
Contributor

Thanks @bzbarsky-apple for identifying this.

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

Successfully merging a pull request may close this issue.

3 participants