Why doesn't this module store the raw session ID in the cookie directly? #176

Closed
bookercodes opened this Issue Jun 21, 2015 · 68 comments

Projects

None yet
@bookercodes

I am posting in relation to this Information Security question that reads:

I am learning about session middleware.

You have to supply a secret or the middleware complains:

app.use(session({
 secret: "abc",
 resave: false,
 saveUninitialized: false,
 store: new MongoStore({
   mongooseConnection: mongoose.connection
 })
}));

I did some investigation and the actual session ID is

eKeYlF1DR6AtVkeFZK9vEIHSZT8e0jqZ

But according to the cookie, the session ID is

s%3AeKeYlF1DR6AtVkeFZK9vEIHSZT8e0jqZ.on5ifVE079C4ctKNdkNiJSh8NkQMckjd5fn%2FsxIQWCk

I am confused. Why is it insecure to store the session ID in the cookie directly?

As I understand it, if an attacker can attain this cookie, maybe via XSS or social engineering, the attacker can still hijack the session. I am not sure what the point of the secret is.

An authoritative member commented on this question:

Hi, welcome to Information Security. I'm not sure what makes you think that it is at all insecure to store the user's sessionid in his cookie (as long as you do it right). From what I can tell, you found that some halfbaked js library does something unintuitive. That doesn't mean it is insecure, or that that is the reason for it. You shouldn't be surprised if this is a halfbaked idea...

There is even more context in the comments of this answer.

Can someone here please shed some light on the issue?

Many thanks.

@dougwilson
Member

Too many people use an auto incrementing number as their session ID. You don't need to social engineer anything to guess another user's ID.

It also protects against leakage of a session ID outside of the cookie, for example if you were in a chat room and the app had a flaw exposing all the connected users' session IDs in the server communications--with only the session ID you cannot use it still, because it would have to be signed by an unknown secret to turn it into a valid cookie.

In the end, this lib was made a long time ago, extracted to it's own module and lives here being maintained. If a security expert would like to help us, that would be great. Signing cookies have many benefits, including you can change the secret and they all expire instantly, you are assured the cookie came from your server and was not guessed, and many others.

@dougwilson
Member

P.S. I am not the author of this module; you can fine the author of modules in the https://github.com/expressjs/session/blob/master/package.json

@dougwilson
Member

I would also like to reiterate that I stepped in to save this module from dying and simply maintain it's status quo. I can see from that stack exchange that the "security folks" are only interested in bashing this module, rather than perhaps coming over here, audited this code to even tell us if removing the signing is OK or not, or do we also have to so something else to be able to remove it?

I can only explain on what I believe are the reasons it was added before I took over this module. I am open to removing it if a security expert can assist in auditing the module and making sure it is the right thing to do, and none of the comments on that exchange are at all constructive as of this writing, mostly just "lol, it's unnecessary". Removing it would require a major version bump, which last happened 2-3 years ago, so we don't make decisions rashly, which is why I would like to see constructive thought before removing it.

@dougwilson
Member

So, I'm trying to figure out when the secret became required in this module (and sorry it's full history is not in this repo; it is a straight extract from Connect 2.x). I see that it is at least there in https://github.com/senchalabs/connect/blob/80b2d9ed70cae1bd51609dd41c1d69d9634a9ede/lib/connect/middleware/session.jsfrom January, 2011.

The other major Express.JS frameowork, hapi.js, also uses cookie signing for the session ID. Their module is https://github.com/hapijs/hapi-auth-cookie It may also be useful to ask this same question there, @alexbooker , so see what the reasoning it (they call the option password instead of secret, and as far as I can tell, it's required just like ours).

Edit: actually, the hapi-auth-cookie module may be more similar to https://github.com/expressjs/cookie-session than this module, so it may not be a valid comparison, sorry! The https://github.com/hapijs/yar module is probably the most similar to this module.

@dougwilson dougwilson added the question label Jun 21, 2015
@bookercodes

All good. I posted an issue there.

