Update sha1 to sha256

[ajinabraham]

Recent research shows that SHA1 is a weak hash function known to have collisions. Consider updating it to stronger collision resistant (at this time) hash functions like sha256.
Ref:
https://shattered.io/
https://shattered.io/static/shattered.pdf
https://blog.qualys.com/ssllabs/2014/09/09/sha1-deprecation-what-you-need-to-know

[dougwilson]

Interesting. AFAIK this hash is used in a way in which would not be susceptible to that. Can you explain further on why you believe this usage would be at issue? How would a user cause a collision in the usage in this library?

The site seems to state

Any application that relies on SHA-1 for digital signatures, file integrity, or file identification is potentially vulnerable.

But AFAIK that is not the use case for the changed part in question here, so if you can provide some further details, that would be very helpful !

Edit: The last link you provided seems to be specifically about the usage in certificates, which AFAIK is also not applicable in the usage here as well.

[ajinabraham]

This is applicable if user input anywhere from HTTP request reaches the hash() function. I did a quick check by setting up a sample app and tried to control the data that reaches the hash function. But it looks like the signature checks associated with connect.sid doesn't allow me to tamper anything in the object that reaches the hash function.

Session {
  cookie: { path: '/', _expires: null, originalMaxAge: null, httpOnly: true }
}
Session {
  cookie: {
    path: '/',
    _expires: 2020-05-15T18:41:10.983Z,
    originalMaxAge: 100000,
    httpOnly: true
  },
  views: 1,
  user_test: 'asdasdasd'
}

If you are aware of any alternative means by which user controlled input reaches the hash function, this issue is applicable and you should consider using a stronger hash function, otherwise this might not be applicable to the project.

PS: I was doing a quick security scan on top node.js projects and this one popped out and I could only do a basic verification before opening this PR.

EDIT: By user controlled input I mean the data that comes from http client or browser, or the data that an attacker can control.

[dougwilson]

All of the input into the hash function is from the server-side code that users of this module write. It typically consists of private data that says on the server side and never travels to the client.

[twelve17]

Forgive me if I am misunderstanding, but I think vulnerabilities in sha1 do apply to this library:

Any application that relies on SHA-1 for digital signatures, file integrity, or file identification is potentially vulnerable.

I believe those at some use cases where the effect of a collision is more obvious, but it is not a complete list.

My understanding of the weaknesses in sha1 from those links is that, in rare cases, it can yield collisions:

a collision or clash is a situation that occurs when two distinct pieces of data have the same hash value,

What is the repercussion in this library if two different inputs yield the same hash value?

Looking at the code, sha1 is used in the hash() function, so any place that relies on the hash() value is, presumably, relying that a given hash value can only represent a given set of unique inputs. If a collision were to happen, it would mean that hash() function would yield the same hash for two different sets of inputs.

Where can two different session states yielding the same hash cause problems?

In isModified, this could yield a false negative (thinking it was not modified when it was):

   // check if session has been modified
    function isModified(sess) {
      return originalId !== sess.id || originalHash !== hash(sess);
    }

In isSaved, this could yield a false positive (thinking it was saved when it wasn't):

    // check if session has been saved
    function isSaved(sess) {
      return originalId === sess.id && savedHash === hash(sess);
    }

There might be or two other places (inflateSession) where this might be an issue, but I wasn't sure.

The linked articles have an emphasis on security because the implication of the possibilities of collisions opens obvious opportunities for malicious actors. I think, in the case of this library, the nearest implication in the quote above is probably "file integrity".

Perhaps this middleware could accept an algorithm option that can default to sha1, to preserve backwards compatibility?

[dougwilson]

A false positive for the isSaved and isModifed is not a security vulnerability, though. If you think it is, please email me a PoC that demonstrates how this would lead to a security vulnerability.