[drupal-devel] [bug] user_access returns invalid data

[killes]

Issue status update for http://drupal.org/node/16705

 Project:      Drupal
 Version:      cvs
 Component:    user.module
 Category:     bug reports
 Priority:     critical
 Assigned to:  javanaut
 Reported by:  javanaut
 Updated by:   killes at www.drop.org
 Status:       patch
 Attachment:   http://drupal.org/files/issues/user_access_bug_0.patch (513 bytes)

Updated javanaut's patch according to anonymous' suggestion. A more
thorough approach would be preferable of course.


killes at www.drop.org



Previous comments:
------------------------------------------------------------------------

February 3, 2005 - 22:32 : javanaut

Attachment: http://drupal.org/files/issues/user_access_bug.patch (473 bytes)

The user_access function in user.module returns the results of the
*strstr()* function, which returns a string, not a Boolean like the
documentation suggests.  This was screwing things up for me since
flexinode relies on user_access for it's node_access('create'..)
functionality.
The attached patch uses strpos instead of strstr.  It was created from
a 4.5 codebase, but I noticed that the same issue is in HEAD as well. 
I'm using it on my dev site, and node_access('create'..) calls are now
working properly and nothing I'm using seems to have any problems with
it.


------------------------------------------------------------------------

February 4, 2005 - 00:20 : Anonymous

+  return strpos($perm[$account->uid], "$string, ") !== FALSE ? TRUE :
FALSE;
could be written more simply as
+  return strpos($perm[$account->uid], "$string, ") !== FALSE;
Keep in mind it's also possible to type-cast the result of strstr to a
boolean value (i.e. return (bool) strstr($perm[$account->uid],
"$string, ");)... I am unsure as to whether strstr or strpos would be
faster... perhaps a quick benchmark could answer the question of which
we should use.


------------------------------------------------------------------------

February 4, 2005 - 02:24 : javanaut

Heh, after posting that, I realized what I had done, but never got back
around to simplifying it.  Good eye.


------------------------------------------------------------------------

February 4, 2005 - 06:25 : tangent

The PHP docs claim that strpos is faster and less memory intensive than
strstr.
For what it's worth, the second statement below does not seem more
simple or readable than the first to me though it's hard to say if
casting an int as a boolean is more or less performant than performing
a logical comparison.
return strpos($perm[$account->uid], "$string, ") !== FALSE ? TRUE :
FALSE;
return (bool) strpos($perm[$account->uid], "$string, ") !== FALSE;


------------------------------------------------------------------------

February 4, 2005 - 15:25 : javanaut

The security flaw that this fixes is where strstr finds too man positive
results.  Example:
module A has permission "type A create content" (Allowed for role)
module B has permission "create content" (Disallowed for role)
user_access("create content") is run for module B and returns
non-FALSE, non-null results, even though the role is disallowed. 
Avoiding this would require a permission naming convention such that no
permission were a substring of another.


------------------------------------------------------------------------

February 4, 2005 - 15:37 : tangent

Permissions are comma delimited aren't they? The comma space in the
string would handle this IIRC.


------------------------------------------------------------------------

February 4, 2005 - 15:57 : javanaut

In this example, the search string passed to strstr becomes:
"*create content, *"
The main list of permissions being searched would contain the string:
"type A *create content, *"
..which would match.  This problem may not be resolved by this fix,
btw.


------------------------------------------------------------------------

February 4, 2005 - 17:32 : tangent

While a regular expression would resolve this symptom, this example
demonstrates that using only strings to identify permissions is not
very robust. Is there a reason (performance would be a good one but
only so far) that permissions aren't stored with an index?


------------------------------------------------------------------------

February 6, 2005 - 11:18 : Dries

Would it be possible to implode() the permissions string and to use
in_array() ?


------------------------------------------------------------------------

February 6, 2005 - 12:11 : Anonymous

Dries: Possible, but not desirable. in_array is a rather slow function.
I'd rather put the permissions as keys of an array $perm and use
isset($perm['do something']). We should then probably store serialized
arrays in the table. That is ok, because the table is a sort of cache.
Gerhard


------------------------------------------------------------------------

February 6, 2005 - 12:20 : Dries

Fine with me.


------------------------------------------------------------------------

February 6, 2005 - 13:40 : JonBob

If we use such an array, we need to use array_key_exists() rather than
isset() to avoid strict/PHP5 errors.


------------------------------------------------------------------------

February 6, 2005 - 13:55 : killes at www.drop.org

JonBob: I don't think so. If the array is defined, then we can use isset
to ask if a particular index exists.


------------------------------------------------------------------------

February 6, 2005 - 14:09 : killes at www.drop.org

Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB)

Here is an untested patch as proof of concept.