I appreciate your efforts in researching this issue. Thank you.

@dougwilson
Member

No problem. I am completely open to changing this, but it would be nice to understand what risks that would open, if any. I know this module has been the main Node.js session module for a long time and has passed much, much scrutiny prior. I mean, at worst, it's just unnecessary, but not actually harmful (or is it harmful? let me know), and at best it provides automatic protection for users who are using autoincrementing session IDs.

@gabeio
Member
gabeio commented Jun 21, 2015

It also protects against leakage of a session ID outside of the cookie, for example if you were in a chat room and the app had a flaw exposing all the connected users' session IDs in the server communications--with only the session ID you cannot use it still, because it would have to be signed by an unknown secret to turn it into a valid cookie.

Sadly, the author of the post that @alexbooker mentioned is correct if there was a way to retrieve cookies from other users, signing is zero defense as it would pass the exact cookie signed and all. The only thing this prevents is changing of the sessionID which is still something significant, in the event that there is NO way to steal other user's cookies the only way left to attempt to attempt to take over another user account is to tamper with the sessionID.

@dougwilson
Member

My main problem here is that the main answer on that stack exchange thread is an attack at this module's author and not constructive, which makes that person loose all credibility to me, so as far as I'm concerned there isn't even an answer to your question on stack exchange. I hope you understand that being hostile and berating people directly is not how things ever move forward in a community.

@dougwilson
Member

@gabeio this is correct, signing is not a defense at all from stealing cookie values.

This module also never claims anywhere that it is a defense, nor does this module suggest anywhere that storing the session ID in the cookie is actually insecure, which to me, makes the actual original question "Why is it insecure to store the session ID in the cookie directly?" weird, because we never said it was insecure to do so. To me, the question should be "Why doesn't this module store the raw session ID in the cookie directly?"

@dougwilson
Member

@alexbooker I was reading though OWASP to see if they had any guidelines, but I didn't see anything specific. I did find this section, though, which seems to be at least a reason why you'd want to HMAC the cookie value, which is what we are doing: https://www.owasp.org/index.php/Session_Management_Cheat_Sheet#Detecting_Session_ID_Anomalies

@alexbooker , thoughts?

@dougwilson
Member

Hey @evilpacket , do you have an opinion on this? What are your thoughts?

@dougwilson dougwilson changed the title from Why is it insecure to store the session ID in the cookie directly? to Why doesn't this module store the raw session ID in the cookie directly? Jun 21, 2015
@gabeio
Member
gabeio commented Jun 21, 2015

@dougwilson apparently the post author is mostly saying that the signing is completely useless, as long as the sessionid itself is long and truly cryptographically random. http://security.stackexchange.com/questions/92122/why-is-it-insecure-to-store-the-session-id-in-a-cookie-directly/92127?noredirect=1#comment155701_92127

@dougwilson
Member

@gabeio I understand that, which is also the same person who started right off the bat with "From what I can tell, you found that some halfbaked js library does something unintuitive" and "it's not halfbaked, it's fully baked cluelessness" and "The author of that JS library seems to be quite clueless, but with just enough clueness to make a mess of things. " thus, I do not trust any security advice from that user, no matter how smart or authoritative they are, as it is not constructive.

This module has existed since 2010 requiring a secret and signing the cookie's value, and not since has anyone come by saying "hey, don't do that for reason x, y, and z". Even this is just a "why?" and does not (yet) include any research, any analysis of how removing it would affect users, etc.

Since the session's cookie value should always be set as HTTPOnly, it's value, in the end, does not harm anything, because client-side JavaScript will not have access to the value and thus would have no reason to need to interpret the value.

This module is a very generic module, where users are allowed to pick really crappy session IDs, and thus out of the box, this module tries to make the cookie provided to the user tamer-proof. The session IDs we default to are also may not be cryptographically secure, so we would at least have to make them secure before we should remove the signing.

@gabeio
Member
gabeio commented Jun 21, 2015

