This issue follows on from http://drupal.org/node/69202

Currently, when a new user regsiters for a Drupal account the details are sent to the users supplied email address. This provides a basic mechanism that confirms the user is at that email address. However, once registered, users are permitted to change their email address without further confirming that the user is in fact at that email address.

Possible implications

  • A user can change their email address to be that of an unsusspecting third party as no confirmation of change is required. Using a second Drupal account (with it's email address also faked using the same method) the first user is then able to send anonymous malicious messages to the unsusspecting third party
  • A slow method for sending spam but exploitable none the less

This patch (originally by chx) averts this by having a "confirm stage" email sent before Drupal actually accepts the new email address.

drumm has originally mentioned a lack of comments. Well, most of the patch adds code into existing functions. The main change in the introduction of user_change_mail() which is a new callback. Added appropiate comment to this function.

I'm hoping beginner will follow up this with "the not so easy bit" ;)

Files: 
CommentFileSizeAuthor
#73 85494-73.patch14.27 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 48,694 pass(es), 37 fail(s), and 42 exception(s).
[ View ]
#37 user-mail-change.patch7.83 KBZen
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user-mail-change.patch.
[ View ]
#34 email_confirm_5.patch8.04 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 email_confirm_4.patch8.04 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 email_confirm_3.patch7.62 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 email_confirm_2.patch6.95 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 email_confirm_1.patch6.95 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 email_confirm_0.patch6.93 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 85494_user.module_r1.687_patch7.49 KBAjK
#14 email_confirm.patch5.84 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 user_mail_change_confirm.patch6.09 KBhunmonk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_mail_change_confirm.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 85494_user.module_r1.684_patch_16.21 KBAjK
user.module_r1.682_patch6.2 KBAjK

Comments

LOL! Why single me out :) I'm innocent, I didn't do anything, I promise!

Can the code be extended for deal with another situation where the user's email is not confirmed: when new user accounts must be reviewed by the admin before being set active. I have had this happen on a Drupal site. If the settings is that the admin must authorized new accounts, the admin may waste time reviewing new accounts created by robots or by a human spammer with fake emails. It is not always obvious which account is bona-fide, which is fake. The admin shouldn't have to make a decision on an account with an invalid email (user error is also possible).

The workflow should be:
1) user creates account
2) user confirms email is valid (where part of your code is relevant)
3) admin authorizes (or not) the account.

The following ancient proposal is not necessarily good, but it may give some ideas for a better systematic approach: http://drupal.org/node/64861.

My point is that your new function user_change_mail() should be user_confirm_mail() so that it is general enough, and reusable in the different scenario I've exposed.

I hope you don't regret that I DID follow up with "the not so easy bit" ;)

Not at all, your proposal sounds good. However, as drumm has pointed out, let's keep core patches nice, simple and byte sized ;)

This issue addresses one simple potential exploit which I would class as a bug. What you want is, imho, is a feature. A good one, but none the less, a feature. Introducing that on this issue will hamper it's chances of a commit.

The current issue is plain, simple and hopefully committable (review will tell).

I'd be happy to look at what you want to do, just lets get it on another issue and thrash it out there so this cuurent issue doesn't get bogged down.

Sound reasonable?

Thanks for the feedback.

regards,
-AjK

(p.s. I didn't single you in particular, just a gentle prod to get a review! How sneaky of me ;)

Your 'sneakiness' worked (and I don't mind it, so don't worry) :)

I know different issues must be kept separate, so I perfectly agree with your point.

My question still stand, though: is there a way to make your new function more general so it can be used for this bug report and reused for other separate issues later?
How difficult would it be to reuse existing code and make it more general to reuse in every situation when we want a mail check?

The answer to your question is "probably fairly easy". The only reason I'm holding back on this is that we are in code freeze right now.

I believe this issue is regards a "critical bug" and labelled as such stands a chance of being committed.

I believe what you want is a "feature" and if this issue thread goes down that road I'm 100% certain this issue won't be addressed in the code freeze before Drupal5 is released.

I would suggest you start a new issue for what you want and in link back to this issue and say "could have gone in that issue but missed the boat".

