expressjs / session

Simple session middleware for Express
MIT License
6.26k stars 978 forks source link

The capability to supply a serializer (parse, stringify) for the req.sessionID in setcookie & getcookie (due to privacy & security concerns) #468

Open pascalvree opened 7 years ago

pascalvree commented 7 years ago

Due to privacy/security concerns, we have to encrypt the sessionID that's stored within a cookie. It would be great, if we could set a cookie value/sessionID serializer within the session options so we can keep using your excellent module. I am aware of the reasons for 'signing' a cookie values and being able to 'encrypt' the sessionID should not replace 'signing' the cookie. It should be in addition to 'signing' the cookie value.

If I would supply you with a pullrequest (from a fork) that implements the following change, the option to supply your own sessionID/value serializer using options.serializer; would you be willing to consider it?

   function session(options) {
      var opts = options || {};

      // get/set the cookie value serializer
      var serializer = opts.serializer || { parse: value => value, stringify: value => value };
      ...
   }

   // set cookie
   setcookie(res, name, serializer.stringify(req.sessionID), secrets[0], req.session.cookie.data);

   function getcookie(req, name, secrets) {
     ...
     return serializer.parse(val);
   }
dougwilson commented 7 years ago

Hi @pascalvree I guess first I would like to have a better understanding of what the "privacy/security concerns" are of just having the signed session ID in the cookie. Since the session ID is just a completely random string that has no meaning beyond associating a data bag for this module, I don't see how exposing it is bad? Because if you just encrypt it with a CBC, for example, the plain text (session ID) to cipher text would still be a 1:1 relationship, effectively the new cipher text string would be the session ID to the users, since it would remain static throughout their session just like the current random characters of the session ID are.

pascalvree commented 7 years ago

Hi @dougwilson I agree with you there. There will be a 1 to 1 relationship, even when crypted.

We're building a platform that will work with financial and personal data, therefore we're obligated to implement several security measures. One off these measures, forces us to encrypt (and sign) cookie values. The signing is allready in place. Encryption of the cookie value is not yet in place.

The purpose of encrypting the sessionID is to harden the application, for it makes it harder to connect the cookie value to the sessionID to the property bag. Because the sessionID value is not out-in-the-open. It's not to prevent theft, it's a measure to not disclose the value out in the open.

Next to this measure, we're also looking into regenerating the sessionID regularly (also a measure we have to implement in order to harden out application). These measures are obligatory and are inspired by https://www.owasp.org/index.php/Session_Management_Cheat_Sheet.

To be perfectly honest, we have to abide by the govermental regulations due to handling financial and privacy related information. bump

pascalvree commented 7 years ago

@dougwilson, sorry to bother you. Have you had time to read and evaluate the additional info I provided yet? I would gladly provide you with a pullrequest (also containing some test). But before I would do that, I'm curious towards how you feel about the info and the feature I describe above :)

gabeio commented 7 years ago

Just wanted to pointing out that the OWASP link only states that the cookies need to be encrypted if the cookie's value actually contain data of value. Though honestly I'd highly suggest against storing ANYTHING within cookies besides completely random keys (as already done in this library). What it does state though is that the database which stores these sessions really needs to be encrypted (I would even highly suggest on an encrypted disk especially in your situation).

If the session objects and properties contain sensitive information, such as credit card numbers, it is required to duly encrypt and protect the session management repository.

Ensure entire cookie should be encrypted if sensitive data is persisted in the cookie.

pascalvree commented 7 years ago

@gabeio yeah I agree with you there. In our situation we have to deal with some governmental and financial regulations that extend OWASP. Due to our application handling personal and financial information. Those regulations state that all information stored within cookies (even if it's only a randomized sessionid) is sensitive information that needs to be encrypted.

in order to get that information encrypted and still keep using the express-session module, I propsed a small patch that adds that functionality. The patch is backwards compatible and consists of 3 change to the code.

These 3 changes grant us the freedom adhere to those financial and governmental regulations/restrictions due to the information we're processing in our app while still using the npm provided express-session package. It would mean much to us, if you guys would accept the proposed changes. If you would accept these changes, I'll provide a pullrequest which embodies them.

Let me know, if you guys need more information in order to decide if you would accept them :)

pascalvree commented 7 years ago

@dougwilson have I provided you with enough information regarding our use-case and the proposed change in this thread/issue?

dougwilson commented 7 years ago

Yea, though the reasoning still doesn't make any sense. But now that I understand more, this is already a proposed feature, with many existing discussions and in progress PRs (for example #420 #158 #79 and more).