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

Proposal to allow clients to specify a sessionid when connecting. (bug 497) #498

Closed
wants to merge 1 commit into from

Conversation

mthomas
Copy link

@mthomas mthomas commented Aug 30, 2011

This is a proposal for new functionality.

The intended use case is to allow clients to reconnect using the same sessionid that they originally connected with. This, combined with the RedisStore would allow a socket server to fall out of a cluster, a client to reconnect to another server in the cluster, all while maintaining data associated with the socket.

My proposal would be to modify the Manager.handleHandshake method to pull a sessionid from the query if it exists.

In order to utilize this feature, clients would have to set the sessoinid in the socket.options.query (client).

Any thoughts?

@einaros
Copy link
Contributor

einaros commented Aug 30, 2011

That's a data leakage and/or DoS vulnerability in the making right there. I'd spend more time considering options before turning to this.

@mthomas
Copy link
Author

mthomas commented Aug 30, 2011

Can you expand on that a little bit? My thoughts were the following...

  • we already will issue a user a new sessionid whenever they request one -- just hit the socketio endpoint repeatedly and you can create an infinite number of sessionids.
  • if we are worried about someone issuing themselves an arbitrary sessionid that overlaps with someone else's sessionid (guessing a sessionid) we should just increase the keyspace of the generated sessionids to make guessing impossible.

Thoughts?

@mthomas
Copy link
Author

mthomas commented Aug 30, 2011

All that said, I would also ask, do you even want this sort of "long lasting session id" to exist in socket.io? I think it could be useful in some/many cases, but one could also build this functionality on top of socket.io instead of in to socket.io.

@einaros
Copy link
Contributor

einaros commented Aug 30, 2011

we already will issue a user a new sessionid whenever they request one -- just hit the socketio endpoint repeatedly and you can create an infinite number of sessionids.

Sure, but the keyspace is large enough for that to be ok - so long as there are no overlaps, and "active but not currently connected" sessions are removed from storage regularly. This however brings me to another point: the current Manager.prototype.generateIdis faulty. First of all, seeding with something like server ticks + client count is far better than just multiplying two random numbers together (random * random is more random than random?). Second, there's nothing currently preventing id collisions. They are unlikely, sure, but entirely possible.

if we are worried about someone issuing themselves an arbitrary sessionid that overlaps with someone else's sessionid (guessing a sessionid) we should just increase the keyspace of the generated sessionids to make guessing impossible.

In my opinion it's better to either

  1. Disallow the client to say who he is entirely, or
  2. At least require the client to prove he is who he says he is, such as entrusting him with an encrypted or hashed bit of data containing a server generated secret along with the original session id. If this piece of data is passed to the server, it would be easy for the server to confirm the validity of the client - with no realistic risk of forgery

@mthomas
Copy link
Author

mthomas commented Aug 30, 2011

I like #2, but doesn't a sufficiently large/random session id provide the exact same protection? Either way the server sends the user something (in your idea sessionid+hashed/encrypted bit and in mine just a sessionid) and the user can later send that exact thing back to re-establish identity.

No need for a second encrypted/hashed token/bit of data if the sessionid is so large that it is impossible to guess.

To your first point, I totally agree. One strategy for creating session ids is UUID+(a few bytes of fairly strong random entropy) (in this case + meaning concatenation) -- UUIDs already provide a way of creating a globally unique identifier. However it is possible to guess UUIDs (super hard, practically impossible), so we add on some extra random to make them unguessable.

Another strategy is to just use very large (256bit, for example) strong random numbers for sessionids and leave it at that. While there would be some chance of collision/guessing one, the probability is many, many orders of magnitude less than that of an asteroid destroying the earth in the next one second.

Would you like me to create a patch to make the generateId function behave in this way? Also, does the sessionid need to be purely numeric?

@einaros
Copy link
Contributor

einaros commented Aug 30, 2011

I like #2, but doesn't a sufficiently large/random session id provide the exact same protection?

To be fair, no. Guids, within a scope, exist to provide a guaranteed unique identifier within said scope, for storage and referencing. Contrary to popular belief, they do not exist as a security measure.

While we're talking guids, there's also length to consider. If you have a load-heavy application, passing a longer than necessary identifier isn't necessarily a wise choice.

Anyhoo, all I'm really saying is that this isn't a decision to be made in a haste.

@mthomas
Copy link
Author

mthomas commented Aug 31, 2011

