WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 3 months ago

#43936 new defect (bug)

Settings: Warn when open registration and new user default is privileged

Reported by: kraftbj Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: administration Cc:

Description

Much like our Strong Passwords work, we can help inform site administrators when their actions may be sub-optimal.

WordPress allows a site owner to open registration AND set the default new user level to "Administrator". While this combination may make sense for some sites (on an intranet?), this is typically a really really bad idea.

We should provide some type of confirmation to ensure site administrators are intending to open their site up.

If registration is open and default level is Subscriber (read cap only), the current behavior is fine. If registration is open and other capabilities are included in the default role, we should have some type of checkbox or "are you sure" notice. "By allowing open registration and a default role of {role}, anyone who can visit the site would have the ability to {have full control of your site|publish content|etc}."

We saw this in the wild on a site in support today :).

Attachments (3)

43936.diff (834 bytes) - added by subrataemfluence 17 months ago.
43936.2.diff (1.8 KB) - added by kraftbj 17 months ago.
43936.3.diff (2.1 KB) - added by kraftbj 17 months ago.

Download all attachments as: .zip

Change History (14)

#1 @subrataemfluence
17 months ago

  • Component changed from Administration to Users
  • Focuses administration added

Good catch!! Rather having a checkbox to "confirm", I would rather suggest to remove the "Administrator" option from the dropdown from this page. Since admin always has the ability to change a user's role he can easily do so by going to that user's profile any time.

I agree that opening Administrator privilege for a new user registration should not be an option anyway!

#2 @subrataemfluence
17 months ago

  • Keywords has-patch added

I have added an additional optional parameter which will allow to pass an array of roles which need to be excluded from the dropdown.

Example:

<?php
wp_dropdown_roles( get_option('default_role'), array( 'administrator', 'editor' ) );

The above roles won't render Administrator and Author in the dropdown.

However, if called like

<?php
wp_dropdown_roles( get_option('default_role') );

All roles will be available in the dropdown like now.

Version 0, edited 17 months ago by subrataemfluence (next)

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


17 months ago

#4 @kraftbj
17 months ago

Thanks for the patch @subrataemfluence! A couple of logistical notes on the patch - It is helpful to run the svn diff from the root so you have the full path (e.g. src/wp-admin/includes/template.php instead of just template.php). That'll make it easier to apply to patch.

Also, be sure your code follow WordPress Coding Standards ( https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/ ). There were a few spacing issues that I noticed.

Submitting a follow-up patch that includes updated inline docs, coding standards compliance, and implements the modified wp_dropdown_roles to resolve the ticket issue.

@kraftbj
17 months ago

#5 follow-up: @kraftbj
17 months ago

A couple of things:

  1. We should add a filter so plugins can add additional roles (e.g. Keymaster) to exclude or to allow more roles (e.g. a private intranet where one _does_ want to allow new users as Admins).
  1. Does it make sense to exclude roles or exclude certain capabilities?

@kraftbj
17 months ago

#6 in reply to: ↑ 5 @subrataemfluence
17 months ago

Thank you for further fine tuning my proposed patch and thumb up for the introduction of the additional filter!

By the way, I forgot to upload the patch for template.php! :P

Replying to kraftbj:

A couple of things:

  1. We should add a filter so plugins can add additional roles (e.g. Keymaster) to exclude or to allow more roles (e.g. a private intranet where one _does_ want to allow new users as Admins).
  1. Does it make sense to exclude roles or exclude certain capabilities?

#7 @subrataemfluence
17 months ago

I would rather suggest this ticket to be labeled as "Feature request" since the current functionality does not have any bug in it.

#8 @roytanck
10 months ago

We've seen a couple of plugin vulnerabilities recently that allowed attackers to set these options, even while unauthenticated.

The obvious attack vector was to enable registration and set the default role to admin. This was not done through the admin settings page, but through manipulated URLs.

Besides not offering the option in the dropdown, I think core should also not add the user if this combination of settings exists.

Personally, I can think of no use case that would require this combination of settings. It's essentially "please take my site".

#9 @desrosj
3 months ago

#46744 was marked as a duplicate.

#10 @dd32
3 months ago

#46744 was closed as a duplicate of this, which I agree with.

The main difference is that this is a warning/only allows selecting safe values in the UI, where #46744 focuses on the malicious setting of options to bad values through a vulnerability that allows setting of options (of which, are common in recent years in plugins).

Preventing a user selecting a dangerous combination is needed, but it also needs to validate that the values in the database are safe to rely upon IMHO
As an example, filter on the default value:

function filter_default_role( $default_role ) {
  // $users_can_register = ....
  if ( $users_can_register && get_role( $default_role )->has_cap( 'manage_options' /* or other cap deemed useful, `publish_posts` could also be used */ ) ) {
    $default_role = 'subscriber'; // Fallback roll for when an unsafe roll has ended up in there
  }
  return $default_role;
}

#11 @desrosj
3 months ago

#46661 was marked as a duplicate.

Note: See TracTickets for help on using tickets.