This patch fixes an information disclosure flaw as it does not check access properly under certain circumstances. Anonymous users could get access to read information they should not have access to.
For example, it may expose a complete list of existing user accounts (user name and ID) to anonymous users: http://example.com/mpac/autocomplete/menu/users When an attacker knows a username they can start a brute force attack to gain access with that user.
In this patch:
1. Only logged in users can access autocomplete path.
2. Add autocomplete path into admin paths.
3. If users don't have "access user profiles permission", user alias are excluded from results.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | more-user-access-check-2698843-4.patch | 1.14 KB | rimen |
| #2 | more-user-access-check-2698843-1.patch | 1.88 KB | pandaski |
Comments
Comment #2
pandaski commentedComment #3
stborchertPatch looks good, but can we have some simple tests for this?
Comment #4
kurtfoster commentedI've made a minor change here. The patch was using the user_is_logged_in callback, which allowed users without permissions to manage the menu to access the menu. I've changed this back to user access using the administer menu permissions instead so the access is restricted based on permissions.
Comment #5
stborchert@kafmil: "administer menu" as permission is wrong here, since you need to list other stuff than menu items, too (i.e. nodes).
The patch from Joseph looks good but simply needs a test.
Comment #6
stborchertComment #7
kurtfoster commented@stBorchert, the menu lists other things, but the usage should only be from people administering the menu. If I'm logged in as a plain authenticated user I can view node titles and ids of unpublished nodes on that url, bypassing Drupal permissions. Using the administer menu permissions doesn't prevent anything being listed, it simply enforces the drupal permissions system for access to a function that should only be used when administering the menu.
Comment #8
stborchert@kafmil: I checked your patch on a clean install with a user having the permission to administer URL aliases. The user is not able to use MPAC on admin/config/search/path/add because he doesn't have the administer menu permission.
This is not how it should work.
Comment #9
kurtfoster commented@stBorchert, yep, I get it now, sorry, that was roughly hour 15 that day and I wasn't thinking clearly. The other patch works, however I still think it's not completely correct to display uppublished content. It's not a serious security breach, but it is still going around proper permissions.
Comment #10
rimen commentedI think we should use core menu item access check
Comment #11
rimen commentedComment #12
stborchertWhile this looks much better there should be a way to alter the access check so a module could decide to not disable access to some items.
Still needs test ...
Comment #13
rimen commentedI don't think that alter access to menu items is a part of this module
Drupal provides many posibilities to alter menu item access and it depends on the situation
For example for node links custom modules still can use hook_node_access,
or on hook_menu_alter set custom access callback to menu items
Comment #14
astonvictor commentedI'm closing it because the issue was created a long time ago without any further steps.
if you still need it then raise a new one.
thanks