-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: create a section on session hijack #641
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
chore: create a section on session hijack #641
Conversation
|
It may be worth adding a bullet point under no.1 in session management like:
(I'm not sure if MUST/SHOULD verbiage is compatible with the fuzzy requirement of treating it in a 'secure manner') |
9ae6cc6 to
4f06749
Compare
| MCP servers **SHOULD** bind session keys to user-specific information. | ||
| When storing or transmitting session-related data (e.g., in a queue), combine the session ID with information unique to the authorized user, such as their internal user ID. Use a key format like `<user_id>:<session_id>`. This ensures that even if an attacker guesses a session ID, they cannot impersonate another user as the user ID is derived from the user token and not provided by the client. | ||
|
|
||
| MCP servers can optionally leverage additional unique identifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IP address binding may not generally best practice as the IP address from users on or tethering to mobile devices is apt to change frequently when roaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's admittedly hard to find good alternatives if the user isn't logged in. I did hesitate for this, as it does cause false negatives. I just wanted to add that there can be other data which can work. I'm happy to drop it if it doesn't add much.
Also, according to the spec, if you try to use a session ID and the session is no longer valid and your request is rejected, you get foreced to re-initialize. That can break the state on a stateful server though, so not exactly ideal, but for most scenarios it would just continue to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to add LGTM but tripped over the same thoughts on IP address in the concluding paragraph. Using an IP address in this way is such an unreliable anti-pattern that the mention of it could raise doubt on the credibility of the remainder of the guidance, which is otherwise solid. If it's possible to remove this example, the rest looks good.
4f06749 to
d73a925
Compare
vineethsai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Den Delimarsky 🌺 <53200638+localden@users.noreply.github.com>
Co-authored-by: Den Delimarsky 🌺 <53200638+localden@users.noreply.github.com>
|
|
||
| #### 2.3.2 Session Highjack Impersonation | ||
|
|
||
| ```mermaid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the diagram so that it's next to the description? I also wonder if we should have the "Session Hijack Impersonation" first since it's the simpler variation.
Co-authored-by: Stephen Halter <halter73@gmail.com>
Co-authored-by: Stephen Halter <halter73@gmail.com>
Co-authored-by: Stephen Halter <halter73@gmail.com>
Adds a section in the security best practices document on potential for session hijacking, and mitigations
Motivation and Context
With a horizontally scaled http server, there is a potential for session hijacking with incorrect implementation.
How Has This Been Tested?
Documentation only.
Breaking Changes
None
Types of changes
Checklist
Additional context