Sorry, I am not trying to say that signing is bad or good or otherwise. I am just trying to understand both sides here. Looking around at others: https://docs.djangoproject.com/en/1.8/topics/http/sessions/#session-serialization states that a leaked signed session can lead to a remote code execution vulnerability. Though this is python and may not be applicable here.

@dougwilson
Member

Right. So far everything I'm seeing is all just speculation "maybe the HMAC is done wrong" or "if the session IDs are cryptographically secure and random/long", etc. Well, can we have an expect please come analyze this module and tell us? I would just like to see a true analysis and actual hard facts and numbers, rather that just making guesses, flailing arms, etc.

@dougwilson
Member

@gabeio in case it helps, that user asked:

What is the threat you are trying to mitigate here?

To why my reply is, this library has done this since at least 2010. We are not actually defending it protecting against something specific, rather we are defending against simply removing it based only on speculation and not on actual research into the workings on this library. How can you truly say that it does nothing? Can you provide analysis showing it literally provides no protection so when can turn it off and post the analysis for those asking questions afterwards?

@dougwilson
Member

For this who did not subscribe to the yar issue @alexbooker opened, a response was posted over at hapijs/yar#76 (comment) .

@hueniverse

I will add one thing that might not be obvious.

Most of the code used by this module for security is in the node-cookie-signature module. That module follows the standard practice of appending an HMAC to the string value being signed separated by a period character.

However, here: https://github.com/tj/node-cookie-signature/blob/master/index.js#L42 it is doing something that might seem odd to (hashing values before comparison). This is documented here: tj/node-cookie-signature#15. Basically, you never want to compare security related values using the built-in string comparison because under certain network conditions, the time it takes the server to reject a request because of mismatching values (password, hash) can tell the attacker how many characters it got right and keep trying, making it much easier to guess.

Another way of doing the same thing (which I think is more elegant) is provided here: https://github.com/hapijs/cryptiles#fixedtimecomparisonstring-a-string-b.

@dougwilson
Member

Thanks, @hueniverse , always good to see you :)

As for the original question, @alexbooker , I hope you have at least got something? I'm always open to improvements to the ecosystem, especially from people knowledge in the area. So far for all the years of this module, not a single security expert has stepped in to help or point in a direction as far as I know. If you know one who can help, let us know. Right now I'm leaving this issue open to see what comes out of it, but if there is nothing actionable still from this in a month, I'm not going to keep it open indefinitely.

Open source software is made better by thoughtful and proven contributions made to them, rather than distant rude comments.

@nallar
nallar commented Jun 22, 2015

Assuming this has been implemented correctly, it acts as tamper protection. This is of dubious benefit.

If the initial session ID is long and hard to guess, it adds nothing (and maybe increases chance of collisions, depending on the implementation). For example a properly generated GUID has such a low chance of collision that tamper protection isn't doing anything meaningful - the chance of guessing a session ID is astronomically small.

However, if the initial session ID is generated by incrementing a value for example, this does help. Using the raw int session ID would allow the user to easily guess other sessions.

The ability to invalidate all active sessions by changing the secret is a good point - however this sort of tamper protection definitely isn't the only way of doing it - simply removing all stored valid session IDs would work instead.

@dougwilson
Member

Thanks for your feedback @nallar ! So, I would like to get this figured out, and have the following questions:

If the initial session ID is long and hard to guess

Good to hear :) I see you said "if", so you're not saying we are for sure. Can you point me exactly how I can know if it is?

and maybe increases chance of collisions, depending on the implementation

Ah, I see. How can I tell if this is the case? Is there software I can use?

However, if the initial session ID is generated by incrementing a value for example, this does help

Besides the built-in ID generator, people can define their own and are definitely currently using just incrementing integers.

@dougwilson
Member

Basically, @nallar and all, I'm willing to do whatever we need, but so far I'll I've gotten is "if this" or "assuming that" and impossible to measure things like "If the initial session ID is long and hard to guess". That's all well and good, but in order for people to make correct decisions, I need to know how to measure things. For example, how do I know if the IDs we are generating are "[...] long and hard to guess"?

