Problem/Motivation

In Drupal 8 and earlier, it is possible for a user to create a path alias that clobbers an essential core route such as "admin" or "user/login". For example, a malicious user with permission to create content using an unfiltered text format and to "Create and edit URL aliases" could override the "user/login" path with a node crafted to look like it but with an HTML form that posted the submission to another URL that harvested the login credentials. Of course less serious abuses can be easily imagined, such as merely rendering certain admin pages inaccessible.

Proposed resolution

Perhaps the path alias system should, in its validation handler(s) (validation logic is unfortunately duplicated in various places, depending on what interface you create the alias through), make sure that a requested path is not already defined. That could be a UX improvement, too. But then I don't know if that conflicts with behavior that might be considered a feature.

Remaining tasks

Come to consensus on how to handle the issue.

User interface changes

TBD.

API changes

TBD.

Files: 
CommentFileSizeAuthor
#39 drupal.sanitize_aliases_121362-39.patch4.57 KBJoshuaRogers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.sanitize_aliases_121362-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 drupal.sanitize_aliases_121362-37.patch5.59 KBJoshuaRogers
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 26 fail(s), and 0 exception(s).
[ View ]
#27 drupal.sanitize_aliases_121362-27.patch3.61 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,101 pass(es).
[ View ]
#22 drupal.sanitize_aliases_121362-22.patch3.55 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,937 pass(es).
[ View ]
#21 drupal.sanitize_aliases_121362-21.patch3.55 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,934 pass(es).
[ View ]
#15 drupal.sanitize_aliases_121362-15.patch1.94 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,794 pass(es).
[ View ]

Comments

jp.stacey’s picture

Category:bug» feature

I'm not sure precisely what feature you're after from the first paragraph - do you want it to be impossible to set those paths, or do you want path.module to set them aside somehow (to define them seperately beforehand)?

Regardless, the standard Apache mod_rewrite config prevents Drupal from intercepting requests to existing files. Apache checks to see if a system file or directory exists: if it does, Drupal never gets a look-in.

I'm changing this to feature request as I don't see any buggy behaviour here: if you set a path to an existing file, the existing file is still served up by the webserver, isn't it?

JHeffner’s picture

Yes, I'd consider it a feature request at this point. It would be nice if path.module wouldn't allow a user to use a url of a known "reserved" path used by the product.

Robin Monks’s picture

Just to make it clear, this is reserved as in, "won't work due to how apache handles rewrites" and not reserved as in "a core system URL" like admin, feed, etc. The first one I think would be a great addition, since I have run into that problem once before myself. The second wouldn't not be a good idea, since I have often needed to replace core URLs with my own pages as needed.

So, it's a +/-1 :)

Robin

JHeffner’s picture

@Robin, right.. I had to use the second method several places as well.

Dave Reid’s picture

Title:Reserved paths with clean urls» Do not allow aliases to be created for existing or reserved paths
Version:5.1» 7.x-dev

Bumping to 7.x for a feature request. Also marking #366275: 403 on alias 'sites' as a duplicate of this issue.

Dave Reid’s picture

Title:Do not allow aliases to be created for existing or reserved paths» Do not allow existing or reserved paths as aliases
Dave Reid’s picture

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

Bumping to D8 since its too late for D7.

scor’s picture

subscribing. #22336: Move all core Drupal files under a /core folder to improve usability and upgrades should help with making Drupal's path less likely to be used as URL aliases.

a6hiji7’s picture

This issue is also there in D6. Will be nice if a solution is provided. I have run into this issue while creating a French site. There is a page titled "Thèmes" which is the French word for "Issues". The path for the page became "themes" which conflicts with the Drupal themes directory name and I am getting a "403 Forbidden error". Since clients want the path to remain the same (as we are migrating their old site keeping the paths same) it has become a big problem for me.

Dave Reid’s picture

Merging in the following issues as duplicates:
#803382: Manually entered path can override another Drupal internal path
#1018960: Add hook_path_validate() API
#757732: Overriding Drupal paths

