The user.module function user_roles contains a query to fetch roles with a certain permission from the permissions table, joined with the roles table. The permission is searched for using a LIKE query which is not the safest for a comma-separated list.

$result = db_query("SELECT r.* FROM {role} r INNER JOIN {permission} p ON r.rid = p.rid WHERE p.perm LIKE '%%%s%%' ORDER BY r.name", $permission);

The problem is that when you call the function like this (taken from the upload.module admin_settings):
$roles = user_roles(0, 'upload files');

You could also get the roles returned with permissions like 'dont upload files', 'upload filesystem'

I would suggest to change the way the permissions are stored in the permission table (comma separated, but without a space after the comma) and to change the query to:

$result = db_query("SELECT r.* FROM {role} r INNER JOIN {permission} p ON r.rid = p.rid WHERE FIND_IN_SET('%s',p.perm) ORDER BY r.name", $permission);

Comments

mcarbone’s picture

mcarbone commented
Version: 5.3 » 6.x-dev

This is definitely a bug, and I almost relied it on it while writing a patch for another issue (http://drupal.org/node/195161).

Also, this bug affects Drupal 6.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

Bumping to critical, although it's theoretical afaik, there's all kinds of potential gotchas here.

catch’s picture

catch
he/him
Primary language English
commented
Title: Notice: possibly unsafe/incorrect return from user_roles function » User permissions query uses LIKE
hswong3i’s picture

hswong3i commented

-1 for FIND_IN_SET: it seems to be a MySQL specific SQL function. I am not able to search its document for PostgreSQL and Oracle.

mcarbone’s picture

mcarbone commented

Is there a good solution for this that doesn't involve refactoring the permission table? I can think of several involving using REGEXP, or other non-trivial MySQL functions, but I imagine they would be considered bad practice. Other solutions would involve adding delimiters to the beginning and end of the string, so that LIKE can be used, but that doesn't seem ideal.

chx’s picture

chx commented
Priority: Critical » Minor

Nothing is broken here. This is a gotcha not a bug. We were fine with this, it'd be great if fixed but if went five or i dunno how many stable series like this, we can continue.

catch’s picture

catch
he/him
Primary language English
commented

This seems fair enough. There's no actual example of this breaking anything, if a contrib module did it, we could file an issue against that module.

mcarbone’s picture

mcarbone commented

Hmm, I'm not so sure I agree with making this minor. Working on another patch that involved the 'post comments' and 'post comments without approval' (http://drupal.org/node/195161), I almost relied on this LIKE -- and then I realized that if this bug was ever fixed, my patch would break. In other words, one bug can lead to another.

In any case, I agree that it's not critical. Certainly D6 can launch with this if D5 has had it for a year. But I don't think that just because a function hasn't worked as advertised for a year that we might as well let it continue to not work. I think the priority should be 'normal,' but I won't bother switching it unless others agree.

teezee’s picture

teezee commented

FIND_IN_SET is MySQL specific.

Does anyone know why currently permissions are stored with comma's followed by a space? Is this done deliberately to be able to search for 'role name,' (note the comma). This is not the nicest way to work but neither is the LIKE part.

The user_admin_perm function saves the permission names. The following comment can be found there:
// Compile role array:
// Add a comma at the end so when searching for a permission, we can
// always search for "$perm," to make sure we do not confuse
// permissions that are substrings of each other.

I think the simplest thing to do here is to change the original query to:

$result = db_query("SELECT r.* FROM {role} r INNER JOIN {permission} p ON r.rid = p.rid WHERE p.perm LIKE '%%%s,%%' ORDER BY r.name", $permission);
mcarbone’s picture

mcarbone commented

teezee, what about this case?

"move files, remove files,"

Doing a LIKE '%move files,%' will still satisfy both. It can work that way only if a comma is put at the beginning of the permission string as well.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Priority: Minor » Normal

IMHO not minor. What we do at other places where a LIKE query is required on comma separated data sets, is rather ugly. We need to look for both prefixes and suffixes, ie. the following cases:

- only permission in permission list: column = '%s'
- permission in the beginning: column LIKE '%s,%%'
- permission in the middle: column LIKE '%%,%s,%%'
- permission at the end: column LIKE %%,%s'

So you OR all these together and get a long conditional. Don't tell me it is ugly, I know.

You can "help" this with CONTACT(',',column,',') LIKE '%%,%s,%%' instead, so you add the commas in place for "easier" matching. BUT this introduces an expression instead of the column name, so each column will need its value passed through an expression. This generally looks good, because the field cannot be indexed, but this column cannot be indexed anyway...

In this case, we have 2-10 rows I guess. I don't think people have more then 10 roles, but they might have a few more. Whether four conditionals OR-ed, or an on the fly expression performs better here can be measured.

Another option is to store the column with the leading and trailing comma, which makes the LIKE nicely controllable, ie it becomes column LIKE '%%,%s,%%'. Then the loading logic needs to strip this off whenever a PHP array is created. This is a slight API modification, but gives us best performance. Would also need an update function.

JirkaRybka’s picture

JirkaRybka commented

A nitpick: We have one space after each comma, so it needs to be either column LIKE '%%, %s,%%' with comma+space added to start of stored string, or the storage format changed to remove that space.

teezee’s picture

teezee commented

@mcarbone, abolutely correct, is missed that one!
@Gábor Hojtsy, I agree that using a comma-prefix is (under current circumstances) the best way for now

In my view, the way these strings are stored is not the right one. I would like to suggest the following:

Saving the permission names is done in user_admin_perm_submit(), the query is built up like this:

db_query("INSERT INTO {permission} (rid, perm) VALUES (%d, '%s')", $role->rid, implode(', ', array_keys($form_values[$role->rid])));

A few things to change:

user.module / user_admin_perm()

while ($role = db_fetch_object($result)) {
- $role_permissions[$role->rid] = $role->perm .',';
+ $role_permissions[$role->rid] = $role->perm;

foreach ($role_names as $rid => $name) {
  // Builds arrays for checked boxes for each role
-  if (strpos($role_permissions[$rid], $perm .',') !== FALSE) {
+  if (strpos($role_permissions[$rid], ','.$perm.',') !== FALSE) {
    $status[$rid][] = $perm;
  }
}

user_admin_perm_submit()
Change the query creation in user_admin_perm_submit() to:

db_query("INSERT INTO {permission} (rid, perm) VALUES (%d, '%s')", $role->rid,
	(count($form_values[$role->rid]) ? ','.implode(',', array_keys($form_values[$role->rid])).',' : '')
);

In this way, if any perms exist, they are stored like so: ',perm1,perm2,perm3,'

Change the LIKE part of the queries in user_roles() and user_filter()

LIKE '%%,%s,%%'
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

teezee: at first look, these changes look fine. A few comments:

- user_admin_perm() also has a sizable comment on why the comma was there, so remove that too
- an update function is definitely needed to add these commas to existing roles in an upgrade
- a bit of a lookaround for more places where this change might need to be done

Taran’s picture

Taran commented

Just curious to hear from those closer to the code...

Do you think this bug could have caused this problem? :

http://drupal.org/node/201843

teezee’s picture

teezee commented

@Taran #15
Nope, that was just a missing Anonymous user (uid 0 in users table must be available).

This issue is about the incorrect way permissions are stored

teezee’s picture

teezee commented
Title: User permissions query uses LIKE » User permissions stored and checked wrongly
Version: 6.x-dev » 5.6

I found another tricky situation in user_access(). There, the same check is performed using the comma.

Use case:
There is a test.module which implements permissions 'move files' and 'remove files'. This is just a test case but what if you want to give a user permissions to remove files, but you do NOT want them to move files. The user_access function will return true in both cases!

I changed the version to Drupal 5.6 because this problem occurs (as far as i've been able to check) all through version 5.

mcarbone’s picture

mcarbone commented
Version: 5.6 » 7.x-dev

This affects 7.x-dev as well.

(teezee: The version should be set to the most recent version it affects, not the earliest, and then fixes can be backported as needed.)

mcarbone’s picture

mcarbone commented
Version: 7.x-dev » 6.x-dev

Scratch that. It has been fixed in D7: #73874: Normalize permissions table

I'll set to Drupal 6.x-dev, but with the fix in D7 being fairly non-trivial, maybe this should just become a won't fix.

manikan’s picture

manikan commented

Change the following line in user_admin_perm function in user/user.admin.inc

From:
if (strpos($role_permissions[$rid], $perm .',') !== FALSE) {

To:
if (strpos( ", ". $role_permissions[$rid], ", ". $perm .',') !== FALSE) {

Status: Active » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.