Why are these examples of dangerous code?
Heine — Thu, 09/10/2008 - 09:35
Just think about it for a minute before jumping to someone, who has the first one right or Gordon, who nails them both.
Example I
function example_one($nid) {
// Lots of code
$node = node_load($nid);
// ...
$user = user_load(array('uid' => $node->uid));
}
// Lots of code
$node = node_load($nid);
// ...
$user = user_load(array('uid' => $node->uid));
}
Example II
function example_two($account) {
// ...
$account = user_load(array('uid' => $uid));
}
// ...
$account = user_load(array('uid' => $uid));
}
- Login to post comments
because it don't take care
Anonymous (not verified) — Thu, 09/10/2008 - 10:51because it don't take care of permissions
$uid undefined on Example II
flevour (not verified) — Thu, 09/10/2008 - 10:52I have no clue why these examples would be dangerous, curious to know the answer.
I think the first example
someone (not verified) — Thu, 09/10/2008 - 11:00I 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
Nicholas Thompson (not verified) — Thu, 09/10/2008 - 12:03In 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
Anonymous (not verified) — Thu, 09/10/2008 - 12:08Are 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
Mir Nazim (not verified) — Thu, 09/10/2008 - 13:59Reason #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
Gordon (not verified) — Thu, 09/10/2008 - 13:02Example 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
Heine — Thu, 09/10/2008 - 13:48You 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.
xamox (not verified) — Thu, 09/10/2008 - 13:08My 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.
Access permission checking is almost useless here
asiby (not verified) — Sun, 29/11/2009 - 21:46I am convinced that user_access() or node_access() is useless here.
If you wrote a code that can end up being installed as a module, then it is not as if the general public was trying to execute some random PHP. If you need to load a user object, simply avoid the $user object and use $account instead. Also, unless an object is passed by reference to a function on purpose, why the hell would you modify that argument directly? A rule of thumb also is that you should not use a reference to the $user object as function parameter unless you need to modify the $user object inside the function.
I think that this whole thing in pretty basic and boils down to PHP 101.
Cheers
Watch out for include files as well!
Andrew Berry (not verified) — Thu, 09/10/2008 - 14:19I just wrote this code yesterday for a quick update, and it serves as a great example for something to watch out for:
* Add a company column for each person.
* Copy existing company information in to each order
*/
function ec_course_info_update_5001() {
include(drupal_get_path('module', 'ec_course_info'). '/ec_course_info.inc');
$ret = array();
$ret[] = update_sql("ALTER TABLE {ec_course_info_person} ADD company varchar(255) NOT NULL");
// Copy it in for each existing transaction
$q = "SELECT DISTINCT txnid FROM {ec_course_info_person}";
$r = db_query($q);
$i = 0;
while ($txnid = db_result($r, $i++)) {
$txn = store_transaction_load($txnid);
$user = user_load(array('uid' => $txn->uid));
$company_name = $user->{PROFILE_COMPANY_NAME};
if (!empty($company_name)) {
$q = "UPDATE {ec_course_info_person} SET company='%s' WHERE txnid=%d";
db_query($q, $company_name, $txnid);
}
}
return $ret;
}
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:
global $user;
// Fully load the user object so we have profile fields.
$u = user_load(array('name' => $user->name));
empty($u->{PROFILE_COMPANY_NAME}) ? $company_name = '' : $company_name = $u->{PROFILE_COMPANY_NAME};
...
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
Kuldip (not verified) — Fri, 10/10/2008 - 05:09I think if() statement is necessary at there
because if you have no value in $nid or $uid then it gives for each error