| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user.module |
| Category: | feature request |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Security improvements |
Issue Summary
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" ;)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| user.module_r1.682_patch | 6.2 KB | Ignored: Check issue status. | None | None |
Comments
#1
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" ;)
#2
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 ;)
#3
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?
#4
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
#5
#6
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
#7
Corrected bug in #6
#8
going to test this later.
#9
Some t calls in #7 need work:
<?phpwatchdog('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.
#10
updated patch attached. this addresses many issues:
patch has been pretty thoroughly tested on a clean HEAD site, and seems to work perfectly.
#11
Excellent work, Chad!
#12
We shouldn't set 'X-Mailer' => 'Drupal'. AFAIK, drupal_mail already does that ...
I'll provide a more in-depth review shortly.
#13
adding to queue...
#14
@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?
#15
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?
#16
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!
#17
Patch attached addresses / corrects this.
Hmm, I may be English but languages is "low" on my "best of" list. So, I failed to spot the missing full stop!
#18
okay, i believe i've fixed up the last of the problems:
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...
#19
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?
#20
per chx's consult, changed $from to the site mail.
#21
Just before the beta. To sum up: what's the point in the initial email confirmation if I can change it to foo whenever?
#22
patch broken by the capitalization commit. updated patch attached. no changes in code functionality whatsoever--just fixed up the conflict and updated the capitalization.
#23
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.)
#24
Note that this is the child of the contact spam issue which is indeed a very serious one -- hence the critical.
#25
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?
#26
per Dries' request, i have reworked the user messages in this patch.
#27
#28
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?
#29
see #15
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 ;)
#30
per dries' comment in #15 i'm leaving this change in.
fixed.
yeah, seems like a good idea. added.
#31
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.
#32
actually, contact.module already states the UID of the sender so we are covered there. thanks for illustrating that, boggieman.
#33
#34
patch updated. functionality is the same.
#35
#36
#37
Patch no longer applies; re-rolled with updates to hook_menu; untested.
-K
#38
no time to put into this anymore. any takers?
#39
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?
#40
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).
#41
Hello everyone, has this problem been solved in drupal 5.x versions?
#42
Changing back to 6.x-dev. I doubt this will be changed for 5.x.
#43
This can happily live in a contrib. Viva la form_alter!
#44
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
#45
Re: #40: A user is not able to change her email address to one that is already used by another user.
#46
As chx said: "This can happily live in a contrib" and here is the proof:
http://drupal.org/project/email_confirm
feedback is welcome
#47
I withdraw my objection earlier. This reallty does make perfect sense. Considering where we are in the release cycle, critical is warranted here.
#48
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.
#49
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.
#50
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.
#51
FYI - Decscription / Feature request for the email_confirm module mentioned in #46:
"Account hijack prevention management (prerequisites)"
http://drupal.org/node/475264
#52
+1 subscribing
#53
This seems like a good core feature to have for D7. I hope it doesn't die here.
+1 subscribe
#54
"feature request" and "critical" are mutually exclusive.
maybe priority-major is warranted.
#55
7.x is feature frozen anyway.
#56
Changing to major as per tag.
#57
Tag update
#58
subscribing
#59
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 :-/
#60
Unless I'm totally mistaken, this feature is already in D7, so this is fixed.
#61
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.
#62
Oops! You are right, sorry. :)
#63
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?#64
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.
#65
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
#66
IMO, this is important to have in core. Absolutely needed.
#67
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.
#68
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!
#69
Is there a contrib module that provides this functionality for Drupal 7? Thanks
#70
http://drupal.org/project/email_confirm - have a beta release for testing on d7.
#71
@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.
#72
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.
#73
This is the re-roll against 8.x from #37 and a working patch.
#74
The last submitted patch, 85494-73.patch, failed testing.