Activation codes are problematic to be used in a URL

[phalox]

An activation code, you would typically send via an email, and append to a URL. But it seems that the algorithm used is generating characters that are not allowed:

e.g. $2y$10$hSl57gPMjTfIPf0sv5ic3.WTC2nWpfbwRAqsFd8T/2xzjNWrBPgKi

$ signs are by default not allowed in a CI URL. Fixed by replacing with ":", and doing so again in the other direction. / (forward slash) is a valid output, and this causes the URL parameter to be broken up in 2 parts.

It also seems that the register() function is simply using this variable and adding it to the URL, which I believe will give issues in many cases? From what I can see, this will be an issue in both hashing cases...

The easiest fix would be to use urlencode(), but it adds characters that make the URL look strange to the user. Or finding a different way for an activation key. In the end it's just used as an identifier that's stored entirely in the database. I don't see why you need special crypto for this. Just a good amount of length.

[benedmunds]

Hmm, probably happens intermittently due to the random hash. I think urlencoding is fine with me. Anyone else have an opinion?

[phalox]

I'm a little confused though. I've enabled the built-in email activation, but the code that's being passed in that URL is totally different from what's in the DB.

Here the codes:

The email is generated with the default template and uses the id and activation_code of the ion_model. What am I missing here? The activation_code in the email seems to be more compatible with a well formatted URL.

PS: I'm creating a new user, then deactivating him so that they have to activate by email.

[benedmunds]

Which branch are you using? and I agree, it should use the users.activation_code

[phalox]

I'm living somewhere on the V3 branch. I think on this commit: https://github.com/benedmunds/CodeIgniter-Ion-Auth/commit/ec1822dd10fe20a30b4438e8a6f4a8ba1921582b

But if it'd use the activation_code we bump into the charachters issue. So I'd really like to understand where this emailed code comes from.

The only place I see this used is: Lib/Ion_auth line 299 and I don't see any conversion happen.

/me must be turning nuts

On Thu, 11 Oct 2018 at 14:46, Ben Edmunds notifications@github.com wrote:

Which branch are you using? and I agree, it should use the users.activation_code

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/1271#issuecomment-428941501, or mute the thread https://github.com/notifications/unsubscribe-auth/ADzJemC4LcK1RDiteDwlC6dXnRCZG1hbks5ujz2ngaJpZM4W1now .

[benedmunds]

@phalox check the forgotten_password_code and activation_selector for matches

[phalox]

Forgotten password code is NULL, as I have simply used the 'register' function, like here:

https://github.com/benedmunds/CodeIgniter-Ion-Auth/blob/3/libraries/Ion_auth.php#L268

deactivate() causes the activation code to be genreated. A little later the activation code is added to the array for the view: 'activation' => $activation_code.

in the activate.tpl.php it's outputting this link: anchor('auth/activate/'. $id .'/'. $activation, ...).

.... and whiel writing this, I think I've found the answer....

The activation_code variable in the model object is actually a different value than what is in the DB:

    $token = $this->_generate_selector_validator_couple(20, 40);
    $this->activation_code = $token->user_code;

    $data = [
        'activation_selector' => $token->selector,
        'activation_code' => $token->validator_hashed,
        'active'          => 0
    ];

So the use of activation_code isn't consistent. because sometimes it refers to validator_hashed and only during deactivation it refers to user_code. In fact this variable isn't even set in any other use of the model. But I do seem to like the user_code output better because it's a combination of random_token, which in my case isn't returning funny characters. The hashing is returning all sorts of stuff that's not URL compatible.

But reviewing my own implementation of inviting a new user, I see that I should have just gotten the activation_code (the temporary 1 time code) just from the object, not from the DB.

Did you on purpose not store the activation code in the database so that only a hash version can be validated?

Toon

On Fri, 12 Oct 2018 at 15:53, Ben Edmunds notifications@github.com wrote:

@phalox https://github.com/phalox check the forgotten_password_code

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/1271#issuecomment-429333374, or mute the thread https://github.com/notifications/unsubscribe-auth/ADzJeq5WGKY3fkCUCmmS3Z_4AH4DVo75ks5ukJ7vgaJpZM4W1now .

[benedmunds]

Good work tracking this down. This code was written by @Indigo744 so I'd like to get their input, any feedback here?

[Indigo744]

@benedmunds thanks for tagging me.

@phalox