As a first step in this, would you be open to pull request strengthening the session ids? I can do some research into best practices for secure session identifiers (while balancing the length) and put something together.

@einaros
Copy link
Contributor

einaros commented Aug 31, 2011

Sounds good to me.

@denisu
Copy link

denisu commented Aug 31, 2011

What if a client specifies his own id, as for example 0. No matter how long the ids are, anyone can simulate two clients with the same id. I don't think its a good idea to let the client choose an id, which is used internally at the server. Excuse me if I got it wrong.

I like the idea with persistent clients through reconnects though. But maybe there are better ways to do it?

@mthomas
Copy link
Author

mthomas commented Aug 31, 2011

Really good thought... we would have to check to see if a sessionid is valid. That would mean storing all session ids in the store, expiring them, etc...

So we aren't letting a client choose a sessionid, just reconnect using their previous sessionid.

Something along the line of

Handshake:

  • did client pass in a session id?
    • check that the session id is valid?
    • yes, reuse session id. no, fail handshake.
  • otherwise
    • assign new session id
    • persist with expiration to store

Heartbeat:

  • update expiration of session id

@TEHEK
Copy link

TEHEK commented Sep 18, 2011

What if some malicious user tries to connect using someone else's session id? Client-side scripts can be reverse engineered and data can be tampered.

@tommedema
Copy link

What Tehek said.

You can however provide a secret key upon connection, which is only known to this user. If the user wishes to restore the state after a reconnection, he must provide this key.

After reconnection, the old key is replaced by a new one and the user is informed about the new key.

@dvv
Copy link
Contributor

dvv commented Sep 18, 2011

@tommedema: but the new key still sits at client-side, right?

@tommedema
Copy link

@dvv, both server and client side, yes. The only way this can be exploited is when the user transfers his key to someone else, in which case there is no real issue.

@dvv
Copy link
Contributor

dvv commented Sep 18, 2011

sorry, does "key is stolen" result in effect similar to "user transfers the key"?

@tommedema
Copy link

@dvv, ehh, yes, just like a stolen password. Theft will always cause problems.

Though you can reduce the chances of theft by using SSL or disabling this state persistence upon reconnection-option.

In a way that's like saying, let us not produce cars, because cars can be stolen, and we don't want that to happen.

@dvv
Copy link
Contributor

dvv commented Sep 18, 2011

(no need to make invalid examples). I mean if key is stored in browser, it is weak, because it is a shared environment and there are and always will be techniques to steal info in such environments.

@tommedema
Copy link

@dvv, the fact is that there is no safer way to accomplish this. Many people will agree that this is safe enough for most applications.

The only way this can be exploited is by means of:

  • someone having physical access to your browser session
  • the webmaster inserting malicious third party scripts in to the page, allowing these third parties access to the global scope
  • the user running malicious software (trojans, etc.) specifically targeted to extracting this kind of information
  • the website being vulnerable to XSS attacks

All these situations are extremely unlikely, and will not be a problem for most applications. Also, if any of these problems do appear, then this kind of session restore is the least of your worries. :)

Another fact is that browsers will force a disconnection when you open a flash file browser or alert box on several operating systems. This is the only solution for these disconnection issues.

Finally, in case someone does not need this functionality, it should be a toggle so that it can be turned on or off.

@dvv
Copy link
Contributor

dvv commented Sep 18, 2011

thanks for a truly good summary. though i'd say they ways you enumerated are not so extremely unlikely in the wild world (think of free metasplot, and paid exploit combines, and malicious corporate sysad, and...), i tend to agree so far i see no safer way than proposed challenge-response pattern

@einaros
Copy link
Contributor

einaros commented Sep 18, 2011

Why, oh why, is this problem attempted solved in 'new' ways? The only proper way of doing this is to use simple public-key crypto to send a client key joined with a server provided nonce (for replay protection) from the client to the server.

Also, what's being discussed here is a serious problem with the polling transports we're using right now. It really is trivial to break into other sessions, ssl or no ssl, if the session id is at any point guessable or available to other clients.

@mthomas
Copy link
Author

mthomas commented Sep 23, 2011

@einaros the key phrase is "if the session id is at any point guessable or available to other clients." -- the goal is to create a session id that is impossible/infeasible to guess (which will be possible to create one this lands https://github.com/bnoordhuis/node/compare/issue1330-2).

@martinthomson
Copy link
Contributor

I have a different take on this problem. See pull 858 for details.

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 this pull request may close these issues.

8 participants