I'm all up for doing what you want, it's a good solution, just not on this thread (we can develope what you want on a new thread right now, it doesn't have to be postponed, just I'd like it on it's own issue, pref as a patch after this patch gets commited, to modify the function in core, not modify a patch with a patch).

seem reasonable? (if/when you start a new issue, contact me via my contact form so I can get subscribed to it)

regards,
--AjK

Title:Prevent "contact form" email abuseVerify changing user email addresses

Status:Needs review» Needs work

Found a bug in this patch.

When user updates their "my account" but leaves the password fields empty (i.e. they don't want to change their password but are changing something else) the email still gets sent informing them of their "intention to change their password".

Obviously, if the password fields are empty the "change email" workflow should not kick in and the email should not get sent.

I'll sort this out shortly but for now changing status to "code needs work"

regards,
--AjK

Status:Needs work» Needs review
StatusFileSize
new6.21 KB

Corrected bug in #6

going to test this later.

Status:Needs review» Needs work

Some t calls in #7 need work:

<?php
watchdog
('user', t('User %name used one-time email change link at time %timestamp.', array('%name' => "<em>$account->name</em>", '%timestamp' => $timestamp)));
?>

Afaik, % calls theme_placeholder, additional em-tags are not necessary. In the mail t with %-variables will get you em-tags in the plaintext mail.

StatusFileSize
new6.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_mail_change_confirm.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

updated patch attached. this addresses many issues:

  1. user admins should not trigger a confirmation email
  2. if user changes their password, the new password must be used in the hash
  3. error messages missing 'error' status
  4. unnecessary elements in $variables array
  5. email string substitutions should use !, not %
  6. drupal_goto after confirmation needed to be more intelligent
  7. cleaned up wording of the confirmation email

patch has been pretty thoroughly tested on a clean HEAD site, and seems to work perfectly.

Status:Needs work» Reviewed & tested by the community

Excellent work, Chad!

We shouldn't set 'X-Mailer' => 'Drupal'. AFAIK, drupal_mail already does that ...

I'll provide a more in-depth review shortly.

adding to queue...

Assigned:Unassigned» hunmonk
StatusFileSize
new5.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@Dries: looks to me like we don't need that header array at all there.

attached patch removes it. anything else we need to do here?

Status:Reviewed & tested by the community» Needs work

This patch needs work. Punctuation is not consistent; some sentences end with a point, other don't. Also, the URL scheme is awkard.

Restting a password happens at ?q=user/reset. Restting an e-mail adress happens at ?q=user/change/mail. Maybe that should be ?q=user/reset-password and q?=user/reset-mail.

I haven't tested the patch yet but it is not clear what happens as long the new e-mail adress hasn't been confirmed. What are the implications?

I haven't tested the patch yet but it is not clear what happens as long the new e-mail adress hasn't been confirmed. What are the implications?

The "reset email" that is sent out is time limited (24hours). If the reset link is not activated within this time, the email address is not altered and remains the same. In fact, it doesn't change to the new address until activation by the one time, time limited email link.

As for the implications, it means that if the Drupal generated link in the email sent out isn't activated within it's given timeout value, the email address is not altered.

Did that make sense? It's late!

Status:Needs work» Needs review
StatusFileSize
new7.49 KB

Maybe that should be ?q=user/reset-password and q?=user/reset-mail.

Patch attached addresses / corrects this.

Punctuation is not consistent; some sentences end with a point

Hmm, I may be English but languages is "low" on my "best of" list. So, I failed to spot the missing full stop!

StatusFileSize
new6.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

okay, i believe i've fixed up the last of the problems:

  • corrected bad/inconsistent punctuation, particularlry missing periods and inconsistency in the use of the word e-mail
  • changed the callback name to user/change-mail. user/reset-mail is not really an accurate name
  • changed the mail key to user_change_mail. mail_change_mail_send seemed awfully confusing

tested the mail change and password reset functionalities on a clean HEAD site, and both are working perfectly AFAICT. it would be great if we could get one more person to test them both...

Tested on fresh head:

1) Email changed only when the activation link is clicked. Did not wait to see if the link is disabled after the timeout though
2) Reset password works as it normally does

I noticed that in the confirmation mail that was sent when the email is changed, the From field is the user's old email address. Is this intentional? Or should the mail come from the site's email address?

StatusFileSize
new6.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

per chx's consult, changed $from to the site mail.

Status:Needs review» Reviewed & tested by the community

Just before the beta. To sum up: what's the point in the initial email confirmation if I can change it to foo whenever?

StatusFileSize
new6.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

patch broken by the capitalization commit. updated patch attached. no changes in code functionality whatsoever--just fixed up the conflict and updated the capitalization.

chx: note that we store the initial e-mail address, even if you chose to change it later. $user->init should always be valid, and we should have some sort of audit trail.