Not really part of the original issue but goes straight along with it is we want to limit the ability for users with just the 'create url aliases' permission to make URL alises for system paths like 'node', 'user', or 'admin'.

bfroehle’s picture

I've drafted up a D7 module Path restrict which essentially uses a method equivalent to hook_admin_paths() to determine if a URL alias should be allowed.

Edit: Created a sandbox project on Drupal for the code (instead of the GitHub repository that was previously linked).

Robin Monks’s picture

We still want to make sure we can get this into D8 core as well.

betamos’s picture

I was really surprised when I discovered this issue. Isn't this a big security implication? The only required permission to mess up a Drupal site big time is "Create and edit URL aliases" (which are often given to simple content editors etc).

Then it's just to start hijacking critical core paths like "admin/config", "user/login" with e.g. a link to a malicious site.

I don't know what would be the ideal solution but perhaps protecting existing system paths, router items (like "node/%") and the files directory. Btw, isn't it weird that aliases are prioritized higher than system paths when a page request is made?

Dave Reid’s picture

JoshuaRogers’s picture

Assigned:Unassigned» JoshuaRogers
Status:Active» Needs review
StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [MySQL] 35,794 pass(es).
[ View ]

I've added some validation logic to the path module. It checks that the alias does not already map to another location, does not refer to a file on the filesystem, and does not begin with 'node'.

Robin Monks’s picture

Is it a "file system path" or an "item on the file system"? When I think of path I think more of directory. Also; should your wildcard check also apply to users?

Just thinking aloud; the rest of the patch looks good on visual inspection.

/Robin

JoshuaRogers’s picture

An item on the file system is definitely more accurate; it checks against both files and folders since either one would preempt Drupal.

You're right about users. Possibly taxonomy as well. Maybe I should make it check against the installed modules rather than keep a list? That would make sure that it matched taxonomy/, user/, node/, etc.

Robin Monks’s picture

Yeah; the ultimate solution to this would be to be smart enough to parse though hook_menu; that could make a substantial performance hit, however.

Lars Toomre’s picture

I would recommend creating this check in its own function that then can be called from the form validation function. This then would allow unit testing (rather than a web test) and it also can be used when entities are created via code (like Feeds).

JoshuaRogers’s picture

Status:Needs review» Needs work

Sounds good. I'll work on breaking that code out, adding some unit tests, and addressing the issues from #16.

JoshuaRogers’s picture

Status:Needs work» Needs review
StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 35,934 pass(es).
[ View ]

Okay, I think that I've addressed the issues that were brought up. I ended up using a DrupalWebTestCase instead of a Unit test, however, since part of the validation logic does require the database.

JoshuaRogers’s picture

StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 35,937 pass(es).
[ View ]

Rerolling

marcingy’s picture

Status:Needs review» Needs work
<?php
} else {
    
// If it isn't a duplicate, it still might be invalid.
   
$valid = path_alias_is_valid($path['alias']);
?>

should be

<?php
}
else {
   
// If it isn't a duplicate, it still might be invalid.
   
$valid = path_alias_is_valid($path['alias']);
?>

This would be nice as a variable to allow sites to reverse additional name spaces for other entities

<?php
$restricted_paths
= array('node', 'taxonomy', 'user');
?>

Again this would be nice as a variable in case a site has an additional standalone file that bootstraps drupal etc

<?php
$restricted_files
= array('install.php', 'cron.php', 'update.php');
?>
Robin Monks’s picture

The variable approach does seem sound; especially if you have forum software or the like living under Drupal's base domain scope and want to avoid conflicts. It would need to be documented in the relevant places for inclusion in a site-specific settings.php file.

JoshuaRogers’s picture

That makes sense. I'll try to roll that up this evening.

Robin Monks’s picture

Another quick niggle; I believe all of those form errors /should/ end with a period to make them correct structurally.

/Robin

JoshuaRogers’s picture

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
PASSED: [[SimpleTest]]: [MySQL] 35,101 pass(es).
[ View ]

Well, it seems that my concept of "this evening" really meant "two nights from now." ;) Either way, I've taken the advice from #23 and #26. I have not yet added the documentation to the patch, but intend to do that soon. In the mean time, here is the rerolled patch.

