Cookies should store token, not hashed password

[joshuaadickerson]

https://github.com/elkarte/Elkarte/issues/2344

This is a shared issue between a lot of software products. We really shouldn't be storing passwords in cookies. See why in that link.

@jdarwood007 I was going to send you an email about this, but I figured you'd just ask me to make an issue anyway. This is in your wheelhouse and would be great if you spearheaded the development wink

[MissAllSunday]

I just read the announcement about focusing on bug fixes only and while I agree I was thinking about this particular issue. I have a working class for using a hash/token cookie. I've been working on porting that to SMF, question is @colinschoen, @Sesquipedalian would you guys be interested on adding this feature as a "last thing" before RC? The impact should be minimal. SMF targets 5.4 so an external lib for random_bytes() will have to be used but other than that its a pretty straight forward change.

[colinschoen]

I’d be in favor, yes.

[joshuaadickerson]

The impact would be that everyone would be logged out.

[albertlast]

Since the cookie got only the salted pw hash and the cookie is only send when you are get in touch with smf...

Get me to the point where this in my eyes is not a big issue, so ther is no need for smf 2.1 release, could create new issue in the upgrade process.

So instead of introduce new issue, you should fix the other/existing cookie issues.

[joshuaadickerson]

The security benefit is not so much in moving a salted and hashed password out of a cookie, as much as it is the ability to log out other users and creating an even better hashed password. Read everything I wrote in the Elkarte link.

It's not a big issue, but it adds a lot of possibilities for new features. There is no downside to this.

[MissAllSunday]

Indeed. Theres a bunch of posibilities, one of them is to create an user interface where they can see and select their active sessions and shut down them as needed. Admins can do the same of course. A scheduled task can be added to prune old entries too.

Not using the password also means we can improve the token cookie as needed without worrying about messing up with users data. Worst that can happen is they been logged out.

Another benefit, since the password will be touched/modified a lot less, we can increase the cost for it.

Since this is going to completely replace the existing cookie system, all issues associated with it will go too. No serialize stuff, plain json will be used and theres going to bea lot less data stored in the cookie. The minimal required would be:

        'userID' => $id,
        'token' => hash(self::$algo, self::$token),
        'selector' => $selector,
        'expires' => time() + self::$expires

The selector is used to avoid brute force attacks againts the database, when you check for an entry it doesn't do the check againts the token but the selector.

I haven't had time to work on this, will see if I can come up with something in the coming weeks.

[albertlast]

I see no user how is asking for this... and i see no need to add this in this late time of the stage.

[MissAllSunday]

It should have been added earlier, I didn't do it when I was a dev simply because there was other stuff to attend to but it was always my intention to do so.

And no, you will not find any user requesting this as they might not be able to fully understand it but that doesn't mean it isn't required. Just as they didn't requested SMF to use sha1 back in the day.

Just the posibility to easily delete compromised sessions without touching the password is reason enough to add this. If a bad guy want to steal your cookie all they are going to get is an easily disposable token that only works for your current machine and nothing more.

[albertlast]

When someone steal the session cookie, than will be no new session create -> which you could easy kill. Because he use your session, so you don't notice if someone hijack your session.

When someone is knowing you password, than he kills your session and create a new password.

• No request for this feature
• No usefull additionaly functionallty
• More complex approache
• No additionaly security

Btw you forgot in your cookie defintion the path, otherwise you cant handle when a board move in a subfolder. see: https://github.com/SimpleMachines/SMF2.1/pull/4373

[MissAllSunday]

The same already happens with the current approach and they not only get the session but the hashed password too.

An user can see if their session has been used in the past X days or weeks, if they do not recognize the session or they see one with recent activity that wasn't them, they can kill it. This is not possible with the current approach.

The whole point of this is not to touch the password/ not give it away that easily/ have disposable tokens per machine. This won't prevent session hijacking or other types of attacks but it makes it more difficult for a bad guy to get the password and try to crack it and it makes it easier to identify sesison hijacking and act on it.

[albertlast]

When the session is hijacked than they can't see that the session was used x days before, because is ther current session (when you want to show when you last visit was that you can archive with existing solution).