(We've never had this kind of functionality, and all of a sudden it is critical? The critical status is debatable, IMO.)

Note that this is the child of the contact spam issue which is indeed a very serious one -- hence the critical.

Status:Reviewed & tested by the community» Needs work

I think there is room for improvement:

1. I wonder what my mother will think when she reads: "The account identification numbers did not match.
2. "There was a problem with this one time link. Please try again.'" Try what again? And what difference could it make?

StatusFileSize
new7.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

per Dries' request, i have reworked the user messages in this patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

Is there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.

"If it is not used, your e-mail address change will not be processed." might want to say what will happen instead of what won't happen- "If not used, your email address at [site] will not change."

Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?

Is there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.

see #15

Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?

Good point, I would say so (if we're copying the functionality often found on other sites, might as well make it a deep copy ;)

Status:Needs work» Needs review
StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

s there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.

per dries' comment in #15 i'm leaving this change in.

If not used, your email address at [site] will not change.

fixed.

Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?

yeah, seems like a good idea. added.

i question the need for this. it is trivially easy to forge the from address of any email, not just ones sent from drupal. if proper authentication is our goal, i suggest we add a header like X-Drupal-UID and send the uid there. Or send the $user->init email address. Those are both authoritative/verified.

I don't really object to this going in, but it doesn't buy a whole lot and the critical status is shaky at best.

actually, contact.module already states the UID of the sender so we are covered there. thanks for illustrating that, boggieman.

Priority:Critical» Normal

StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email_confirm_5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

patch updated. functionality is the same.

Version:x.y.z» 5.x-dev

Version:5.x-dev» 6.x-dev

StatusFileSize
new7.83 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in user-mail-change.patch.
[ View ]

Patch no longer applies; re-rolled with updates to hook_menu; untested.

-K

Assigned:hunmonk» Unassigned

no time to put into this anymore. any takers?

Priority:Normal» Critical

I'd say this is critical. We need to be sure that a user owns the email address registered for their account, otherwise what's the point confirming it in the first place?

Also, without this confirmation, a user could change their registered email address to someone else's, causing all kinds of problems (content associated with a person, unable to create a new account, etc).

Version:6.x-dev» 5.x-dev

Hello everyone, has this problem been solved in drupal 5.x versions?

Version:5.x-dev» 6.x-dev

Changing back to 6.x-dev. I doubt this will be changed for 5.x.

Version:6.x-dev» 7.x-dev
Category:bug» feature
Priority:Critical» Normal

This can happily live in a contrib. Viva la form_alter!

Status:Needs review» Needs work

No surprise, but this fails against HEAD:

$ patch -p0 < user-mail-change.patch
(Stripping trailing CRs from patch.)
patching file modules/user/user.module
Hunk #1 FAILED at 801.
Hunk #2 FAILED at 1256.
Hunk #3 succeeded at 1381 (offset 106 lines).
Hunk #4 succeeded at 1536 with fuzz 1 (offset -14 lines).
Hunk #5 FAILED at 1763.
3 out of 5 hunks FAILED -- saving rejects to file modules/user/user.module.rej

Re: #40: A user is not able to change her email address to one that is already used by another user.

As chx said: "This can happily live in a contrib" and here is the proof:

http://drupal.org/project/email_confirm

feedback is welcome

Priority:Normal» Critical

I withdraw my objection earlier. This reallty does make perfect sense. Considering where we are in the release cycle, critical is warranted here.

I should elaborate on my use case. My client is is borrowing an idea from Facebook about authorization. Specifically, if you have a verified @harvard.edu email address then you belong to harvard group and are allowed to see certain premium nodes and so on. Naturally, we need to verify email address changes so that people don't just switch into harvard group.

So, I hope the need has been proven and someone can move on to polishing and committing the patch.

subscribing. Currently I'm drawn toward leaving this feature as a module add-on. Workflow is different for different sites and what one might do another will do something else. If the feature is added to core it needs to be an option that the admin can use or not use. So, this might as well as just be a contrib package.

Should this patch get re-rolled for D7, please have it call hook_user('update') - for Moshe's use case, other modules may need to know when the email address has been changed and take action based on the new address.

FYI - Decscription / Feature request for the email_confirm module mentioned in #46:

"Account hijack prevention management (prerequisites)"
http://drupal.org/node/475264

+1 subscribing

This seems like a good core feature to have for D7. I hope it doesn't die here.

+1 subscribe

Priority:Critical» Normal

"feature request" and "critical" are mutually exclusive.
maybe priority-major is warranted.

Version:7.x-dev» 8.x-dev

7.x is feature frozen anyway.

Priority:Normal» Major

Changing to major as per tag.

Tag update

subscribing

Subscribing. Some of this functionality should, IMHO, live in core. I've form altered the registration and account forms too many times to get this/similar functionality :-/

Status:Needs work» Fixed

Unless I'm totally mistaken, this feature is already in D7, so this is fixed.

Current password field on user profile

Wait... I thought the issue was about actually confirming the email address itself, not the user's *intention* of changing the address. E.g. If a user changes his email, then the user would get a confirmation email sent to the new email address, and the new email address wouldn't be used for $user->mail until its confirmed.

Right? That's kind of what I read in most of the posts here... I think.

Status:Fixed» Needs work

Oops! You are right, sorry. :)

Totally the wrong place to post this. That's what you get when you post while asleep. Sorry folks

I use LT along with http://drupal.org/project/email_confirm which deals with this issue.

My problem is when a user changes their email address BEFORE verifying the old one. This function is needed as a second chance for users to use a different email address and activate their account. But even though email_confirm is one form of email confirmation, LT is nor aware of it or that it already verified the address. I think I will write something for email_confirm to integrate with LT.

How do force account verification from PHP?

I support this request, as not having a valid user email address at all times is also a potential legal issue.
Users could post illegal content and there would be no ways of persecuting them.

It's not hugely problematic, as Drupal retains the original, validated address in the "init" field. But that address might be outdated and no longer functional.

Status:Needs work» Postponed

This functionality should stay in contrib, email could be changed by custom auth provider and it's not really useful for now. Let's close this issue

Status:Postponed» Needs work

IMO, this is important to have in core. Absolutely needed.

Agreed. Some even thought to make it critical -- while that's debatable, it's definitely a hole that needs to be fixed and should be part of core's default system.

There's a potential security issue here, user password may somehow happened exposed -- simple password, same password with other low secure sites etc.

Then people get the password can have full control of the account -- change password, email etc. And you'll never can get back your account!

So, we should at least let the hackers not able to change email easily. User needs to have email change confirm in the old email address and this features is very common in websites.

This feature should be able to port to D7, as it's a potential security issue!

Is there a contrib module that provides this functionality for Drupal 7? Thanks

http://drupal.org/project/email_confirm - have a beta release for testing on d7.

Issue tags:+Security improvements

@rogical - since Drupal doesn't store the password in plaintext I don't see how a password change could expose the user's current password. Further, I don't see how someone can be tricked into setting their email to that of an attacker.

Tagging to increase awareness.

I rather see a legal problem here and not a security problem. As a web site owner, you usually wish to maintain a valid email address for each of your users, in case they do anything illegal. So users shouldn't be allowed to change their address to an unconfirmed one.

You could also defend it from a usability point, since the verification protects the users from misspelled addresses which would prevent them from receiving updates.

Status:Needs work» Needs review
StatusFileSize
new14.27 KB
FAILED: [[SimpleTest]]: [MySQL] 48,694 pass(es), 37 fail(s), and 42 exception(s).
[ View ]

This is the re-roll against 8.x from #37 and a working patch.

Status:Needs review» Needs work

The last submitted patch, 85494-73.patch, failed testing.

This bug has been in existence since D5.x (2006) it's managed to exist before, during and after code freeze on D6, D7 and D8. Whilst the need for debate is understood this is a pretty basic and low level feature. The use case for it has been iterated and re-iterated many times on this issue queue and whilst a contrib module does exist the case is STILL there for this to appear in core.

There are, from what I can see, no blockers to this becoming a core feature so the time has come to ask why, after 7 years, this is not already a feature?

Is this ever likely to appear in core?

#69 Posted by arnoldbird on February 23, 2012 at 6:55pm

Is there a contrib module that provides this functionality for Drupal 7? Thanks

Hi, may out of topic but may more around questioned by you one of my bookmarks

knowledge-base/contributed-modules-securing-your-drupal-site
(Submitted by Ben Jeavons on Thu, 09/08/2011 - 15:53)
[Among the thousands of modules on drupal.org there are over 100 in the security category. Unfortunately some of those are abandoned or inaccurately tagged. We've looked at every module and compiled this resource to help you understand the security-related community modules available. Not all modules provide security exactly, some are about hardening your site against weaknesses and others are about monitoring and reporting abuses (....)]

.. or link drupal-7-user-login-and-registration

There is, at least in part. The module is: https://drupal.org/project/email_confirm