resubmit: improved persistent cookies

[visit1985]

Each user can have more than one persistent login token which enables persistent sessions in multiple browsers.

Tokens and cookies are regenerated on each login.

Tokens will be deleted if they aren't used for 90 days.

I've modified token generation from time to random number to make it more secure.

Regards,
Michael

[scroogie]

Hi there,

this is my first review of an Owncloud patch, so please bear with me if I'm wrong on some points. I just saw the review request on the ML and thought this is a good opportunity to give something back to Owncloud. Now on to the patch.
Sorry if I sound overly critic, I just try to be accurate.

First some general remarks:

• the patch includes some changes of " to ' for array keys. This is all commendable and that, but I have a problem with such on-the-fly changes of unrelated things. It takes away from the real issue this is about. In my opinion, there should be a seperate issue that patches all of these occurences in Owncloud, if and only if this is a suggestion in the coding guidelines. Otherwise it will end up inconsistent all over the place.
• the patch also includes new methods for the Preferences API. That is another seperate issue in my point of view. In that issue, the sanity of the API can be discussed. This issue here is about token generation.
  -- in such an issue one could discuss the introduction of the created field in the database table. Is not null default '0' sensible for the upgrade path?
  -- why is the field called "created", while it is changed on update? (and thus is more an mtime)
  -- then there is an introduction of "multiValues". If I understood this correctly, this is a preferences key that can appear multiple times with different values (or at least the functionality lets me think that). If you call with an $oldVal that is not yet in the table, it will create a new entry, leaving the old one intact. The rest of the API does not really honor such values as far as I can see. Also, how does the DB react to something like that? There is an index on userid, appid, configkey. In my opinion, such an API introduction can have some sideeffects (and wrong usage if its unclear).
  -- there is a method deleteKey() in Preferences. It takes $user, $app, $key and deletes the corresponding entry. This patch introduces a method "deleteValues". It takes $user, $app, $key, $date. Now this is in my opinion a bit confusing. Why does a method deleteValues take a key? Why does it take a date? Of course, what it does is it deletes all values of a key that are older than $date, but perhaps its the wrong name then? Notice that deleteKey also deletes all values of a key.

About the new token system:

I'm not sure that the proposed system is really an improvement. For example, in the old system, a stolen credential would always invalidate after 90 days. In the new system, a stolen credential stays usable, no matter what happens, as long as the thief visits the page at least once in 90 days. Even if the user changes his password. Or does Owncloud invalidate all rememberMe cookies on password change? (it should of course). Additionally, there is no way for the user to notice this, if he has two valid credentials, because the other one will not be invalidated. So if a user logs in N times, and always deletes the cookie thereafter, there will be N valid session cookies in the database, open for attack.

[bartv2]

This reminded me that i had a different sollution that i hadn't pushed. This is in multi_cookie_login of core. I think my solution is cleaner and doesn't need database changes specially for login cookies.

[visit1985]

Thanks for the review @scroogie! I'll keep it in mind.

@bartv2 your approach is much better than mine, but I found some issues in it:

• We should delete all outdated tokens from the $tokens array (and database) before checking it against the cookie.
• The call of deleteKey() after the authentication failed is wrong because the key was not part of the array and doesn't exist in the database.
• We should use OC_Util::generate_random_bytes() instead of time() in the token generation to improve security.
• As @scroogie wrote, we should delete all tokens on password change.
• We should delete and recreate a token after it is used for authentication. This will improve security some more.

I found this blog entry very inspiring: Persistent Login Cookie Best Practice

@bartv2 if you want I can assist you in coding. But I don't want to take it away from you ;)

P.S.: In a next step maybe we should add a password dialog for accessing the users personal settings to ensure, a unauthorized person can not change the users password.

P.P.S.: There is indeed a coding guideline which says: "double quotes in HTML, single quotes in JavaScript & PHP" ;)

[bartv2]

@visit1985 I don't mind you taking the lead with this, i'm working on other things at the moment.

[visit1985]

Ok, than I will do that and come back to you for a review.

Please close this merge request.

[DeepDiver1975]

Gentlemen - this is how software development in a community should work!
Thanks a lot - I really feel happy!

I'm looking forward to see more detailed reviews and constructive follow-up discussions like this