The approach selector:activator is used for activation code (PR #1212), for reset code (PR #1209) and for "remember me" feature (PR #1208).

The approach, and why it's more secure, is explained in this very interesting article: https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence

So indeed, you have in the DB the selector stored, and a hashed version of the validator. But in the link, you have the selector and directly the validator. I concatenate them in what I called the user_code which is simply selector.validator.

Only the user should know the real validator value. So in case of stolen DB, these token are useless and can't be used to reset/activate/deactivate/restore session (remember me feature).

When developing these evolution, we agreed with Ben to use the same hash function everywhere, be it for password or validator tokens. Hence bcrypt is used, and this is why you see hash with $2y$....

However, the hash should never be seen in the URL (indeed, the user need to have the real validator). Quickly reviewing the code, I cannot understand how you could have a hash inside the URL. First, it poses issue with codeigniter, as you noted, but it wouldn't even work for IonAuth since it would try to rehash it again.

As you can see in this line, the activation_code member is assigned to the user_code, which doesn't have the hash. While the naming may be a little confusing internally (the activation code visible publicly is really the user code), from a user point-of-view it should not matters, only the activation_code member is needed.

I need more information to pinpoint your issue, if there is an issue at all... You said that when using the default controller, you got a proper, codeigniter-compatible, activation URL... So it seems to works as intended... And now I'm confused and I don't understand what is really the problem here 😺

EDIT

While reading again this thread, I noted that you said

But reviewing my own implementation of inviting a new user, I see that I should have just gotten the activation_code (the temporary 1 time code) just from the object, not from the DB.

This is exactly what you should not do. And what you should actually never do when using a third-party library. It's like using internal variable. It could 1) not work as intended 2) change without notice.

Only, always, use the library interface!! This is what libraries are for! To abstract you from the nitty-gritty of handling user's connections and storage securely. Otherwise you are just bypassing IonAuth to make your own implementation. And that can only go wrong.

[phalox]

@Indigo744 Thanks a bunch for your elaborate explanation!

Indeed you've found where I went wrong :-) There's probably a couple of reasons why I tried to retrieve the code from DB (like easily wanting to test myself without email, which I've now solved with "Test Mail Server Tool" on windows).

I also fully understand your reasoning now as how to make a secure activation feature.

Still, 1 comment remains: The current implementation is a little confusing.

1. You've used the variable name "activation_code" which is in fact "user_code", hence the confusion with the database. $this->activation_code = $token->user_code; (model/deactivate())

2. "activation_code" (uhm, "user_code") is only used once in the deactivate function, therefore it does not make sense as an object variable (especially seen its temporary nature).

Therefore I'd suggest:

1. remove "activation_code" from the model.
2. return the user_code by the deactivate() method in the model
3. in ion_auth lib, use the output of deactivate()
4. document the modified functions

Would you agree that these changes would further improve the library?

I'm happy though that this also solved my url_encoding issue, now I can go crazy with activating/deactivating users!

Thanks for all the work you put into this, the abstraction is very much appreciated!

[Indigo744]

When implementing the new security feature for "activation" (and remember me, and reset), I tried to reduce the public interface change so upgrading to IonAuth 3.0 wouldn't need too much work.

Sadly, it translates in weird stuff like this, being named activation_code (like in IonAuth 2), instead of something like activation_user_code. There is also the DB's column naming. I didn't change the activation_code column, but only added an activation_selector. Hence more confusion...

At time, I didn't though it would be too much of a problem, being used internally and all. But this discussion made me realize that this confusion could very well translate to tech-savvy (and suspicious/curious) users who wants to understand what is going on and check if implementation is correct (which is a good thing by the way!).

I am not against modifying the current implementation. However, we should keep in mind that it would break any current website using IonAuth 3.0 and these features. One could argue that IonAuth 3 is not really official yet so it could still be done.

This is a decision only our dear @benedmunds can take. What do you think Ben?

[benedmunds]

Loving the discussion here.

I’d vote that we keep BC where we can, even if it’s a bit more confusing. How about adding additional comments (maybe even take some of the explanation above)? That way it’s more clear to future devs?

[phalox]

One more question:

Since I don't want to touch the abstraction... The account creation procedure would go like this:

1. An invite is sent to a user
2. The user opens the invite and needs to submit more details before he can finalize the activation.

The problem is that I cannot validate the user's user_code other than activating the user. But then the activation is already completed. I'd need to recreate this code:

    $token = $this->_retrieve_selector_validator_couple($code);

    if ($token !== FALSE)
    {
        // A token was provided, we need to check it

        $query = $this->db->select([$this->identity_column,

'activation_code']) ->where('activation_selector', $token->selector) ->where('id', $id) ->limit(1) ->get($this->tables['users']);

So that based on the $query I know the user. In a way, also the $id is useless because the identifier is entirely in the user_code.

Any other ideas?

On Mon, 15 Oct 2018 at 15:35, Ben Edmunds notifications@github.com wrote:

Loving the discussion here.

I’d vote that we keep BC where we can, even if it’s a bit more confusing. How about adding additional comments (maybe even take some of the explanation above)? That way it’s more clear to future devs?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/1271#issuecomment-429854591, or mute the thread https://github.com/notifications/unsubscribe-auth/ADzJenFa7FA9dexG1sa84HxKlws0XkBBks5ulI8ggaJpZM4W1now .

[Indigo744]

@benedmunds I'll have a look around and see where I can add comments. Expect a PR this week 😉

@phalox it sounds like you want to do something that Ion Auth do not provide (yet), which is separation between the activation verification and the activation itself. While looking at adding comments, I could try to add a function like user_can_activate(). How does that sound?

[Indigo744]

@benedmunds @phalox please check #1277

[benedmunds]

PR merged. Closing.