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
- Adds a setting for restricted path alias patterns
- Adds a validation constraint to enforce it
- Adds a permission to bypass it that admins can use
Remaining tasks
- Decide if this is a bug or a task, the presence of new API makes me think its a task
- Reroll the patch at #67 as an MR
- Document the new setting in default.settings.php
- Add test coverage
- Review
- Commit
User interface changes
None
API changes
New validation constraint and permission
Release notes snippet
Todo
Comment | File | Size | Author |
---|---|---|---|
#75 | 2023-06-14_104934_screenshot.png | 34.68 KB | tgoeg |
#75 | 2023-06-14_102307_screenshot.png | 96.71 KB | tgoeg |
#67 | 121362.patch | 5.2 KB | eiriksm |
#60 | Screen Shot 2019-03-09 at 01.33.12.png | 225 KB | johnwebdev |
#39 | drupal.sanitize_aliases_121362-39.patch | 4.57 KB | Anonymous (not verified) |
Issue fork drupal-121362
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
jp.stacey CreditAttribution: jp.stacey commentedI'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?
Comment #2
JHeffner CreditAttribution: JHeffner commentedYes, 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.
Comment #3
Robin Monks CreditAttribution: Robin Monks commentedJust 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
Comment #4
JHeffner CreditAttribution: JHeffner commented@Robin, right.. I had to use the second method several places as well.
Comment #5
Dave ReidBumping to 7.x for a feature request. Also marking #366275: 403 on alias 'sites' as a duplicate of this issue.
Comment #6
Dave ReidComment #7
Dave ReidBumping to D8 since its too late for D7.
Comment #8
scor CreditAttribution: scor commentedsubscribing. #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.
Comment #9
a6hiji7 CreditAttribution: a6hiji7 commentedThis 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.
Comment #10
Dave ReidMerging 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'.
Comment #11
bfroehle CreditAttribution: bfroehle commentedI'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).
Comment #12
Robin Monks CreditAttribution: Robin Monks commentedWe still want to make sure we can get this into D8 core as well.
Comment #13
betamos CreditAttribution: betamos commentedI 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?
Comment #14
Dave ReidMarked #1020412: Detect files on file system when preventing name collisions as a duplicate of this issue.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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'.
Comment #16
Robin Monks CreditAttribution: Robin Monks commentedIs 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
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedAn 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.
Comment #18
Robin Monks CreditAttribution: Robin Monks commentedYeah; the ultimate solution to this would be to be smart enough to parse though hook_menu; that could make a substantial performance hit, however.
Comment #19
Lars Toomre CreditAttribution: Lars Toomre commentedI 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).
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedSounds good. I'll work on breaking that code out, adding some unit tests, and addressing the issues from #16.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedOkay, 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.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolling
Comment #23
marcingy CreditAttribution: marcingy commentedshould be
This would be nice as a variable to allow sites to reverse additional name spaces for other entities
Again this would be nice as a variable in case a site has an additional standalone file that bootstraps drupal etc
Comment #24
Robin Monks CreditAttribution: Robin Monks commentedThe 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.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedThat makes sense. I'll try to roll that up this evening.
Comment #26
Robin Monks CreditAttribution: Robin Monks commentedAnother quick niggle; I believe all of those form errors /should/ end with a period to make them correct structurally.
/Robin
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedWell, 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.
Comment #28
attiks CreditAttribution: attiks commentedwhite space
Comment #29
attiks CreditAttribution: attiks commentedback to NR
Comment #30
bfroehle CreditAttribution: bfroehle commentedI 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:
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled with #28 and #30.
Comment #32
attiks CreditAttribution: attiks commentedThis is looking good to me.
Comment #33
sunThis 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).
This breaks a built-in feature of the URL alias functionality: Intentionally overriding a router path through a URL alias.
I don't think this is necessary.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commented@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. :)
Comment #35
geerlingguy CreditAttribution: geerlingguy commentedSubscribe; 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.Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled using all of the advice from @sun.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedHmm. @sun didn't give the advice that the tests should fail though. That was all my idea.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #43
Anonymous (not verified) CreditAttribution: Anonymous commented#41: drupal.sanitize_aliases_121362-39.patch queued for re-testing.
Comment #46
scor CreditAttribution: scor commentedComment #47
TravisCarden CreditAttribution: TravisCarden commentedThe 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:
Note: I'm unassigning the issue since it's been silent for two years.
Comment #48
scor CreditAttribution: scor as a volunteer commentedComment #49
mbayntonI 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.
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.
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.
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.
Comment #52
osmansecurity vs. convenience...
I am having a similar issue with @a6hiji7 #121362-9: Do not allow existing or reserved paths as aliases
I wanted to have a page called Themes, with an alias of
/themes
. It automatically saved as/themes-0
Are there any workarounds?
Comment #53
TravisCarden CreditAttribution: TravisCarden as a volunteer commentedSorry, @osman.
/themes
isn't even a reserved path--it's a physical directory. So you're not fighting Drupal but Apache. It might be possible to do something in.htaccess
, but it would be a complicated, non-standard hack.Comment #55
catchDiscussed this with other committers on a triage call in relation to #2203573: Path alias validation and tests duplicative, inconsistent and decided since there's a number of duplicate issues of this opened over the years, we should raise the priority to 'major'.
Comment #59
johnwebdev CreditAttribution: johnwebdev commentedOld, but still an existing issue. I built a sandbox https://www.drupal.org/sandbox/johndevman/3038709 where I added a new constraint "ExistingPath" to the PathItem. Still needs some more test coverage, but seems to do the trick so far.
Comment #60
johnwebdev CreditAttribution: johnwebdev commentedComment #61
icgurney CreditAttribution: icgurney commentedThis is still an issue, and I hope one of these patches gets applied soon.
Comment #64
anrikun CreditAttribution: anrikun commentedI'm bumping this as it is really a major issue.
Comment #65
xjmEveryone who's hoping that this can get committed could help by updating the patch to work on 9.3.x. :) (The code is very different than it was in 2013 so it would not merely be a reroll.) Thanks!
Comment #66
eiriksmWorking on a re-roll
Comment #67
eiriksmHm, re-read the comments a bit, and probably a re-roll is not what we want?
Quick re-roll except the test is here. And did a change discussed below.
Have we reached a consensus about the following?
- Should we disallow node/33 alias for a node when a user does not have a new "safe" permission?
- Should we disallow other paths than a hardcoded list of paths?
- Should we make it possible to configure the list of paths? I think we should, just a slight change to the latest patch anyway.
Also: If we allow it to be configured, maybe we should allow it to be configured a bit more flexible than just the first part of the path?
Unassigning since I do not have time to address my own feedback right now. But here's a (re)start.
Comment #68
bkosborneOof, just ran into this when I was debugging an issue where the media library widget on a site stopped working! After lots of xdebugging, turns out that it was because the AJAX request the widget makes to /media-library was overwritten by a page on the site with the same URL!
Comment #69
quietone CreditAttribution: quietone as a volunteer commentedThe proposed resolution section of the IS doesn't make it clear what changes are being implemented. Can someone update that.
This needs to be on 9.3.x now.
Comment #71
mxr576Comment #75
tgoeg CreditAttribution: tgoeg commentedJust lost my (lengthy) comment because of a Firefox crash, I'll keep it shorter, ask for details if needed, please!
I do think this should be handled as a security issue.
With a respective setup for example, even unauthenticated users might take over core paths on sites where some comment functionality or similar might create aliases (e.g. Path Auto setup to make human readable aliases of comments' URLs by using the title or first few words as pattern (this is a common feature you see on the Internet). I also don't see how an editor can create a legit looking
/user/login
site without this "feature".Ran into this with a site using permissions by term. A node with alias
/batch
had been created. Afterwards, a user was created, permissions by term then drops all privileges and rebuilds them. In batches :). Guess what happened. No rebuilt permissions as the alias caught the request to/batch&id=132
, site completely broken.The security issue alone should warrant a "critical" priority I think, even more now that I see this is postponed to 11.x which means this issue that's already 6 years old will have to wait a few more for a fix.
As stated above, we need to come to a consensus how this should be solved before updating the IS. Reading through this whole thread shows a few different use-cases I try to cover.
Here's my suggestion:
This, together with the dynamically built list, also solves the problem of existing white-/blacklists and URLs that get added afterwards.
This should implicitly answer all the questions in https://www.drupal.org/project/drupal/issues/121362#comment-14103203 with "Yes, configurable as outlined in the mockup".
A completely different strategy might be to explicitly trigger a request to the alias that shall be added and determine whether it is free by the answer received.
Comment #76
catch@tgoeg "I do think this should be handled as a security issue."
Handling this as a security issue would mean unpublishing this issue, opening a new issue in the private security tracker, and then any solution having to go through the core security release process. Given this issue has been open for 16 years in the public queue that is not going to stop anyone finding out about it. It's also very hard/impossible to do security releases with significant behaviour changes like a new configuration page, which is something that would normally only be introduced in a minor release, not to (at this time) 9.4.x
I think there's an argument this allows users in certain configurations to DOS a site though, so agreed with making it a critical bug.
Comment #77
tgoeg CreditAttribution: tgoeg commentedThe 'net does not forget, as they say.
I totally agree with you. I did not want to change the issue administration-wise, but reflect that there definitely is a security aspect to it, which should not be taken lightheartedly (which I felt was the case, reading the above comments).
If a critical priority reduces the time to fix this, this should be enough. Knowing that 9.x and even 10.x will remain without a fix however feels pretty uncomfortable. I think we should at least strive for a patch that applies to 10.x if it really is too late for any changes in this major series..
Comment #78
catchThe change could land in a minor release (i.e. the next one would be 10.2.x now that 10.1.0 is at rc stage), it just won't land in a patch release if it's adding new configuration and a UI for it.
Comment #79
larowlanThis is now the oldest open bug in core.
Taking a look at it for Bug Smash Initiative.
What is being added here looks like a new API - so perhaps this should be a task.
In that case we would call it 'Add a restricted patterns setting to limit what can be used as path aliases'
The current patch does the following:
* Adds a setting for restricted path alias patterns
* Adds a validation constraint to enforce it
* Adds a permission to bypass it that admins can use
I think this is a reasonable approach to getting something in - this issue has been open for 17 years.
The next steps are:
* Reroll the patch as an MR
* Add test coverage
* Document the setting in the default.settings.php file.
Does anyone feel strongly about keeping this as a bug?
Comment #82
pameeela CreditAttribution: pameeela commentedI came here to see whether there was any discussion of bug vs task :)
Oddly enough, just last week we had a use case to redirect
/user/register
because they are about to launch a new site and wanted to pause registrations. A redirect to a content page explaining the situation was by far the easiest way to achieve this, because it was linked from many places across the site.So this combined with the fact that it generally feels like an add-on leads me to task.
Edit: Oops, realised this applies specifically to aliases, not sure whether it changes the equation...
Comment #83
catchYeah I bumped this to critical in #75, because I think the example given that certain combination of permissions are able to do worse things due to this issue than they otherwise would be is a good one, but I also think it's a task. There's no functional bug as such, this was intended behaviour when it was added, it's just that this issue wants to change that behaviour to make things more locked down.