[drupal-devel] [bug] user_access returns invalid data
Project: Drupal Version: <none> Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: javanaut Status: patch 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. javanaut -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal -Version: <none> +Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: Anonymous Status: patch + 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. Anonymous Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 21: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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
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 Heh, after posting that, I realized what I had done, but never got back around to simplifying it. Good eye. 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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: tangent Status: patch 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; tangent Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 16: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 - 18: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 - 20:24 : javanaut Heh, after posting that, I realized what I had done, but never got back around to simplifying it. Good eye. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
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
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: tangent Status: patch Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. tangent Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 16: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 - 18: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 - 20: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 - 00: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 - 09: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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
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 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. 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; ------------------------------------------------------------------------ February 4, 2005 - 08: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 - 08:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: tangent Status: patch 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? tangent Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 16: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 - 18: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 - 20: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 - 00: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 - 09: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 - 09:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 09: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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: Dries Status: patch Would it be possible to implode() the permissions string and to use in_array() ? Dries Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 23: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 - 01: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 - 03: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 - 07: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 - 16: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 - 16:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 16: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 - 18: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? -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: Anonymous Status: patch 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 Anonymous Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 21: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 - 23: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 - 01: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 - 05: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 - 14: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 - 14:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 14: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 - 16: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 - 10:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: Dries Status: patch Fine with me. Dries Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 23: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 - 01: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 - 03: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 - 07: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 - 16: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 - 16:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 16: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 - 18: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 - 12:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 13: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 -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: JonBob Status: patch If we use such an array, we need to use array_key_exists() rather than isset() to avoid strict/PHP5 errors. JonBob Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 16: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 - 18: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 - 20: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 - 00: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 - 09: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 - 09:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 09: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 - 11: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 - 05:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 06: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 - 06:20 : Dries Fine with me. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal Version: 4.5.0 Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: killes@www.drop.org Status: patch JonBob: I don't think so. If the array is defined, then we can use isset to ask if a particular index exists. killes@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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
Project: Drupal -Version: 4.5.0 +Version: cvs Component: user.module Category: bug reports Priority: critical Assigned to: javanaut Reported by: javanaut Updated by: killes@www.drop.org Status: patch Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept. killes@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@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. -- View: http://drupal.org/node/16705 Edit: http://drupal.org/project/comments/add/16705
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@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@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@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@www.drop.org Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept.
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: Dries Status: patch javanaut: can you confirm that #14 is working for you? (This could do with being documented in the code.) Dries Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 23: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 - 01: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 - 03: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 - 07: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 - 16: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 - 16:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 16: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 - 18: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 - 12:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 13: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 - 13:20 : Dries Fine with me. ------------------------------------------------------------------------ February 6, 2005 - 14: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 - 14:55 : killes@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 - 15:09 : killes@www.drop.org Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept. ------------------------------------------------------------------------ March 14, 2005 - 00:25 : killes@www.drop.org 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.
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: javanaut Status: patch Looks functional to me. +1 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; ------------------------------------------------------------------------ February 4, 2005 - 08: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 - 08:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 08: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 - 10: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 - 04:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 05: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 - 05:20 : Dries Fine with me. ------------------------------------------------------------------------ February 6, 2005 - 06: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 - 06:55 : killes@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 - 07:09 : killes@www.drop.org Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept. ------------------------------------------------------------------------ March 13, 2005 - 16:25 : killes@www.drop.org 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. ------------------------------------------------------------------------ March 23, 2005 - 15:33 : Dries javanaut: can you confirm that #14 is working for you? (This could do with being documented in the code.)
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: Dries Status: patch javanaut: I assume you haven't tested it yet. Because you're capable of reproducing the bug, would you mind giving this a try? Dries Previous comments: ------------------------------------------------------------------------ February 3, 2005 - 23: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 - 01: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 - 03: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 - 07: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 - 16: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 - 16:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 16: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 - 18: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 - 12:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 13: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 - 13:20 : Dries Fine with me. ------------------------------------------------------------------------ February 6, 2005 - 14: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 - 14:55 : killes@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 - 15:09 : killes@www.drop.org Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept. ------------------------------------------------------------------------ March 14, 2005 - 00:25 : killes@www.drop.org 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. ------------------------------------------------------------------------ March 23, 2005 - 23:33 : Dries javanaut: can you confirm that #14 is working for you? (This could do with being documented in the code.) ------------------------------------------------------------------------ March 23, 2005 - 23:46 : javanaut Looks functional to me. +1
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: javanaut Status: patch 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 :) 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; ------------------------------------------------------------------------ February 4, 2005 - 08: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 - 08:37 : tangent Permissions are comma delimited aren't they? The comma space in the string would handle this IIRC. ------------------------------------------------------------------------ February 4, 2005 - 08: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 - 10: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 - 04:18 : Dries Would it be possible to implode() the permissions string and to use in_array() ? ------------------------------------------------------------------------ February 6, 2005 - 05: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 - 05:20 : Dries Fine with me. ------------------------------------------------------------------------ February 6, 2005 - 06: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 - 06:55 : killes@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 - 07:09 : killes@www.drop.org Attachment: http://drupal.org/files/issues/user-permissions.patch (2.83 KB) Here is an untested patch as proof of concept. ------------------------------------------------------------------------ March 13, 2005 - 16:25 : killes@www.drop.org 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. ------------------------------------------------------------------------ March 23, 2005 - 15:33 : Dries javanaut: can you confirm that #14 is working for you? (This could do with being documented in the code.) ------------------------------------------------------------------------ March 23, 2005 - 15:46 : javanaut Looks functional to me. +1 ------------------------------------------------------------------------ March 24, 2005 - 01:22 : Dries javanaut: I assume you haven't tested it yet. Because you're capable of reproducing the bug, would you mind giving this a try?
participants (6)
-
Anonymous -
Dries
-
javanaut
-
JonBob
-
killes
-
tangent