Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: javanaut Status: patch 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. javanaut Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 15: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 3, 2005 - 17: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 3, 2005 - 19:24 : javanaut Heh, after posting that, I realized what I had done, but never got back around to simplifying it. Good eye. ------------------------------------------------------------------------ February 3, 2005 - 23: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; -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705