attiks’s picture

Status:Needs review» Needs work
+++ b/core/modules/path/path.moduleundefined
@@ -179,10 +179,58 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  ¶

white space

attiks’s picture

Status:Needs work» Needs review

back to NR

bfroehle’s picture

Status:Needs review» Needs work
+++ b/core/modules/path/path.moduleundefined
@@ -179,10 +179,58 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+ * @return
+ *   TRUE if the alias is valid, otherwise the error message.
+ */

I think this interface of returning TRUE if it is valid and a string (which converts to TRUE) if it is not valid is incredibly confusing.

That is:

if (path_alias_is_valid($alias)) {
  // do something... always ?!
}
JoshuaRogers’s picture

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 35,227 pass(es).
[ View ]

Rerolled with #28 and #30.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

This is looking good to me.

sun’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // We could create an alias that would conflict with another node/user/taxonomy
+  // later on, so we need to take account of that. (e.g. Create node 7
+  // with the alias 'node/9/delete'. It will make it impossible to delete
+  // node 9 later on without modifying node 7 first.) We pull it from configuration
+  // so that the user can modify it in their settings.php.
+  $restricted_paths = variable_get('restricted_path_aliases', '/(?:node|taxonomy|user)(?:\/|$)/i');
+  if (preg_match($restricted_paths, $alias)) {
+     return FALSE;
+  }

This breaks a built-in feature of the URL alias functionality: It is perfectly valid to create an alias node/123 for a new node ID 500. Especially useful when node/123 existed before, but got deleted and did not have an alias. Thus, node/500 exists on node/500 as well as on the former node/123 path (but the latter merely as an alias).

+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // Verify that no other module has reserved this address
+  $router_item = menu_get_item($alias);
+  if (!empty($router_item)) {
+    return FALSE;
   }

This breaks a built-in feature of the URL alias functionality: Intentionally overriding a router path through a URL alias.

+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // Before invoking Drupal, the webserver will first check the local
+  // filesystem for files/folders that match the path, so Drupal cannot
+  // match any path that would match an item on the filesystem.
+  // Additionally, .htaccess redirects some D7 paths to their D8 counterparts
+  // (e.g. cron.php). Those have to be enumerated and checked seperately.
+  $restricted_files = array('install.php', 'cron.php', 'update.php');
+  if (file_exists($alias) || in_array($alias, $restricted_files)) {
+    return FALSE;
+  }

I don't think this is necessary.

JoshuaRogers’s picture

@sun You make some good points. The first two use cases showed why an administrator might want to create /node/7 or /user/login. It still might be problematic to give those rights to a site contributor though. Since aliases are enabled by default, a disgruntled contributor could effectively make the site unusable for a period by aliasing /user and /user/login. An admin without experience building HTTP requests or eiting databases might not know how to login to recover the site.

Would it be worth considering another permission that would be required for aliases that could break the site?

As for the last bit of code listed, I created a node with an alias "myfile.txt". When I attempted to view the node, the .htaccess file sent me to the file on my local filesystem instead. In this case it seems a bit misleading to let the user use the alias.

Thanks for the review @sun. :)

geerlingguy’s picture

Subscribe; would love to have the path_alias_is_valid() handy, as I basically had to write it on my own for a project today. Useful for more than just core stuff.

JoshuaRogers’s picture

Category:feature» bug

The problem with the existing functionality is that any user can create a page that take precedence over one of the default pages if they have the permission "Create and edit URL aliases". If that permission were marked with a warning of any type, it might not be a problem. As it is though, a novice administrator could find most of their users unable to login simply by giving a seemingly safe permission to a restricted role.

If we are to leave this functionality in place, could we split the "Create and edit URL aliases" permission into "Create and edit URL aliases" and "Override core URL aliases"? That would still provide the functionality while allowing a more granular set of permissions for users.

-- Edit --

So, I completely forgot (and overlooked) that I had already replied to this one. Rerolling a new patch. /embarassed.

-- Edited Again --