@nallar
nallar commented Jun 22, 2015

Besides the built-in ID generator, people can define their own and are definitely currently using just incrementing integers.

Then tamper protection is necessary. I can't confirm the way you're doing it is secure though - not a cryptography expert, sorry.

@hueniverse

@nallar your analysis fails to account for separation of concerns and for security layering. Session ids should not have to comply with security requirements as they are an implementation detail that depends on the storage solution and other constrains. It is also poor security architecture to use a single source of protection (hard to guess id). Analyzing this as a simple matter of math is wrong.

@nallar
nallar commented Jun 22, 2015

@hueniverse
That's correct. Given that the user can supply poorly generated IDs, tamper protection is a necessity.

However, it's wrong to claim that a single source of security is unsafe. The chance of GUID collision when properly generated* is miniscule.

edit: *V4 GUIDs from a good random source.

@hueniverse

@nallar if we are going to mince words, It would be wrong to claim that GUID is unsafe but also wrong to claim it is safe. It would be not-safe as in does not comply with industry best practices.

Bearer tokens over TLS are theoretically secure. But... what if you fail to validate the server certificate? What if you misconfigure something and leak your bearer tokens over a secure channel but to the wrong party? Layering security is a key principal which you are tossing aside as irrelevant.

Also, this is a generic module assuming as it's core requirement a wide range of users. It absolutely has to assume that some people will use it poorly (e.g. increment ids) and accommodate that. This is also common practice when building modules for a wide audience.

IOW, it is fun to make these academic theoretical statement about security but in the real world where this work exists, your attitude is harmful.

@nallar
nallar commented Jun 22, 2015

@hueniverse

IOW, it is fun to make these academic theoretical statement about security but in the real world where this work exists, your attitude is harmful.

I quite clearly stated in the first comment:

If the initial session ID is long and hard to guess, it adds nothing
[snip]
However, if the initial session ID is generated by incrementing a value for example,...

and subsequently replied with

Then tamper protection is necessary.

after being advised that library users can supply their own session IDs.

That's hardly advising this project to drop the tamper protection. If your issue is with more generally stating that it's unnecessary if your session IDs are suitable, I still stand by that statement.

@dougwilson
Member

So this discussion seems to have stalled out without anything happening for 7 days now. Can anyone offer up anything from #176 (comment) ? If not, then we'll end up closing this ticket out after a week without action, as we can't just keep a ticket open forever without someone offering up analysis on our current implementation rather than speculation.

@gabeio
Member
gabeio commented Jun 30, 2015

Good to hear :) I see you said "if", so you're not saying we are for sure. Can you point me exactly how I can know if it is?

@dougwilson
https://www.owasp.org/index.php/Insufficient_Session-ID_Length
owasp (open web application security project) states the session id should be above 128 bits.

@dougwilson
Member

So there are a few points here:

  1. The 128 bit length figure comes from the equation on that page. A key part of that is the number of bits of entropy in the session identifier. How can we determine that? We would need to confirm it fits in with their expected value before we can confirm the length of ours is good enough.
  2. If we remove the singing from our session IDs, then what should we do with user-supplied IDs? Always sign them still? Never sign them? Sign them but then don't sign ours (because that's not confusing). Drop support altogether for custom session IDs?
@gabeio
Member
gabeio commented Jun 30, 2015
  1. If we remove the singing from our session IDs, then what should we do with user-supplied IDs? Always sign them still? Never sign them? Sign them but then don't sign ours (because that's not confusing). Drop support altogether for custom session IDs?

I still vote, unless someone looks into it and tells us we for a fact got the signing wrong or node got the signing functionality wrong, it's more secure to have it than to not (always sign them still).

@hueniverse

You should just leave this alone. It is fine the way it is. Not perfect but reasonable. Just ignore all the people who play crypto experts on the web.

@sophister