Also you don't want to kill the session, you wan't to change your password (why you wan't only to kill the session?), because you don't know the reason why/how the session was used.

We don't give the password away,

[joshuaadickerson]

In many ways it's less complex. No more dealing with passwords on every request. All of that logic is contained in the login module. Then your authentication module is just checking a string against a string. What can be easier than that?

There's tons of additional security. You can stop sessions from your computer. The password never leaves the database.

There is absolutely nothing you can do if someone has control of your current session. This doesn't address that, but that should already be handled. This addresses "oh shit, I left myself logged in while on a public computer!"

If I were developing, I don't know if I would include this right now if you're getting ready to release, but I would make sure it's the only feature in the next release so that 2.2 gets released quickly.

[albertlast]

In many ways it's less complex. No more dealing with passwords on every request. All of that logic is contained in the login module. Then your authentication module is just checking a string against a string. What can be easier than that?

The existing solution did this already,

There's tons of additional security. You can stop sessions from your computer. The password never leaves the database.

You a said here; https://github.com/SimpleMachines/SMF2.1/issues/3249#issuecomment-347104273 that are no additionaly security and you miss here the chance to mention some of them. Also the database didn't got the password.

There is absolutely nothing you can do if someone has control of your current session. This doesn't address that, but that should already be handled. This addresses "oh shit, I left myself logged in while on a public computer!"

You can change you password and reset all session, which is the finest solution instead of search for the right session to kill

[joshuaadickerson]

Okay, I hope it's implemented, but I'm not going to try to convince you it's the best way forward. One day I hope you'll understand the benefits. Unfortunately, I'm not the person to teach you today.

[jdarwood007]

This seems possible to implant but has some stuff we would have to iron out first and may be too big of a change for 2.1.

1. Add a last_modified to the members table.
   • Simple timestamp of when that member was last modified.
   • This is used in the token, if anything changes in the profile that we determine needs a token update, this allows that to happen.
   • This can also allow the admins to instantly invalidate all logins.
   • During upgrades, this would be set to date_registered initially; If doesn't kill the server should add rand (0, 7 days) to ensure uniqueness.
2. We would use a log_login_tokens table with id_member, token, token_time
   • token would be a bcrypt hash containing some sort of hash of id_member, member_name, password_hash, email and last_modified
   • token_time would be the time the token was created/updated
   • We would only update the token every 5 or 10 minutes.
3. If the admin changes the cookie expire time, the tokens table is checked for any tokens to expire and remove them (insta kill old sessions).
4. The cookie sends the id_member and token
5. The token validation is performed and if missing from the token table or expired (cookie expire time also should occur), does not allow logins.

[Sorck]

I have been giving this a bit of thought (and have nearly got a working solution locally) that folds the obvious improvements and some subtle ones.

New table:

smf_logins (id_token, token_hash, id_member, time_create, expires)

1. The user cookie "password" (as currently called in Load.php) will move from a SHA-512 hash based on the user password to being a two element array containing ID_TOKEN and TOKEN.
2. TOKEN is a base 64 encoded 54 byte cryptographic random number. This length is chosen as it produces an ascii representation that uses 72 characters - filling SHA-512. It is generated at LOGIN or TOKEN_REGENERATE
3. We create a hash of token using a random salt. This gives us TOKEN and TOKEN_HASH. Note that the token hash is assumed salted using bcrypt so the salt is appended at the end of the hash.
4. SMF inserts TOKEN_HASH into the database (along with id_member, expiry time, etc) and retrieves the autoincrement value of ID_TOKEN.
5. We give the client TOKEN and ID_TOKEN as per (1)
6. Tokens MAY be regenerated by SMF at any time. This could be on a rolling basis (every X minutes, every week) or only after certain actions.
7. Changing passwords MAY invalidate user passwords.
8. 2FA check success can be stored in the smf_logins table, creating a roken regen.
9. Tokens can be deleted from the database once expired, or upon user choice, or admin choice, etc.
10. last_online could be added to the smf_logins table, as could last_ip, and user_agent to help users identify their sessions.
11. If a user trys to authenticate without a valid token, blank their cookie as they are not logged in.

This procedure prevents the following two issues:

• Length extension attacks - the user does not see the token hash.
• DB dump leakage - if an attacker gets a copy of the database they cannot generate a login cookie without brute forcing some SHA-512 hashes.

Other option:

• Don't delete the token from the DB but have an "invalidate" column - would allow you to detect possible hacking attempts.

[joshuaadickerson]

You are way over thinking this. You don't need any encryption. You don't need the ID of the member or the token ID on the cookie. Just the token. If someone is trying to brute force a token, it is going to take them a ridiculous amount of time with 512 bit tokens. You can always add more entropy. You can make it the max size of the cookie.

It doesn't hurt to put the ID there though. Then just search for the ID, return the token and verify it. The other way is searching for the token and returning the ID.

I'd move the ID to a separate cookie while you're at it. Keep thing simple. No deserializing data.

No reason to have an id for the tokens.

On Mon, Sep 3, 2018, 3:50 PM Sorck notifications@github.com wrote:

I have been giving this a bit of thought (and have nearly got a working solution locally) that folds the obvious improvements and some subtle ones. New table:

smf_logins (id_token, token_hash, id_member, time_create, expires)

1. The user cookie "password" (as currently called in Load.php) will move from a SHA-512 hash based on the user password to being a two element array containing ID_TOKEN and TOKEN.
2. TOKEN is a base 64 encoded 54 byte cryptographic random number. This length is chosen as it produces an ascii representation that uses 72 characters - filling SHA-512. It is generated at LOGIN or TOKEN_REGENERATE
3. We create a hash of token using a random salt. This gives us TOKEN and TOKEN_HASH. Note that the token hash is assumed salted using bcrypt so the salt is appended at the end of the hash.
4. SMF inserts TOKEN_HASH into the database (along with id_member, expiry time, etc) and retrieves the autoincrement value of ID_TOKEN.
5. We give the client TOKEN and ID_TOKEN as per (1)
6. Tokens MAY be regenerated by SMF at any time. This could be on a rolling basis (every X minutes, every week) or only after certain actions.
7. Changing passwords MAY invalidate user passwords.
8. 2FA check success can be stored in the smf_logins table, creating a roken regen.
9. Tokens can be deleted from the database once expired, or upon user choice, or admin choice, etc.
10. last_online could be added to the smf_logins table, as could last_ip, and user_agent to help users identify their sessions.
11. If a user trys to authenticate without a valid token, blank their cookie as they are not logged in.

This procedure prevents the following two issues:

• Length extension attacks - the user does not see the token hash.
• DB dump leakage - if an attacker gets a copy of the database they cannot generate a login cookie without brute forcing some SHA-512 hashes.

Other option:

• Don't delete the token from the DB but have an "invalidate" column - would allow you to detect possible hacking attempts.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SimpleMachines/SMF2.1/issues/3249#issuecomment-418184275, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL6D3qYmKXV0jEk_ppQvjy-U9RXDI_wks5uXYgcgaJpZM4HAuvw .

[hrvylein]

You could use JWT as a token solution.

[MissAllSunday]

nah, hash() will do just fine, the thing we do need is random_bytes() which ships with PHP7, fortunately for us, theres an userlang implementation of that library for use with 5.4 which is our current min PHP version.

[albertlast]

You mean this userland random_bytes implementation? https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Sources/Subs-Compat.php#L461

[MissAllSunday]

I prefer we use composer rather than hard-coding the library on our own files.

[albertlast]

And why is this important to the topic here, from where the code is coming? An smcfunc call is missing in my eyes for the random_byte stuff.

[MissAllSunday]

Thanks for pointing this out, will create a new issue to replace the hadcoded functions/classes with composer.

Edit, https://github.com/SimpleMachines/SMF2.1/issues/5136

[albertlast]

@Sesquipedalian do we keep this as rc2 issue? because i see not possible to do this without changing the db layout.

[Sesquipedalian]

It would have been nice to make this change, but it is too late now for SMF 2.1. I agree that we should do this in a future version, though.

[MissAllSunday]

I could argue this pass as a security issue so 2.1.1 looks like a nice target.

Oh and theres no need to modify the db layout, at least not the current tables, a new table is nice but not mandatory.

If we see this as a new feature, ie logins per machine, hability to erase all your logins at onces, etc then the new table is justified and this is then pushed to 2.2 or something like that.

[MissAllSunday]

Indeed, this would have been nice to change in 2.1 but it's too late. I'm still planning to do this, either as a 2.1.1 patch or an official mod.

Closing this but will have in on my todo list.