Why are these examples of dangerous code?

Heine —Thu, 2008/10/09 - 10: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));
}

Example II

function example_two($account) {
// ...
$account = user_load(array('uid' => $uid));
}

Comments

because it don't take care

Submitted by Anonymous (not verified) on Thu, 2008/10/09 - 11:51

because it don't take care of permissions

$uid undefined on Example II

Submitted by flevour (not verified) on Thu, 2008/10/09 - 11:52

I have no clue why these examples would be dangerous, curious to know the answer.

I think the first example

Submitted by someone (not verified) on Thu, 2008/10/09 - 12:00

I think the first example has a lot of code, so there could be definitve a

global $user;

So that the current user gets the author of the node, perhaps the admin

No error handling

Submitted by Nicholas Thompson (not verified) on Thu, 2008/10/09 - 13:03

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

Submitted by Anonymous (not verified) on Thu, 2008/10/09 - 13:08

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

Submitted by Mir Nazim (not verified) on Thu, 2008/10/09 - 14:59

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-by-mir-nazim-drupal-camp-india-2008

Example of dangerous code

Submitted by Gordon (not verified) on Thu, 2008/10/09 - 14:02

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

Submitted by Heine on Thu, 2008/10/09 - 14:48

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.

Submitted by xamox (not verified) on Thu, 2008/10/09 - 14:08

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.

Access permission checking is almost useless here

Submitted by asiby (not verified) on Sun, 2009/11/29 - 21:46

I 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!

Submitted by Andrew Berry (not verified) on Thu, 2008/10/09 - 15:19

I 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 $user call within itself, so it doesn't become global for anything which includes that file!

I think if() statement is

Submitted by Kuldip (not verified) on Fri, 2008/10/10 - 06:09

I think if() statement is necessary at there
because if you have no value in $nid or $uid then it gives for each error

Heine