Hi guys, is there ANY WAY to disable the auto signing for session id.
I'm trying to sharing session between nodejs and php, through memcache. But php sends raw session id ( not signed ) to browsers, white after that nodejs cannot recover session because cookie is not signed.

Although a library may provide best practice as it can, I think it should allow library users to configure it, signed session id, in this case.

@hueniverse

@sophister basically, you are asking for a gun so you and others can shoot yourself in the foot.

@inadarei
inadarei commented Jul 8, 2015

@sophister have you tried solving your problem on the PHP side instead? http://php.net/manual/en/class.sessionhandler.php

@sophister

@inadarei yes, I try change php's default session handler, according to this post

https://www.evernote.com/shard/s209/sh/740efea8-dded-40d0-95cd-2f1041b48eb5/c75e2226f8d896249b540d9f60f0f7a5

But, to solve the SIGNED COOKIE issue, I have to change php's session_id as the following gist

https://gist.github.com/mscdex/9507b0d8df42e0aec825

Finally, we've decided to modify express-session library code, REMOVE the signed related func.

@dougwilson
Member

I'm closing this issue since it's been 2+ weeks and still no one can tell me if our session ID is cryptographically random enough to tack action. I will re-open once the case has been presented showing we are creating cryptographically random session IDs or how to get us there.

@dougwilson dougwilson closed this Jul 8, 2015
@hueniverse

@dougwilson you should not remove the hash regardless of the random debate. The current design is proper.

@ircmaxell

@dougwilson looking at the uid-safe library, it's good and bad:

https://github.com/crypto-utils/uid-safe/blob/master/index.js#L67

function uidSync(length) {
  try {
    return toString(crypto.randomBytes(length))
  } catch (e) {
    return toString(crypto.pseudoRandomBytes(length))
  }
}

Basically, if it can't generate a cryptographically secure identifier, it'll fall back and generate a weak one. That's not good.

I'd suggest bypassing the UID generator entirely, and relying on crypto.randomBytes() yourself, failing if it cannot return sufficient entropy (failing the entire session storage). It's far better to not use weak randomness than it is to limp along.

@dougwilson
Member

Hi @ircmaxell , I know I Twitter you pointed to the C++ call underlying randomBytes. The one that underlies pseudoRandomBytes is exactly the same into OpenSSL. How would it be different, then, if it calls the same OpenSSL function? Can you perform a cryptoanalysis on the output on the two to show they are actually different or the same?

@Ajedi32
Ajedi32 commented Jul 8, 2015

@ircmaxell Worth pointing out: http://www.2uo.de/myths-about-urandom/

That said, the docs for crypto.pseudoRandomBytes do say it:

Generates non-cryptographically strong pseudo-random data. The data returned will be unique if it is sufficiently long, but is not necessarily unpredictable. For this reason, the output of this function should never be used where unpredictability is important, such as in the generation of encryption keys.

So it's probably not a good idea to rely on it, regardless of whether the standard node js implementation does return cryptographically secure random numbers.

@ircmaxell

They don't call the same OpenSSL function. It's templated C++ code, which makes it a bit hard to follow, but randomBytes calls RAND_bytes and pseudoRandomBytes calls RAND_pseudo_bytes. Check out the source:

  if (pseudoRandom == true) {
    r = RAND_pseudo_bytes(reinterpret_cast<unsigned char*>(req->data()),
                          req->size());
  } else {
    r = RAND_bytes(reinterpret_cast<unsigned char*>(req->data()), req->size());
  }

RAND_bytes is strong: https://www.openssl.org/docs/crypto/RAND_bytes.html

Read the warning on RAND_pseudo_bytes on that page.

@dougwilson
Member

Cool. Can you perform a cryptoanalysis on the output on the two to show they are actually different or the same?

@ircmaxell

@dougwilson in general, no. However that doesn't mean that RAND_pseudo_bytes can't be broken. Think of it as a lower bound. 99% of the time it may be fine. But that 1% is when it'll burn you. It's important enough to not rely on for session security (and justify using randomBytes() directly).

