user_access returns invalid data [#16705]

Comment	File	Size	Author
#14	user_access_bug_0.patch	513 bytes	killes@www.drop.org

#13	user-permissions.patch	2.83 KB	killes@www.drop.org

	user_access_bug.patch	473 bytes	javanaut

Comment #1

(not verified) CreditAttribution: commented 3 February 2005 at 23:20

+ 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.

Log in or register to post comments

Comment #2

javanaut CreditAttribution: javanaut commented 4 February 2005 at 01:24

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

Log in or register to post comments

Comment #3

tangent CreditAttribution: tangent commented 4 February 2005 at 05:25

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;

Log in or register to post comments

Comment #4

javanaut CreditAttribution: javanaut commented 4 February 2005 at 14:25

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.

Log in or register to post comments

Comment #5

tangent CreditAttribution: tangent commented 4 February 2005 at 14:37

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

Log in or register to post comments

Comment #6

javanaut CreditAttribution: javanaut commented 4 February 2005 at 14:57

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.

Log in or register to post comments

Comment #7

tangent CreditAttribution: tangent commented 4 February 2005 at 16:32

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?

Log in or register to post comments

Comment #8

Dries CreditAttribution: Dries commented 6 February 2005 at 10:18

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

Log in or register to post comments

Comment #9

(not verified) CreditAttribution: commented 6 February 2005 at 11:11

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

Log in or register to post comments

Comment #10

Dries CreditAttribution: Dries commented 6 February 2005 at 11:20

Fine with me.

Log in or register to post comments

Comment #11

JonBob CreditAttribution: JonBob commented 6 February 2005 at 12:40

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

Log in or register to post comments

Comment #12

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 6 February 2005 at 12:55

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

Log in or register to post comments

Comment #13

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 6 February 2005 at 13:09

File	Size
user-permissions.patch	2.83 KB

Here is an untested patch as proof of concept.

Log in or register to post comments

Comment #14

killes@www.drop.org CreditAttribution: killes@www.drop.org commented 13 March 2005 at 22:25

File	Size
user_access_bug_0.patch	513 bytes

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

Log in or register to post comments

Comment #15

Dries CreditAttribution: Dries commented 23 March 2005 at 21:33

javanaut: can you confirm that #14 is working for you? (This could do with being documented in the code.)

Log in or register to post comments

Comment #16

javanaut CreditAttribution: javanaut commented 23 March 2005 at 21:46

Looks functional to me. +1

Log in or register to post comments

Comment #17

Dries CreditAttribution: Dries commented 24 March 2005 at 07:22

javanaut: I assume you haven't tested it yet. Because you're capable of reproducing the bug, would you mind giving this a try?

Log in or register to post comments

Comment #18

javanaut CreditAttribution: javanaut commented 24 March 2005 at 13:57

I haven't upgraded the module that manifests this bug to CVS yet. Its functionally is identical to the patch I submitted, and I even tested this revised patch (#14) out on a 4.5.2 installation and the module in question works fine. I can test it out on 4.6rc/cvs, but it may be a small while before I upgrade the module in question. I did run it on a bare CVS installation and it is working fine for me.

Today must be bugfix Friday :)