Adding time to getString to add to the entropy and decrease collision…

[ddproxy]

… opportunity

This is not perfect, but it mitigates the problem a little.

[ddproxy]

1921

[KorvinSzanto]

Could we resolve this by adding the user id and hashing the key in Concrete5_Model_UserValidationHash::getString?

<?php

class Concrete5_Model_UserValidationHash 
{

    public function getString($user_id=null) 
    {
        $key = Loader::helper('validation/identifier')->getString(64);
        if (!is_null($user_id)) {
            return md5("{$user_id}:{$key}");
        }

        return $key;
    }

    ...
}

[KorvinSzanto]

I mean really, these are only generated on login. We could just recursively check the database for collisions in Concrete5_Model_UserValidationHash::add couldn't we?

[ddproxy]

This would help - but would allow for same-user hash collision which isn't as big a deal.

Recursively checking the database for collisions is over-engineering the issue whilst the hashes SHOULD be unique already. In the UserValidationHash table - the uHash should be made into a unique constraint to bake this business logic into the design rather than forcing implementing by code.

[KorvinSzanto]

We already avoid same user collisions by clearing out old forever cookies when new ones are created in Concrete5_Model_UserValidationHash::add I think

[ddproxy]

https://github.com/ramsey/uuid is a great example of uuid library to support truly unique identifiers that can be further paired with a user id or other name-spaced identifier to eliminate collision ( https://github.com/ramsey/uuid/blob/2.8/src/Uuid.php#L1054 )

I didn't do this in the PR since the identifier validation was already baked into Concrete as is and does not migrate nicely to the name spaced UUID lib.

[ddproxy]

@KorvinSzanto When I tested my 136k logins - old forever cookies were removed but this relies on the user USING the same browser as before. If using multiple machines this would be moot.

Additionally the database entries were not constrained to a single uHash to uID - so on the database side there are also no constraints.

[KorvinSzanto]

@Remo this library requires 5.3.3, do you think we should integrate?

[katzueno]

Just FYI,

It happened to real life on concrete5-japan.org just now. (It's surprisingly timely matter :p)

• User A logged into c5j.org last night with "Keep logged-in" option
• User A woke up, and come back to c5j.org
• Suddenly, he was logged in as User B.

Fortunately, User A knows me and he immediately reported back to me. Also we're not collecting any sensitive information at c5j.org. (Phew....) It is big deal that it did actually happen on the live server.

[KorvinSzanto]

Yeah, we should definitely make sure this is fixed for good. I wonder when this was introduced. On Thu, Sep 24, 2015 at 6:52 PM Katz Ueno notifications@github.com wrote:

Just FYI,

It happened to real life on concrete5-japan.org just now. (It's surprisingly timely matter :p)

• User A logged into c5j.org last night with "Keep logged-in" option
• User A wakes up, and come back to c5j.org
• Suddenly, he was logged in as User B.

Fortunately, User A knows me and he immediately reported back to me. Also we're not collecting any sensitive information at c5j.org. (Phew....) It is big deal that it did actually happen on the live server.

— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-legacy/pull/1922#issuecomment-143100194 .

[ddproxy]

@Katzueno disable by having both forever cookie methods return false and removing the input for the remember me checkbox.

@korvinszanto the fundamental design of a cookie tied to a user account is flawed. The cookie and browser need to be paired to fully identify that user logged in. However I haven't done this before - instead I think the remember me should function like big web sites (amazon) and instead autofill the user name field for login. This still requires the user to enter their password and further constrains the authenticity of the new session.

[ddproxy]

As per the uuid suggestion - a uuid nor random hash should be used as a validator or authentication method. Even if the hash is bcrypt or md5. Using uuid or rebuilding this unique hash method serves to ensure a valid unique identifier for data and in my opinion should not be used for sensitive identification in the manner of the original issue I filed.

[KorvinSzanto]

Then I think hashing in the user id is a good fix since we already make these tokens unique per user. On Thu, Sep 24, 2015 at 7:15 PM Jon West notifications@github.com wrote:

As per the uuid suggestion - a uuid nor random hash should be used as a validator or authentication method. Even if the hash is bcrypt or md5. Using uuid or rebuilding this unique hash method serves to ensure a valid unique identifier for data and in my opinion should not be used for sensitive identification in the manner of the original issue I filed.

— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-legacy/pull/1922#issuecomment-143104387 .

[ddproxy]

Agreed

[ddproxy]

But, is this method used for generating the session key or anything related to before the user is logged in. I think this may be used for other extensions and may very possibly break ecommerce or non user based hashes if we require a user for the id in the website function.

[KorvinSzanto]

True. Then Let's switch on the type and ensure they are unique with a db query for this type only. On Thu, Sep 24, 2015 at 7:39 PM Jon West notifications@github.com wrote:

But, is this method used for generating the session key or anything related to before the user is logged in. I think this may be used for other extensions and may very possibly break ecommerce or non user based hashes if we require a user for the id in the website function.

— Reply to this email directly or view it on GitHub https://github.com/concrete5/concrete5-legacy/pull/1922#issuecomment-143107544 .

[ddproxy]

Updated - haven't tested this either.

I went back 5 years of releases essentially to C5-legacy init. This issue would have been integrated prior to 5.4.2

[Remo]

Wouldn't openssl_random_pseudo_bytes have a better entropy? Method isn't always enabled, but if it is it could easily be used. We also have a method in concrete5 which uses /dev/urandom which should also provide a proper entropy PasswordHash->get_random_bytes.

I'm no expert in this kind of topic, just pointing out what I've seen, but I'm fine with merging this if there aren't any objections.

[aembler]

Thanks for the post but I don't think this belongs in getString. GetString is supposed to be pretty simple, a method to generate a randomish but readable and copy able strong, and perhaps get one that hasn't been used before. If we need the user ID as part of the hash we should add that in the original usage of it - in the user class.

Couple other points. We should use $user->isRegistered() as $user is always an object. And $user->getUserID() is the proper method name.

[ddproxy]

@aembler The reason this needs to be resolved is because the file (validation/identifier) kinda tells what this object is supposed to do. Validate with an identifier. Regardless of what happens with this method, getString is malfunctioning for the very purpose of validating with an identifier or providing a unique identifier for validation.

@mlocati Thanks! I'm not a core concrete dev and I'm trying to get additional buy-in from Core C5 devs for A change.

[c5vagrant]

@aembler, I'm not sure we can really do this in the user class since the helper is what manages persisting these hashes. We're already pushing that responsibility to this helper by passing $uID, if we don't want to be changing the helper, shouldn't we create a new forever login hash helper to handle just this process?

[ddproxy]

I believe an always logged in hash is a huge security vulnerability.

It would be better, in my opinion, to have this turned off by default or swapped out with a username being saved (in cookie) and autofill the login form.

Integrating ramsey/uuid as a replacement for validation - getString would resolve the collision across the application and any third-party uses of validation - getString.