| Project: | LoginToboggan |
| Version: | 7.x-1.3 |
| Component: | Code |
| Category: | support request |
| Priority: | normal |
| Assigned: | stevecowie |
| Status: | needs work |
Issue Summary
I noticed a potential major problem. It happens when:
1. you have the Set Password option enabled with a Pre-Authorized role
2. where your users are required to be approved by an administrator
3. a pre-auth user decides to reset their password prior to administrator approval
The following code in: logintoboggan.module shows exactly what happens... The user is removed from their pre-auth role and is basically validated whether an admin approves them or not.
function logintoboggan_form_user_pass_reset_alter(&$form, &$form_state) {
// Password resets count as validating an email address, so remove the user
// from the pre-auth role if they are still in it. We only want to run this
// code when the user first hits the reset login form.
if (arg(5) != 'login' && ($uid = (int) arg(2))) {
if ($account = user_load($uid)) {
$id = logintoboggan_validating_id();
$in_pre_auth_role = in_array($id, array_keys($account->roles));
if ($in_pre_auth_role) {
_logintoboggan_process_validation($account);
drupal_set_message(t('You have successfully validated your e-mail address.'));
}
}
}
}
Comments
#1
Does anyone acknowledge that this is a big security issue?
#2
Hi Steven,
We've been looking at it today. Will be in touch shortly.
Edit: rolling a patch for you to test tomorrow morning, CET.
#3
Indeed it is.
I've been looking at this today and have a working patch. Will try to push it tomorrow.
EDIT: Didn't see greg's comment. That's all said then ;).
#4
Thanks!
#5
Ok, so here's the patch.
Some points:
1.- This happens for the use case you mentioned. In that scenario, the admin has to approve the user account in cuestion, and for that purpose, a link is sent to the admin, containing an-url generated with the password that the user has set at registering, among other things.
2.- With that in mind, if we allow the user to change his password *before* the admin has approved its account, the admin won't be able to approve it (because the hash of the link he has won't match with the one on the database), and will receive an error-message saying that the link he's trying to use is a one-time only link...
So the options are:
(a) In the function you pasted, check if the site settings requires the admin approval of the users, and if so, don't remove the user from the pre-auth role, and also re-generate a new activation link and send it by email to the admin.
(b) Simply forbid the user to edit either his password or his email (this affects too, for the tests I did) until the admin has approved the account.
I tried the (a) option, and the security issue was solved, keeping the user in the pre-auth role. However, the new activation email that I was receiving as admin, was still not working (didn't find why).
So went with option (b), and just removed the change password and email options from the user account, when he is in the pre-auth-role, and requires admin approval. Also checked that the user can't access directly to the request new-password page when any of this conditions happen.
#6
Forgot to change status!.
#7
Hi,
Thanks for looking into this. This patch doesn't not seem to work. My test case was the following:
1. Implement Path
2. Clear Caches
3. Register
4. Login with the wrong credentials (the following msg pops up: http://cl.ly/2G2s411H46050C1H0w2z)
5. Click the link in the message and type in my email for the test user and hit submit
6. I get an email with a one time login link and I click on it
7. I can change my psw and save my settings
8. Log out and login as the Super Admin view users
9. The test user has been removed from the Pre-Auth Role
Steve
#8
..that's a "Doh!" for me.
1.- Forgot that the request password reset form is different for registered users than for anonymous users, so as an anonymous user one should actually be able to request a new password, even though if the admin hasn't approved the account yet.
The following is the same patch from before, but just making an extra check to totally ensure that the user is not removed from the pre-auth role if the 'approve by admin' option is set up.
The security issue is then fixed, but this still needs work, since allowing the user to change the pass, will prevent an admin to activate the account via the activation email. Hence, we're back to my first approach (option a in http://drupal.org/node/1350134#comment-5314502).
Will try to look and solve this along the weekend.
Cheers.
#9
Thanks... This seems to work for the security hole. Let me know when you get the rest of it working.
Steve
#10
The logic of the set password setting is so that administrator approval is not required. Have you tested whether you have the problem you describe if you tick 'set password' and also configure that visitors can sign up without approval?
#11
@stevecowie - you think this might be an admin UI issue, e.g. we should not allow that with a form validate handler, rather than trying to code around it?
#12
@greg.harvey - fundamentally, yes, I think this is an admin_ui issue potentially. Think the approach should be first do what I suggested at #10 to find out if the actual problem exists when going with the logic of set password being associated with immediate login, and if it disappears as a problem then the next thing might be to consider is is a legitimate use case that you want users to have the capacity to set their initial password but not actually be approved. I will do the #10 use case test asap and then update this.
#13
Have now tested the module behaviour using the use case described at #10 and the module works as designed. Also, the instructions on the logintoboggan configuration form make this clear:
"They will not be assigned to the 'authenticated user' role until they confirm their e-mail address by following the link in their registration e-mail."
There might be a case for enforcing the setting that visitors can sign up without approval or at least adding some extra guidance on the user configuration form to point out that the setting for administrator approval doesn't make sense if you have allowed users to set their own password. Will look into that.
#14
Forgot to reset the priority.
#15
Hi,
I can't tell if this is being caused by the fix that you guys came up with, but now anytime a Pre-Auth user tries to edit their account the following major error comes up:
http://cl.ly/1n053W2n0f2u2R380N3y
I'm updating the priority to critical... if this is not related than I apologize ahead of time.
Thanks,
Steve
#16
To confirm... This is being caused by the patch... I think it is being caused by the various places where this code is:
+ unset($form['account']['mail']);
Only happens when Pre-Auth users are trying to save their profile.
Steve
#17
#18
I wouldn't use the patch because it's not required. If you look at comments 10 -13, we don't think this is a security issue. If you give new users the capacity to set their own password, then the assumption in LT is that the user will be approved without needing administrator intervention. As soon as the user provides email authentication the 'authenticated user' permission is assigned. Here's the relevant text from the LT admin form:
If "Set password" is selected, users will be able to login before their e-mail address has been authenticated. Therefore, you must choose a role for new non-authenticated users -- you may wish to add a new role for this purpose. Users will be removed from this role and assigned to the "authenticated user" role once they follow the link in their welcome e-mail.
#19
I'm not sure I agree. The idea would be that they shouldn't be able to change their password and email before being authorized, but they should potentially be able to edit other profile elements... The following modification changes this:
function logintoboggan_form_user_profile_form_alter(&$form, &$form_state) {
if ($form['#user_category'] == 'account') {
$account = $form['#user'];
// Checking if the user is in pre-auth role yet.
$in_pre_auth_role = logintoboggan_user_in_preauth($account);
$approval_required = variable_get('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL) == USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL;
// If so, and admin approval is required, he shouldn't be able to change
// his password or email, since this will cause problems with the hashes generated
// for the admin to approve the account.
if ($in_pre_auth_role && $approval_required) {
$form['account']['info'] = array(
'#markup' => t('You can\'t change your email or your password until an administrator
has approved your account'),
'#weight' => -100
);
$form['account']['mail']['#attributes'] = array('readonly' => true);
$form['account']['pass']['#prefix'] = '';
$form['account']['pass']['#suffix'] = '';
$form['account']['current_pass_required_values']['#prefix'] = '';
$form['account']['current_pass_required_values']['#suffix'] = '';
// unset($form['account']['current_pass']);
// unset($form['account']['current_pass_required_values']);
// unset($form['account']['pass']);
// unset($form['account']['mail']);
Thanks
Steve
#20
Hm, but that feels more like a feature request than a bug...?
#21
It still seems like a bug to me because even though Pre-Auth users can't reset their password they can still login and see their profile... If they hit the save button and see an error I would call that a bug. Would you not agree?
#22
steven, as I see it, if that error appears when using the patch I uploaded, it's not actually a bug of the module itself, but in any case a caveat of the patch (which is not added to the module).