Request for feedback on this code

Regular Joined: Aug 06, 2008 Posts: 65

Hello!
Im working on a custom module, and I was hoping that some of the security heads around here would take a look at a few codes, and tell me what you think about sanitation, etc..

Thanks very much for your consideration, and any help!

Article: (Im wondering about the $bodytext, IF NEEDED and HOW a check_html would be incorporated into that.

Code:




if ($articleid) {


$articleid = intval($articleid);





// Ask the SQL for the article's information


$sql = 'SELECT sid, aid, title, time, bodytext, counter FROM ' .$prefix. '_stories WHERE sid=\'' .$articleid. '\'';


$result = $db->sql_query($sql);


while ($myrow = $db->sql_fetchrow($result)) {





$author = stripslashes($myrow['aid']);


$title = stripslashes(check_html($myrow['title'], 'nohtml'));


$time = $myrow['time'];


formatTimestamp($time);


setlocale(LC_TIME, $locale);


ereg ("([0-9]{4})-([0-9]{1,2})-([0-9]{1,2}) ([0-9]{1,2}):([0-9]{1,2}):([0-9]{1,2})", $time, $datetime2);


$datetime2 = strftime(""._DATESTRING2."", mktime($datetime2[4],$datetime2[5],$datetime2[6],$datetime2[2],$datetime2[3],$datetime2[1]));


$datetime2 = ucfirst($datetime2);


$bodytext = stripslashes($myrow['bodytext']);


$counter = intval($myrow['counter']);





echo 'content goes here';





}





} else {


echo 'give them something else';


}

Links thing:

Code:




// ASK - for some statistics


$healthylifenum = $db->sql_numrows($db->sql_query('SELECT * FROM ' .$prefix. '_stories WHERE topic=\'4\''));


$links1num = $db->sql_numrows($db->sql_query('SELECT * FROM ' .$prefix. '_links_links WHERE cid=\'3\''));


$links2num = $db->sql_numrows($db->sql_query('SELECT * FROM ' .$prefix. '_links_links WHERE cid=\'4\''));


$links3num = $db->sql_numrows($db->sql_query('SELECT * FROM ' .$prefix. '_links_links WHERE cid=\'5\''));





// SECURITY CHECK - Sanitize the results:


$healthylifenum = intval($healthylifenum);


$links1num = intval($links1num);


$links2num = intval($links2num);


$links3num = intval($links3num);





$healthlinks = $links1num + $links2num + $links3num;


$healthlinks = intval($healthlinks);

Most Online:

Code:




// START - most online at one time


$guest_online_num = $db->sql_numrows($db->sql_query("SELECT * FROM ".$prefix."_session WHERE guest='1'"));


$member_online_num = $db->sql_numrows($db->sql_query("SELECT * FROM ".$prefix."_session WHERE guest='0'"));





//SECURE


$guest_online_num = intval($guest_online_num);


$member_online_num = intval($member_online_num);





$who_online_num = $guest_online_num + $member_online_num;





// SECURE


$who_online_num = intval($who_online_num);





// ASK - For Most Visitors Online At Once:


$result = $db->sql_query('SELECT module, hits FROM ' . $prefix . '_module_hits WHERE module=\'full_site_online\'');


$myrow = $db->sql_fetchrow($result);


$mostonline = $myrow['hits'];


$mostonline = intval($mostonline);





if ($who_online_num > $mostonline) {


$db->sql_query("UPDATE ".$prefix."_module_hits SET hits=$who_online_num WHERE module='full_site_online'");


}

This read more... thing needs fixing:

Code:

 $description2 = ereg_replace("_", " ", $description);


    if(strlen($description2) > 0) {


   $description2 = substr($description,0,250);


         $description2 .= "...";

What is the difference between:

Code:

echo 'content';

AND

Code:

echo 'content'."\n";

AND

Code:

echo "content";

OR

Code:

echo "content\n";

and what is safest?

Posted: Fri Mar 20, 2009 8:13 am

I don't pretend to be a great expert on this but here's my input.

If articleid is being posted from a form then I would do a

Code:

if isset[$_POST['articleid'] {$articleid = intval[$_POST['articleid'] } else {$articleid = ''}

with appropriate syntax including semicolons. That way you know $articleid is set. You could put the current logic that executes after the if($articleid) inside the isset of the post variable also.

As to doing check_html's on things coming out of the database, you shouldn't need to. They should be sanitized going in. I also don't think you need to do all that stuff with formatting time, it should be stored in the database in time format. Take a look at article.php in the /modules/news directory in your RN distribution for a coding example (although that does a check_html on title for some reason also).

Also if you are doing a numrows on fields the result will intrinsically be returned as a integer so you don't need to doing intvals on the variables.

As to echoing content, you probably want to put a line break after it, in which case "echo 'content'."\n";" will work. But the line break is not a security issue.

Posted: Fri Mar 20, 2009 10:11 am

Hey thanks for your help! Anyone else have any feedback also?

Involved Joined: Jul 03, 2006 Posts: 273

Although fkelly is somewhat correct in saying you shouldn't need to clean stuff on the return from SQL I myself feel you should. Although rare there are ways to corrupt or exploit via the return so as extra safety measure I always like to filter my sql returns as well. I use the Filter function which in turn uses the checkhtml function I am not sure why hardly any nuke coders use it since it is there and does a couple extra steps ontop of checkhtml? I mean it does the adding and stripping of slashes for you so why type more than you have to?

Posted: Sun Mar 22, 2009 6:40 pm

duck wrote:

Although fkelly is somewhat correct in saying you shouldn't need to clean stuff on the return from SQL I myself feel you should. Although rare there are ways to corrupt or exploit via the return so as extra safety measure I always like to filter my sql returns as well. I use the Filter function which in turn uses the checkhtml function I am not sure why hardly any nuke coders use it since it is there and does a couple extra steps ontop of checkhtml? I mean it does the adding and stripping of slashes for you so why type more than you have to?

Can you please show me how to do that in the code? Thank you!

Posted: Mon Mar 23, 2009 4:55 am

Excellent post. Timely too. So just to make sure I have got this correct....Whould someone give an example of a correctly santizied form post $var.

Lets say we have a form that posts $foo. What all needs to be done to it before it goes into the database?

I am pretty sure I am NOT doing it correctly...but this is how I have beenn doing it.

Code:

$reaction = check_html($_POST['reaction'],nohtml);

Thank You for your time!

Dawg

Posted: Mon Mar 23, 2009 6:16 am

In my opinion (non-authoritative of course) the only generic answer that can be given to your question Dawg is that the field has to be properly sanitized or filtered. Beyond that you need to look at the type of the field and the values you want to allow in. What do I mean by that:

[list=]if a field is a checkbox then it can be "on" or not present. You just need to check to see if the $_POST variable corresponding to that field is set or not and set any normal variable that corresponds to either null ('') or "on". For safety, even if the $_POST variable isn't set, you should set the corresponding "normal" variable to ''. The same thing goes for radio buttons.

If the field can take on a specific set of values, say from a select box, then you should check to make sure it is one of those values. Don't run the more generic check_html ... if the field is one of the values in your list of options and you don't have html in that list then it can't have html in it anyway. So why waste the processing cycles.

If the field is an integer then do a intval on it. That automatically filters out any html so you don't need check_html. However, if you only allow certain numeric values then check for those instead. If you say "$myinput == 1 or $myinput == 2" you don't need to intval it -- either of those values is an integer.

Likewise, for text type fields, if you have a specific set of values then you can check to make sure that the input is one of those. If you want to allow "any" input but not allow html then run check_html with the nohtml parameter. If want safe html then run check_html without the nohtml parameter. Be aware that, beyond what you see on the surface, the fckeditor is doing certain filtering, NS is doing filtering of POST strings and within check_html it is calling kses.php which has a whole filtering library built in.

[/list]

Posted: Mon Mar 23, 2009 6:49 am

fkelly,
Thank You very much! I 100% understand what you are saying but the devil is always in the details. I have read and read and read about this topic but everyone seems to do it different. From my viewpoint....if it is good enough for the RN team....then it will be good enough for my little MODs. (I have a really cool one in the works for Drag Racing)

May I ask for one more brain pick...

You discussed a check box, a select box, an interger and a text field.

Would you be kind enough to show "a Properly sanitized" example of each.

Then after you get done I going to ask that you make a "Sticky" out of this thread.

Thank You very much for your time!

Dawg

Posted: Mon Mar 23, 2009 7:16 am

Dawg: okay. What I will do is paste in a block of code that I use for sanitizing form input in a paypal module I run. Basically what I do is a loop on ALL POSSIBLE input from any form that feeds into my "paypal initiation" program so that ANYTHING that gets passed to paypal or is used in paypal calculations is sanitized. If someone, somehow, manages to "forge" the form or put inappropriate values in -- it really doesn't matter because they will most likely just cause the program to die with an error message.

The key is that I take the time up front to create an array of fields that can be passed in from the form(s) and I specify the name, type, filter and maximum length for each field and then check each input against this.

Take a look and post any questions back. As I say, even though I am fortunate enough to be a member of Raven's team, this is by no means "authoritative" nor has it had peer review so before we make it a sticky we ought to get the blessing of other experts. And I am sure there may be other ways to do the same thing that are more efficient or more secure.

With those caveats:

Code:

// all variables that are passed in from any screen should be listed here


$fields = array( array ( 'name' => 'what',


                  'type' => 'hidden',


                  'filter' => 'oneof',


                  'maxl' => 24),


            array   ( 'name' => 'choice',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 14),


            array   ( 'name' => 'lastname',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


            array   ( 'name' => 'firstname',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


            array   ( 'name' => 'address1',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


            array   ( 'name' => 'address2',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


                array   ( 'name' => 'city',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


                array   ( 'name' => 'state',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 2),


                array   ( 'name' => 'zipcode',


                  'type' => 'text',


                  'filter' => 'integer',


                  'maxl' => 5),


                  array   ( 'name' => 'uid',


                  'type' => 'text',


                  'filter' => 'integer',


                  'maxl' => 6),


                array   ( 'name' => 'rec_id',


                  'type' => 'text',


                  'filter' => 'integer',


                  'maxl' => 6),


                array   ( 'name' => 'addentries',


                  'type' => 'text',


                  'filter' => 'integer',


                  'maxl' => 6),


                array   ( 'name' => 'country',


                  'type' => 'text',


                  'filter' => 'text',


                  'maxl' => 35),


                array   ( 'name' => 'phone',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 12),


                array   ( 'name' => 'email',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 50),


            array   ( 'name' => 'pay',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 12),


            array   ( 'name' => 'paymentstatus',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 12),


            array   ( 'name' => 'mtype',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 25),


            array   ( 'name' => 'expiration',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 10),


            array   ( 'name' => 'username',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 25),


            array   ( 'name' => 'employerm',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'donation',


                  'type' => 'text',


                  'filter' => 'integer',


                  'maxl' => 5),


            array   ( 'name' => 'artist',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'artist',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'poet',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'performer',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'other',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'otherdesc',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 30),


            array   ( 'name' => 'exhibition',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'gift',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'office',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'auction',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'accounting',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'fundraising',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'expertise',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 3),


            array   ( 'name' => 'howoften',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 8),


            array   ( 'name' => 'fsgmember',


                  'type' => 'radio',


                  'filter' => 'oneof',


                  'maxl' => 3),


            array   ( 'name' => 'artist',


                  'type' => 'checkbox',


                  'filter' => 'oneof',


                  'maxl' => 2),


            array   ( 'name' => 'offer',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 35).


            array   ( 'name' => 'contact',


                  'type' => 'text',


                  'filter' => 'nohtml',


                  'maxl' => 35)


            );


/*foreach($_POST as $key => $value) {


   $a = $key;


   $aa = htmlentities($value);


   echo $a . ' ' . a . '<br />';


}


*/


$arrsize = count($fields);


for ($row = 0; $row < $arrsize; $row++) {


   $type = $fields[$row]['type'];


   $namef = $fields[$row]['name'];


   $filter = $fields[$row]['filter'];


   $maxl = $fields[$row]['maxl'];


   if (isset($_POST[$namef])) {


      if  ($type == 'checkbox' || $type == 'radio') {


            $$namef = $_POST[$namef]; }


      if ($type != 'checkbox' && $type != 'radio') {


         $$namef = trim($_POST[$namef]);


      }


      if ($filter == 'nohtml') {


         if (strlen($$namef) > $fields[$row]['maxl']) {


            die('system error:  max length of field exceeded'); }


         $$namef = htmlentities($$namef);


      }


      if ($filter == 'integer') {


         $$namef = intval($$namef);


      }


      if ($type == 'checkbox') {


         if ($$namef != 'on') {


            die ('inappropriate value in checkbox field for ' . $namef);


         }


      }


      if ($fields[$row]['filter'] = 'oneof') {


         if ($namef == 'what') {


            if ($$namef != 'membership' AND $$namef != 'superhero' AND $$namef != 'fiber' AND $$namef != 'reart' AND $$namef != 'explore' AND $$namef != 'memexh' AND $$namef != 'animals' AND $$namef != 'time' AND $$namef != 'arriva' AND $$namef != 'home' AND $$namef != 'mad' AND $$namef != 'fun' AND $$namef != 'whodone' AND $$namef != 'seasons' AND $$namef != 'pets' AND $$namef != 'nursery' AND $$namef != 'ticking' AND $$namef != 'musical') {


                die ('inappropriate value in choice of what your are purchasing');


            }


         }


         if ($namef == 'choice') {


            if ($$namef != 'admin_update' AND $$namef != 'admin_delete' AND $$namef != 'admin_add') {


                die ('inappropriate value in administrative update');


            }


         }


         if ($namef == 'pay') {


            if ($$namef != 'paypal' AND $$namef != 'mail check' AND $$namef != 'update') {


               die ('inappropriate value in choice of payment type');


            }


         }


         if ($namef == 'paymentstatus') {


            if ($$namef != 'pending' AND $$namef != 'paid') {


               die ('inappropriate value in choice of payment status');


            }


         }


         if ($namef == 'expertise') {


            if ($$namef != 'no' AND $$namef != 'yes') {


               $expertise = 'no';


            }


         }


         if ($namef == 'howoften') {


            if ($$namef != 'weekly' AND $$namef != 'monthly' AND $$namef != 'one-time' AND $$namef != 'none') {


               $howoften = 'none';


            }


         }


         if ($namef == 'fsgmember') {


            if ($$namef != 'yes' AND $$namef != 'no') {


               $fsgmember= 'no';


            }


         }


         if ($namef == 'mtype') {


            $ch = array('Active Artist', 'Supporting Artist', 'Family', 'Student / Senior', 'Business', 'Donor', 'Patron', 'Benefactor', 'Visionary', 'Sustaining Visionary');


            if (!in_array($$namef, $ch))  {


               die ('inappropriate value in choice of member type');


            }


            else {


               if ($$namef == 'Active Artist') {


                  $amount = '35';


               }


               if ($$namef == 'Supporting Artist') {


                  $amount = '50';


               }


               if ($$namef == 'Family') {


                  $amount = '75';


               }


               if ($$namef == 'Student / Senior') {


                  $amount = '25';


               }


               if ($$namef == 'Business') {


                  $amount = '100';


               }


               if ($$namef == 'Donor') {


                  $amount = '135';


               }


               if ($$namef == 'Patron') {


                  $amount = '250';


               }


               if ($$namef == 'Benefactor') {


                  $amount = '500';


               }


               if ($$namef == 'Visionary') {


                  $amount = '1000';


               }


               if ($$namef == 'Sustaining Visionary') {


                  $amount = '2500';


               }


            }


         }


      }


   }


   else {


      $$namef = ''; // this will assure that any non posted "variables" are at least initialized 


   }


}


// radio buttons not selected


if (empty($expertise)) {


   $expertise = 'no';


}


if (!empty($howoften)) {


   $howoften = 'none';


}

Posted: Mon Mar 23, 2009 9:16 am

Can anyone tell me if there is any security flaws in this:

Code:




$description2 = ereg_replace("_", " ", $description); 


    if(strlen($description2) > 0) { 


   $description2 = substr($description,0,250); 


         $description2 .= "...";

Still very interested in knowing any other security / sanitization needs for this:

Code:




if ($articleid) { 


$articleid = intval($articleid); 





// Ask the SQL for the article's information 


$sql = 'SELECT sid, aid, title, time, bodytext, counter FROM ' .$prefix. '_stories WHERE sid=\"' .$articleid. '\''; 


$result = $db->sql_query($sql); 


while ($myrow = $db->sql_fetchrow($result)) { 





$author = stripslashes($myrow['aid']); 


$title = stripslashes(check_html($myrow['title'], 'nohtml')); 


$time = $myrow['time']; 


formatTimestamp($time); 


setlocale(LC_TIME, $locale); 


ereg ("([0-9]{4})-([0-9]{1,2})-([0-9]{1,2}) ([0-9]{1,2}):([0-9]{1,2}):([0-9]{1,2})", $time, $datetime2); 


$datetime2 = strftime(""._DATESTRING2."", mktime($datetime2[4],$datetime2[5],$datetime2[6],$datetime2[2],$datetime2[3],$datetime2[1])); 


$datetime2 = ucfirst($datetime2); 


$bodytext = stripslashes($myrow['bodytext']); 


$counter = intval($myrow['counter']); 





echo 'content goes here'; 





} 





} else { 


echo 'give them something else'; 


}

Thanks!

Posted: Mon Mar 23, 2009 9:24 am

In the first case all you've done is replace an underscore with a space and substrung the first 250 characters and appended three periods. It doesn't filter out any "dangerous" html that might be injected into descriptions, such as a script tag. In the second case $articleid will be an integer so you can't have any dangerous code in it. But I would use an isset test instead of if($articleid) and if it's coming from a form I would do the test against the $_POST['articleid'] and I'd make sure the variable was set to null if it is not POSTED.

registered · Posted: Wed Mar 25, 2009 8:51 am

duck wrote:

Although fkelly is somewhat correct in saying you shouldn't need to clean stuff on the return from SQL I myself feel you should. Although rare there are ways to corrupt or exploit via the return so as extra safety measure I always like to filter my sql returns as well. I use the Filter function which in turn uses the checkhtml function I am not sure why hardly any nuke coders use it since it is there and does a couple extra steps ontop of checkhtml? I mean it does the adding and stripping of slashes for you so why type more than you have to?

I respectfully disagree. One of my biggest pet peaves as an admin is that I cannot post whatever HTML I wish to in order to do what I am needing to do. If you let an admin post what they should be allowed to post, then what good is it to strip it all out coming out of the database? If someone has found a hole somewhere to inject bad SQL or XSS or ___, then I have bigger problems to deal with.

I personally do not like the filter function either as it is indiscriminate in its stripslashes and addslashes. It pays no attention to whether magic quotes are on or off. Therefore, you can end up stripping slashes that were really intended on being there - such as d:\somedir\somefile.

I'll be honest with you, the whole filtering within *nuke is terrible. It really needs to be re-done and we've been discussing this internally for RN for quite some time. We're still dealing with far too many legacy bandaid solutions.

registered · Posted: Wed Mar 25, 2009 9:37 pm

Defense in layers would suggest all input and output would be filtered from all data sources. I see the need for a filter function - but one that is more specific for each data source. Nuke's current one is flat out bad and does not handle many cases.

Posted: Wed Mar 25, 2009 10:28 pm

Actually Montego, Palibin and I worked before on a version of the filter and check_html functions where we modified it to allow the function to be sent alternate arrays of allowable html for cases like filtering forum html which normally uses a different array. Within it we built bypass functions and such as well and I think it was quite effective although as you say not the perfect solution. Especially since php5 on has new filtering functions that could also be utilized which are not. But still in the end it gave us more flexibility to to filtering incoming and outgoing html the way we wanted. But I was also talking about validating all data back from the DB what should be int is checked to be int what should be no html is checked to be none etc things like that are proper safety protocol I believe cause just because the returned data is supposed to be what came from your db doesn't guarantee it is. But as for the filter function itself in terms of text or html as stands your right needs improvement even more than what Palbin and I had done.

Posted: Thu Mar 26, 2009 7:23 am

Duck, the one thing to watch for is the role of kses.php. While we were working on the Content Plus module for 2.3.01 we found cases where kses was stripping out valid html codes. It thought that "color:" was a bad protocol (like ftp: or http:) for some reason. Kses is very opaque to anyone who is not very familiar with it -- I know I spent hours looking at what was going on and still didn't understand everything it was doing. So, you call check_html and it passes things through kses and you don't know what the heck you have. Plus kses was last updated in 2005 and there is no place to go if you suspect a bug in it except to fix it yourself.

As to "defense in layers" ... I think it all depends. If the layers are consistent and coordinated and comprehensible and easily adjustable then yes. If they are a synonym for "fractured filtering" then no. We have defense in layers in the current RN distribution and I don't think anyone thinks it is an ideal situation.