Thanks a lot for your work on this architecture clean-up. It is a good base for a better implementation. I have commented a few questions and security concerns that raised my attention during a very quick review. Mainly it is about having different remember-me cookies from different browsers.

I don't see any gain in having different remember me cookie values for different browsers. This would only be relevant if you had very valuable data to protect, but in that case, you shouldn't use the remember me feature in the first place anyway. So I'd say from the standpoint of security, this is enough. I'm of course open for differing opinions.

I fixed the SQL error, added the key (only in the install SQL for now, will have to add that in the update SQL, too), removed JCrypt::hasStrongPasswordSupport() and JCrypt::timingSafeComparison() and removed the compatibility library for passwords, that was introduced in 3.2.0 as we are not effecitvely using it anymore.

@Hackwar Removing existing functions is not recommended. Deprecate them instead and remove it with 4.0.
Even if they don't work correctly anymore, 3rd parties may still use them and you don't want to create fatal errors.

Yes, I do want to create fatal errors. These methods are either not secure or they are not working correctly. If we leave them in, people will have bugs that they can't pinpoint.

We decided to revert the b/c breaking changes to the API. With those changes reverted, the removed methods will not work properly either. You either break the API for all extensions that work on =< 3.1.6 or only for those that only work on 3.2.0. That said, the decision for me is simple.

@Hackwar As you know, there is another PR which also uses JCrypt::timingSafeComparison and it works without issues. So don't remove what is not broken. For the broken stuff, you can add a log entry to help people find the bug and point to the correct solution.
If there is no compelling reason, don't remove. Deprecate. It's how things are done.

I would agree with you on a method that has any value, that has lived longer than one version and that isn't insecure. While timingSafeComparison() might not be insecure, I'm throwing it in the same basket as the rest of the methods that were added for 3.2.0 and remove it directly again.

As I understand the timingSafeComparison() function has been written by a crypto-expert.

While I agree with you and also find it completely ridiculous to try to hide a timing difference of nano-seconds in a single compare within a request that takes many miliseconds with a variability of miliseconds, I don't see a reason to break the API. Deprecate it and if you really want to be on the safe side from an auditing perspective, just replace its implemenation with return ( $a === $b ) so it's simple to security-audit :D .

But don't ruin your PR's chance to get committed by completely removing APIs please.

I doubt that the project wants to create FATAL errors to users sites when it's just to WARN them of a possibly insecure implementation (i personally doubt that timingSafeComparison would be insecure) of a single independant function. ;-)

@Hackwar Just asking while browsing through the current code. Does your solution follow the suggestions in this articles: http://fishbowl.pastiche.org/2004/01/19/persistent_login_cookie_best_practice/ and http://jaspan.com/improved_persistent_login_cookie_best_practice? I think the current solution does and they make some valid points there. Or are there newer articles which describe a better solution?

this may be obsolete as we committed #2672

Aye, and if I understand the proposed correctly, it's also highly unsafe. The current implementation uses a random token, a series identifier and the username to authenticate plus a timestamp when it times out. The token is replaced on each login. If any of these three values is invalid, all entires in #__user_keys for this user are deleted which invalidates each cookie. Also it takes care of a user with different browsers.
The proposed solution is less secure than our current implementation as far as I understand it (from the description).

@Hackwar I'm closing this one. If you think it should still be open, feel free to reopen it.

The proposed code only uses one random string. I don't see much difference between the security of the two implementations. In one case the random string needs to fit, in the other the string is longer and has pre-determined parts in it. Whatever attack´vector you use, you need the cookie and you always get the complete cookie. So there is no case where an attacker might only ready 70% of the cookie and thus have not the complete key to the system. The proposed solution also allows you to use this feature with as many browsers at the same time, it simply uses the same string. If one of those browsers logs out, all other browsers are logged out, too.

Yes, this code is not production ready, it wasn't meant to be. It was a quick hack to implement a remember me feature, since everybody said it would take to long to do a new implementation and it was to show that it could be implemented in a much simpler way.

In the end, this feature is by nature unsafe. If you have access to the cookies of a browser, you can get into the site with either implementation. There is no additional security in adding the series token and especially the username, at least not for the one browser solution. I might be convinced that with several browsers you might have less security. Overall however, this feature can not be called safe and if you want to disable it right now, that is not possible. It is so tightly woven into the system, that you can't disable it altogether.

@Hackwar Please read the two links I posted in a comment earlier. I can't explain it better what the purpose is of those added tokens. To me it made sense.
Basically it's about detecting a hacking attemp. If you get the same cookie twice (once from the attacker and the other from the user), you can say something is fishy and can invalidate all of them. You could even show a message to the user then.