In general I don't trust OpenSSL's implementation of anything. But considering Node.JS uses it internally, if it's broken far worse things are going to happen. And it's not worth you working around it for a non-portable solution.

@dougwilson
Member

Gotcha. So we're sure it's only restricted to 1%? The reason we started falling back is two fold:

  1. People were not able to use this module because randomBytes will randomly throw, especially at server start and makes their site unusable.
  2. I'm told they are identical: http://stackoverflow.com/questions/18130254/randombytes-vs-pseudorandombytes#answer-18131768
@ircmaxell

@dougwilson Since it's a pluggable system, there are no guarantees that they are identical. The default install may use the same backend, but there's no assurance of that (note we're talking about openssl install here, not node.js install).

Even if they use the same algorithm, they are most definitely NOT identical. The fact that it throws if there isn't enough entropy is a good thing. That's what you want.

Otherwise you'd be creating insecure session identifiers during startup. The throwing behavior is precisely what you want to happen. That's the only way you can guarantee security of any sort. Silently falling back to an insecure alternative is not a good idea.

@paragonie-scott

If you can't generate a secure random session identifier, the sane thing to do is block until your CSPRNG is properly seeded before continuing on. Which means milliseconds of latency when your server first starts. I think that's a fair trade-off for not being able to predict session IDs and impersonate users at will.

@gabeio
Member
gabeio commented Jul 8, 2015

I think that's a fair trade-off for not being able to predict session IDs and impersonate users at will.

If you say it's only for the milliseconds of the start of the server then you will only be able to impersonate someone who just created a session at the exact beginning of the server start. Any already made sessions are safe and after 1s (as you say) the pool will be filled and randomBytes will never throw an error anyway.

@chrisfosterelli

@gabeio Sessions created from a non-random source would be vulnerable the entire time that the session was used. Sessions created after the entropy pool is full would not be, but that wouldn't automatically make the old sessions secure.

@chrisfosterelli

A better option would be to not use the uidSync method, which returns potentially non-random data, and instead use uid which takes a callback and returns when the entropy pool is full.

@gabeio
Member
gabeio commented Jul 8, 2015

Sessions created from a non-random source would be vulnerable the entire time that the session was used.

I understand how sessions work. Only session created exactly the few milliseconds at server start would be vulnerable. How many users would be really making a new connection exactly at server start. Especially if you have a pool of servers the likely hood is very low. And it's not absolute that it will ever throw an error anyway.

@gabeio
Member
gabeio commented Jul 8, 2015

and instead use uid which takes a callback and returns when the entropy pool is full.


Will throw error or invoke callback with error, if there is not enough accumulated entropy to generate cryptographically strong data. In other words, crypto.randomBytes without callback will not block even if all entropy sources are drained.

it will throw an error either way.

@chrisfosterelli

Oh, actually you're correct @gabeio it appears it either throws or defaults to pseudo-random in either case. Good point.

@ircmaxell

Which is precisely why I recommended moving away from that library.

@gabeio
Member
gabeio commented Jul 8, 2015

I see a few choices here:

  1. give up and say that crypto.pseudoRandomBytes can be as cryptographically strong as crypto.randomBytes as it states that the only time pseudoRandomBytes is "not" actually random (which @ircmaxell states OpenSSL isn't really perfect anyway) is such a slim moment in the beginning of a server start it's non-critical.
  2. adjust the function to block until crypto.randomBytes is available (freezing requests while server is booting).
  3. throw an error when crypto.randomBytes is not available (potentially crashing apps while the server is booting).
  4. mark crypto.pseudoRandomBytes sessions and on the next connection to the server if crypto.randomBytes is then available change the session id (only vulnerable sessions are present when first connecting to a server while it is booting after reconnecting fixes the problem)
@chrisfosterelli

Is anyone able to definitely say how long this period will be until the pool is full? Why is it only milliseconds on a server? Network and disk IO?

@paragonie-scott

Why is it only milliseconds on a server?

It might be nanoseconds. I do know that, for poorly implemented PRNGs, if you enter a race with its initial seeding and win, you get bad random data.

I think the best answer is that Node needs a better alternative for random numbers than OpenSSL.

Something like PHP's random_bytes() would be a fine addition here.

@dougwilson
Member

adjust the function to block until crypto.randomBytes is available (freezing requests while server is booting).

How do we even do this? The problem is even if you call the async version of crypto.randomBytes, Node.js will not wait for the pool to fill, it will instead just immediately return an error to your callback.

@dougwilson
Member

Also, if it's not clear, the solution we come up with MUST work with Node.js 0.8

Which is precisely why I recommended moving away from that library.

@ircmaxell so apparently us simply altering that library to be better is not an option? That reasoning would mean that we simply move away from this library :)

@ircmaxell

It's an option. But once you do that, what's the point? Why not just call the method directly rather than call it through 2 layers of indirection that literally add nothing?

@gabeio
Member
gabeio commented Jul 8, 2015

How do we even do this? The problem is even if you call the async version of crypto.randomBytes, Node.js will not wait for the pool to fill, it will instead just immediately return an error to your callback.

It probably wouldn't be pretty but you probably could just do a while loop and on a specific error just restart the loop attempting to wait for randomBytes to be ready if randomBytes never becomes ready this might be a very bad idea and really go into an infinite loop.

@gabeio
Member
gabeio commented Jul 8, 2015

@dougwilson which is why I am thinking my option 4 is looking pretty good. It will fix any of the issues of sessions staying non-cryptographically safe...

@dougwilson
Member

Why not just call the method directly rather than call it through 2 layers of indirection that literally add nothing?

It doesn't add nothing unless you're saying a cookie's value does not have restriction on the character set so we can just place the raw octets there (hint: you cannot do that). The library changes the character set to be safe for including as a HTTP token. Adding that here means we have to now maintain that here, no one else in Node.js can use the code, we have to provide all the necessary appropriate unit tests. Basically that is absolutely not happening.

which is why I am thinking my option 4 is looking pretty good

yea, we cannot alter the session ID afterwards, so that's off the table.

@dougwilson
Member

This discussion needs to be limited to providing a cryptographic analysis of our current session ID generation so we can stop guessing. I'm going to have to lock this issue if there keeps being further discussions not stemmed off of a provided cryptographic analysis.

@ircmaxell

The library changes the character set to be safe for including as a HTTP token. Adding that here means we have to now maintain that here, no one else in Node.js can use the code, we have to provide all the necessary appropriate unit tests. Basically that is absolutely not happening.

The library calls escape(buf.toString('base64')). Two method calls. One of which is completely unnecessary (you shouldn't be URL encoding unless it's going into a URL). So it boils down to buf.toString('base64'). A library for one method call? I get small and composable libraries, but that's a bit ad-absurdum, no?

yea, we cannot alter the session ID afterwards, so that's off the table.

No, but you could trigger a regeneration (delete the old token, and create a new one). I'm not saying this is a good idea, but it's possible.

I'm going to have to lock this issue if there keeps being further discussions not stemmed off of a provided cryptographic analysis.

We're discussing the quality of random number used in the id generation. There's nothing more fundamental to the security of this library than that. Even if you use "signed identifiers" having predictable (low random quality) session identifiers is still a potential attack vector (especially if the signing key is weak).

@dougwilson
Member

We're discussing the quality of random number used in the id generation

Yes, I understand, but are we discussing a provided cryptographical analysis of the IDs we are producing, or are we discussing speculation?

@ircmaxell

You may be speculating. I'm saying factually that using non-cryptographically-secure random strings as session identifiers is a significant problem. No matter how you try to mitigate it (in a general purpose library anyway).

@dougwilson
Member

I'm saying factually that using non-cryptographically-secure random strings as session identifiers is a significant problem

Ok. PRs are welcome to change this.

@dougwilson dougwilson locked and limited conversation to collaborators Jul 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.