Are you looking for us to answer your question? If not, please just post your opinion; your example isn't obvious and we'd rather discuss the security concepts than play games. Thanks!
Example 1. If there is a global $user before the user_load() then it will change the user to user of the node. Maybe we should look at removing the current user from global and use a function to get a copy of the $user object.
Example 2. This is a PHP 5 issue. Since $account is past down, loading up the new object with may pass it back to the calling function.
When you review code and find something like Example I, you need to check for the absence of a global $user. I recommend to always change $user to $account when not referring to the current user object as code in Example I is very vulnerable to issues when later modified. SA-2008-061 is an example of this vulnerability.
Example II is also something that should sound your code reviewer alarm. Whenever an object is passed to the function it will (as a side effect) be changed in PHP 5, where objects are passed by reference. This is quite problematic when the current user object ($GLOBALS['user']) is passed to the function. While one should always take care with parameters passed by reference, user objects are a special devastating case.
My guess is there is no access control check. So someone could pull up a list of nodes they do not have access to. So my guess it needs the user_access() and node_access() functions. I'm curious to hear the answer, I like post like this though, it makes you think, most people just want to be spoon fed the answer. I know your quite the security guru for drupal so it would be interesting to see some post about past security issues, how they are exploits and fixes for them.
I was wondering why the update script would fail, causing me to be logged in as some other user. I ended up fixing it by renaming the $user variable, but I really should have been looking in my include file:
The file is used to define a few constants (it's included so you have PROFILE_COMPANY_NAME), as well as a common array used in forms. The real fix is to put all of that in a function which returns the form, and contains the global $user call within itself, so it doesn't become global for anything which includes that file!
Comments
because it don't take care
because it don't take care of permissions
$uid undefined on Example II
I have no clue why these examples would be dangerous, curious to know the answer.
I think the first example
I think the first example has a lot of code, so there could be definitve a
So that the current user gets the author of the node, perhaps the admin
No error handling
In example 1 you could pass in $nid as "abc". This would cause node_load to fail and therefore user_load would fail too....
Example 2... $uid is undefined... Apart from that nothing dangerous, is there?
Are you looking for us to
Are you looking for us to answer your question? If not, please just post your opinion; your example isn't obvious and we'd rather discuss the security concepts than play games. Thanks!
Reason #1): Toooooooooooo
Reason #1): Toooooooooooo Much stressing the database.
I think this is good enough reason of not doing it.
more here: http://www.slideshare.net/mirnazim/best-practices-for-drupal-developers-...
Example of dangerous code
Example 1. If there is a global $user before the user_load() then it will change the user to user of the node. Maybe we should look at removing the current user from global and use a function to get a copy of the $user object.
Example 2. This is a PHP 5 issue. Since $account is past down, loading up the new object with may pass it back to the calling function.
Correct
You are right.
When you review code and find something like Example I, you need to check for the absence of a global $user. I recommend to always change $user to $account when not referring to the current user object as code in Example I is very vulnerable to issues when later modified. SA-2008-061 is an example of this vulnerability.
Example II is also something that should sound your code reviewer alarm. Whenever an object is passed to the function it will (as a side effect) be changed in PHP 5, where objects are passed by reference. This is quite problematic when the current user object ($GLOBALS['user']) is passed to the function. While one should always take care with parameters passed by reference, user objects are a special devastating case.
Hrm.
My guess is there is no access control check. So someone could pull up a list of nodes they do not have access to. So my guess it needs the user_access() and node_access() functions. I'm curious to hear the answer, I like post like this though, it makes you think, most people just want to be spoon fed the answer. I know your quite the security guru for drupal so it would be interesting to see some post about past security issues, how they are exploits and fixes for them.
Watch out for include files as well!
I just wrote this code yesterday for a quick update, and it serves as a great example for something to watch out for:
I was wondering why the update script would fail, causing me to be logged in as some other user. I ended up fixing it by renaming the $user variable, but I really should have been looking in my include file:
The file is used to define a few constants (it's included so you have PROFILE_COMPANY_NAME), as well as a common array used in forms. The real fix is to put all of that in a function which returns the form, and contains the
global $usercall within itself, so it doesn't become global for anything which includes that file!I think if() statement is
I think if() statement is necessary at there
because if you have no value in $nid or $uid then it gives for each error