This pull request fixes issue #1063

[rcaa]

No description provided.

[paulsputer]

Thank's for the PR, but wouldn't this allow anyone to generate a session cookie given prior knowledge of the username? Perhaps taking the hash of the UserModel object would provide sufficient entropy.

[rcaa]

Perhaps the UserModel object without its password, @paulsputer ?

[gitblit]

I don't think the presence of the password will make it less secure; SHA1 is an avalanching hash. I do think the hash of the UserModel is an improvement.

[rcaa]

Are you guys suggesting something like?
user.cookie = StringUtils.getSHA1(String.valueOf(user.hashCode()));

[paulsputer]

Given that User.hashCode() hashes only the username it will have the same session hijacking security issue. Hashing other attributes of the UserModel would help minimize this risk. However, we must then consider the side effects. i.e. hashing permissions would cause session refresh on permission changes.

It's also worth considering the statistics here. Hashkiller has only 44E+9 of the possible 1.46E+48 hashes yet if we're only hashing usernames there will be far fewer permutations given assumptions about username construction and depending on configuration, usernames may be visible anyway. So there are many easier ways to get in. Also you may like to check out the RPC to make sure that's not allowing information leakage which could such aid attacks. ;)

Probably the most appropriate solution would be to generate a collection of random bytes and use that. It could be useful to allow the UserModel to generate it's cookie to avoid replication of the generation logic.

[flaix]

However, we must then consider the side effects. i.e. hashing permissions would cause session refresh on permission changes.

True, but actually that would be a good thing because permission changes should invalidate the existing cookie and create a new one.
Since the cookie is stored on the server, it doesn't need to contain any information, it could be completely random, isn't it? I don't see the cookie getting reconstructed anywhere on the server.

[gitblit]

Gitblit uses server-side sessions. The cookie is used to identify an account, but nothing more. I'm pretty sure that permissions are updated on each incoming request so if an admin changes them, the user should immediately see those changes without a logout/login cycle.

[flaix]

I also have an interest in tightening security for Gitblit, so I would advocate for an change of cookie creation. Can we close this PR in favour of #1116 which uses only a random value?
I could also offer a new PR with the added change that it uses SecureRandom from Bouncy Castle, plus setting a secure cookie if only HTTPS is used. The latter may have to be an option for weird forwarding cases, do you think?

[flaix]

Closing this pull request in favour of new pull request #1116 which uses a pure random cookie.