I am seeing more and more user registration attempts on my sites, although none of them have a visible user block. And none of them feature registration anyway. These attempts (from people? bots?) are clearly evil. I guess this is the price we are paying for the increase in Drupal's popularity.
The default in Drupal is "Visitors can create accounts and no administrator approval is required." It is easy to overlook this setting when installing Drupal. It is not mentioned on the welcome screen. It is not part of the new initial installation guidance in D6.
This sucks. Once registered an evil-doer has already passed one hurdle on his way to breaking a Drupal site.
I believe we need secure defaults. I believe the default should be "Only site administrators can create new user accounts."
Comment | File | Size | Author |
---|---|---|---|
#43 | drupal.admin_only_174972_43.patch | 15.5 KB | rfay |
#40 | drupal.admin_only_174972_40.patch | 13.61 KB | rfay |
#38 | drupal.admin_only_174972_38.patch | 12.83 KB | rfay |
#36 | drupal.admin_only_174972_36.patch | 12.83 KB | rfay |
#31 | drupal.admin_only_174972_31.patch | 8.99 KB | rfay |
Comments
Comment #1
StevenPatzComment #2
JirkaRybka CreditAttribution: JirkaRybka commentedI would vote for a warning somewhere in text-guides, as many sites (MOST sites, I believe) need the user registration working the currently default way, so "secure defaults" would mean breaking the out-of-box functionality, making Drupal more difficult to set up in common cases.
Comment #3
alanburke CreditAttribution: alanburke commentedHave to agree with Gaele here.
I guess it depend on use cases [will you site allow open registration or not].
My opinion is that open registration should be a conscious choice, and should be done when access rules have been tested and refined.
I would see this as a "usability bug/feature" and thus still OK for Drupal 6.
Alan
Comment #4
JirkaRybka CreditAttribution: JirkaRybka commentedI have no strong reason to push on this - fine with me with one comment: If default changes to disabled free registration, then the step to allow visitors for registration should be definitely mentioned somewhere in the installation process.
Comment #5
gaele CreditAttribution: gaele commentedWell then. Whatever the default would be, could this be made a conscious choice in the installation process?
Comment #6
Gábor HojtsyWell, this is an "install profile" thing. There was a push to include more install profiles in Drupal 6, but it did not happen and we are in bugfix mode, so new profiles will not be accepted. Whether you set up a community site or a brochure site makes a difference in how you allow user registration.
Comment #7
gaele CreditAttribution: gaele commentedAnd the default install profile is aimed at what?
It's not that the spambots are bugging me. I can manage. I just don't want Drupal to get a bad name because of this. Right now Drupal out-of-the-box defaults to "spambots: allow".
Comment #8
catchNeither, both.
I think having the default behaviour to block new user registrations would result in a lot of support requests, agree this would be good in a brochure site profile though.
Comment #9
gaele CreditAttribution: gaele commentedWhat i mean is: the default install profile should ask. "Is this what you want?"
Comment #10
dwwI agree with:
A) defaulting to closed (or at least requires-admin-approval).
B) making this setting visible in the installer.
We're now going to bother everyone installing drupal with a question of if they want to enable the update.module to help keep their site more secure, in the interest of appeasing the <1% of our user base who's so freaked out about privacy they don't want to have their site check for updates (see http://drupal.org/node/178581). Therefore, I think it makes perfect sense to have a question in the installer about such a fundamentally important aspect of how the site is configured. This is something that nearly 100% of drupal installations should think about, and no matter what we choose as the default, we'll probably be wrong for 2/3rds of all sites.
Comment #11
stBorchertThe patch inserts a new fieldset to the configure form during installation where the registration options can be set.
There should be some additional explanation on how this setting affects the site behaviour so literally this patch needs some work.
Comment #12
geodaniel CreditAttribution: geodaniel commentedThis patch would get a thumbs up from me, perhaps also with an addition to the frontpage checklist of things to do when setting up a site, saying something like 'you may now wish to allow users register for your site by changing the /Public registration/ option on the User settings page'.
Often when a site is started, even if it's a community site, you wouldn't want to let people in until you were ready to launch it anwyay, so the default is good in my opinion.
Comment #13
gaele CreditAttribution: gaele commentedThe patch from #11 worked for me. The wording of the options is already very extensive, so I don't see a need for any further explanation.
Attached is a new version in case this gets committed: user registration settings: more logical order.
Comment #14
mdupontJust stumbled upon the issue. I frequently set up small websites with Drupal and with the time I had overlooked this aspect. As a result, one of my sites (by luck, one in development) was left with the default setting of "Everybody can register without anyone being notified"... This is a dangerous default behavior.
I'm 100% supporting a patch like the above, or at least that the default be changed to "Visitors can create accounts but administrator approval is required."
Comment #15
alanburke CreditAttribution: alanburke commentedComment #16
alanburke CreditAttribution: alanburke commentedLet the testbot have a look
Comment #17
MichaelCole CreditAttribution: MichaelCole commentedTestbot is ignoring the patch. Probably because it's from 2007.
This is probably dead code, but possibly not a dead issue.
Comment #18
Dave ReidMerging in some duplicate issues:
#698144: D7: Fresh install allows visitors to create accounts
#794892: Change default account creation setting to "Administrators only"
Comment #19
rfayWe should get this taken care of.
Comment #20
rfayComment #22
rfayI guess there are quite a few tests that assume the (former) default.
Comment #23
iantresman CreditAttribution: iantresman commentedSecurity should trump the historical default.
People who know what to do, will configure who can create accounts. Novices, who are initially unaware that such an option exists, will be glad that their site is not left open to anonymous sign-ups.
Comment #24
rfayOK, hopefully the bot will like this one. It just adds the traditional default to the tests that failed in #19.
Comment #26
rfayOne more try. I think I have them all now.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedThis is not a security issue - there is nothing insecure about allowing people to sign up for accounts on your site.
As we discussed in one of the duplicate issues, the default install allows people to sign up for accounts and then to write comments and upload user profile pictures... however, this isn't an issue of security; it's one of expectations. Plus, only other registered users can see these by default (so your site won't be a particularly effective spam engine if people try to use it that way), and they're easy for an administrator to delete. (Note also that the latest patch at #364159: Enable 'access comments' permission for anonymous users by default. would enable comment moderation for registered users from the get-go.)
Considering that one of the biggest strengths of Drupal is that it allows you to build community websites, we should be pretty wary of turning this off by default. I can sort of see the argument for why it should be the default value of the variable (in the user.module itself) and therefore inherited as such by the Minimal profile, but for the Standard install profile to do this too the argument seems much weaker. I think one of the primary points of the Standard install profile is for people to get a quick sense of what Drupal is and the main things it has to offer, and user registrations are a big part of that.
The suggestions/patches earlier in the issue of making this an option in the installer are kind of an interesting compromise, although anyone who suggests adding yet another option to the installer comes under great scrutiny these days (and rightfully so) from anyone who is interested in usability :)
Comment #28
rfay@David_Rothstein, I disagree with you on a practical level. On many sites, and probably by default "authenticated user" has some privileges that the site owner did not intend to grant to unknown people. Thus new site, unintended users, unintended privileges.
Comment #29
gregglesI apologize for only skimming the above comments. If I missed something important please find me in irc and point me to it.
At the OWASP conference in DC in 2009 I talked to a lot of "security" folks and they all said "why would you allow this by default?" And then at the security BOF in San Francisco Micah Tapman mentioned this as something that should be changed on every site.
In Colorado 2 years ago there was a state political candidate who had a site built for her where the authenticated role had full administrative permissions and where this default ability to create accounts was still present, essentially allowing anyone who knew how to find /user to become site admin. A person who happened to be a political adversary used that to put her site offline which was both a favor to her because it prevented other attacks and a real nuisance (background on this site).
So, I would say that this is generally perceived as a security issue and that we have real world data that it created security vulnerabilities on a site.
My preference would be to require administrative approval. It adds to security while alerting an admin if/when someone tries to create an account. That seems safe enough while still giving context appropriate information in the case someone is signing up rather than just an "access denied."
Comment #30
rfayAs discussed above, the install profile needs to be set, as well. This patch adds that.
Comment #31
rfaygreggles makes a strong argument for allowing visitors to create accounts, but with Admin approval. This prevents the security side of this issue, but is softer toward the traditional community-centered nature of Drupal. A site by default would allow visitor account creation, but they wouldn't be able to do anything until the admin approves. There will still be some confusing spam accounts to clean up, but this is a smaller step away from the traditional default, and stays closer to Drupal's values.
Changing the title.
New patch that changes the default to "Visitors, with admin approval".
Comment #32
iantresman CreditAttribution: iantresman commentedIf I'm not expecting people to be able to sign up by default, then I consider this to be a security issue, even if it is not in the usual sense.
Comment #33
Dries CreditAttribution: Dries commentedI'm OK with this patch in principle, although I don't really consider it a security issue. But let's do it right:
We should be using constants for this instead of integer values.
In documentation we write 'administrator' instead of 'admin'.
Comment #34
rfay@Dries, all of the code in all of core uses the raw integer values (and always has). My temptation was just to use string values for the variable, but that would break compatibility for upgrade. Your approach preserves compatibility, but it was not the intent of this humble patch to fix Drupal's approach to this variable. That said, I agree with you, we should fix this ugliness. I guess a new constant definition can go into common.inc, even though this is a user.module variable. Do you have a better suggestion of where the constant definition should go?
Comment #35
catchThe constant should go into user.module. Unless for some reason it's required earlier than user.module is loaded, which I really hope it isn't.
Comment #36
rfayHere's a patch addressing #33, using constants instead of integer values.
Comment #38
rfayJust a typo, but took me about 20 minutes to get it :-(
This one should pass the bot.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedI guess this compromise setting makes some sense - it has a bit of the benefits of both approaches. At some point, we really need to figure out what the purpose of our install profiles are, though, as it would make these kinds of things easier to decide :) Pretty much anything can be construed as a "security risk" for certain types of sites; it all depends on what they do with the site after they install it.
Anyway, the patch:
The #options array should be keyed by the new constants - i.e.,
USER_REGISTER_ADMINISTRATORS_ONLY => t('Administrators only'), USER_REGISTER_VISITORS => t('Visitors')
, etc.(I have no idea what adding these constants has to do with this issue by the way, except for the "while we are touching some of the same lines of code" factor... but it's a good idea anyway.)
Each of these should get its own docblock.
Comment #40
rfayThis patch addresses the excellent comments in #39. #3 there was an error on my part.
David_Rothstein++
Community Review++
Comment #41
catchI think we need a 6-7 update which keeps the existing variable default for sites which upgrade, but don't have this explicitly set. Either that or display a message on update that the default is being changed and they might want to review it.
Comment #42
gregglesI suggest we go with the first idea (keeps the existing variable default for sites which upgrade, but don't have this explicitly set) to minimize changes to the UI.
Comment #43
rfayThis adds a user_update_N() to set the user_register variable to the D6 default if it was unset. Other than that, it's exactly the same as #40.
Comment #44
rfayDo you think this looks good to go? Would appreciate either a comment or an RTBC. Seems like pretty simple stuff.
Comment #45
gregglesSure, seems like a great idea to me - let's RTBC.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedOops, lost track of this one, but yeah, the latest patch looks good.
However, I'm still not understanding something about the changes to the install profiles in this patch... If USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL is the new variable default used throughout the user module, then why do the install profiles have to bother calling
variable_set('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)
? It seems like that doesn't accomplish much except store an extra row in the database...Comment #47
rfay@David: Reading the issue it looks like I put the install profile variable setting in as a response to your #27, but it kind of looks like I misunderstood what you were saying. However, I don't think it does any damage. IMO it's probably better to have it explicitly in the variable table than just implicitly in the code in a thousand places. For example, a contrib module may still have a different default in the variable_get() (logintoboggan for example). Better to have it explicit.
Comment #48
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I guess that makes sense. Probably not a precedent we want to set for all variables, but this is kind of an important one :)
Setting this to "needs work" for documentation... seems like the new constants as well as the new default value for the variable should be documented in the module upgrade guide?
Comment #50
rfayI'll put it there.
The more important thing is to notify end users of the change. I wonder how we do that.
Comment #51
rfayAdded to the D6->D7 upgrade guide, http://drupal.org/update/modules/6/7#changed_user_default
Comment #52
Stevel CreditAttribution: Stevel commentedMarked #723124: make the possible values of user_register explicit as a duplicate
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedI updated http://drupal.org/update/modules/6/7#changed_user_default a bit to also refer to the new defined constants that were introduced here.