So, I had read the "I don't think this is necessary" as applying to the whole patch. I'm a mostly asleep idiot ATM. #drupalcon

JoshuaRogers’s picture

Status:Needs work» Needs review
StatusFileSize
new5.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 26 fail(s), and 0 exception(s).
[ View ]

Rerolled using all of the advice from @sun.

Status:Needs review» Needs work

The last submitted patch, drupal.sanitize_aliases_121362-37.patch, failed testing.

JoshuaRogers’s picture

Status:Needs review» Needs work
StatusFileSize
new4.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.sanitize_aliases_121362-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hmm. @sun didn't give the advice that the tests should fail though. That was all my idea.

JoshuaRogers’s picture

Status:Needs work» Needs review
JoshuaRogers’s picture

StatusFileSize
new4.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,498 pass(es).
[ View ]

The last submitted patch, drupal.sanitize_aliases_121362-39.patch, failed testing.

JoshuaRogers’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 39: drupal.sanitize_aliases_121362-39.patch, failed testing.

scor’s picture

Issue summary:View changes
TravisCarden’s picture

Assigned:JoshuaRogers» Unassigned
Issue summary:View changes
Related issues:+#118769: Reserved paths with clean urls

The security team has discussed this internally and come to the consensus that the issue can be handled publicly, since any phishing or access escalation vectors that would be exacerbated by this behavior would already be exploitable without it. The chief concern therefore is one of stability--that users either unwittingly or maliciously could create aliases that render certain core functionality unusable. Here are the questions I can think of that still need to be answered:

  • What should the system do when a user tries to create an alias at a taken path?
  • What constitutes a taken path (e.g., core routes? module routes? directories? physical files?), and how will they be detected?
  • What about an alias that preexists a new route, such as an alias that conflicts with a later added contrib module route when that module is enabled?
  • And finally, to @geerlingguy's point in #35, can/should we expose the test via an API function that contrib modules can use, too?

Note: I'm unassigning the issue since it's been silent for two years.

scor’s picture

Issue tags:+Security improvements
mbaynton’s picture

I think the main use case for aliases doesn't require them to have the overarching capabilities to override things that they have now.

The suggestion in #36 to have a 'safe' url alias permission more suitable for untrusted content authors, and a restricted 'override' URL alias permission makes sense to me as a way to retain backwards compatibility while fixing the security problems. But, I wonder if attempting to validate whether a particular alias requires the 'override' permission is the right approach. If aliases are stored in a way that the permission used to create them is recorded, I think it's just a matter of when in the "routing" process which aliases are applied. Current aliases, and any aliases built using the 'override' permission continue to impact the path before the fist attempt to find the matching route. New aliases built using the 'safe' permission are only applied when no route/controller was found on the first pass, one step before returning a 404. If a 'safe' alias matches at this stage, the route matching/controller locating logic is run again on the de-aliased path.

Here are the questions I can think of that still need to be answered:

What should the system do when a user tries to create an alias at a taken path?

For 'override' aliases, happily allow as is done today. For 'safe' aliases, apply the same logic used to decide if a path matches something when a request is received, and if the new alias already matches something, fail the form validation so the user can choose another one.

What about an alias that preexists a new route, such as an alias that conflicts with a later added contrib module route when that module is enabled?

This is a strength of this approach; pre-existing 'safe' aliases are superseded by the new route. There could always be path conflicts in cases like adding a module, but I think this is a better resolution strategy than we have today.

And finally, to @geerlingguy's point in #35, can/should we expose the test via an API function that contrib modules can use, too?

The only test for 'safe' aliases should be whether a request to the path would return a 404 (alias is available), or something other than a 404, (alias is not available).

The UI on content creation would change to create 'safe' aliases; legacy aliases with overarching power to override things would be relegated to the administrative aliases UI (and hopefully, would have precious few items on new sites.) This plan would align very well with #1553358-40: Figure out a new implementation for url aliases, which also suggests moving towards a new alias storage model, with the old one left intact but becoming uncommon, and having most alias-creating UI manipulating the new alias entities.