Filter Inputs--Escaping Outputs

Regular Joined: Jun 02, 2004 Posts: 88

I am trying to get my head around db inputs/db outputs and SQL Injection in the PHP Nuke code (not worried about phpBB). My goal is to get around having to use addslashes and stripslashes everywhere.

Assuming I am only using the MySQL database, can I use the following function for db inputs and outputs if the function was placed in the mainfile.php?

Well? Are my thoughts and code good for stopping SQL Injections?

thanks in advance.....

Code:

  function escape($value, $no_html = 0) {


    if (get_magic_quotes_gpc()) {


      $value = stripslashes($value);


    }


    $value = mysql_real_escape_string($value, $this->dbl);





    if ($no_html) {


      $value = strip_tags($value);


    }


   


    return $value;


  }

Would this function do the same thing?

Code:

function quote_smart($value)


{


    // Stripslashes


    if (get_magic_quotes_gpc()) {


        $value = stripslashes($value);


    }


    // Quote if not integer


    if (!is_numeric($value)) {


        $value = "'" . mysql_real_escape_string($value) . "'";


    }


    return $value;


}

For example, in the modules/Content/admin/index.php in the function content_save there is this line:

Code:

    $text = addslashes(check_words(check_html($text, "")));

Could this be changed to this:

Code:

    $text = escape(check_words(check_html($text, "")));

or this:

Code:

     $text = quote_smart(check_words(check_html($text, "")));

and then it would be safely put into the database?

And then in displaying that specific db input, in modules/Content/index.php in the function showpage there is this line:

Code:

    $mytext = check_html($mypage['text'], "");

Could it be changed to this?:

Code:

    $mytext = escape(check_html($mypage['text'], ""));

so it would be safe from SQL Injection?

---as you can tell, the Content code is from the 3.2b patches----

My bottom line question is, will this approach, if applied to all the other similar PHP Nuke (not phpBB) code, help prevent SQL Injections in all the PHP Nuke code? And, will this stop the slash issue some have with the 3.2b patches?

thanks again in advance...

registered · Posted: Sun Aug 27, 2006 10:57 pm

All I can tell you is that I believe this issue is being looked into for future patches.

If you want a good book on the subject of PHP security, I would highly recommend this one:

Essential PHP Security
Chris Shiflett

He has another one, too, that is a "Pro" version.

Posted: Mon Aug 28, 2006 5:34 am

http://shiflett.org

I got the concept of Filter Inputs, Escape Outputs from his website. And I will be going to the bookstore today. lol....

I had not heard about the "Pro" version.....

Chris Shiflett works on the premise that no user input data can be trusted. To that end he suggests all input data, whether it is from the user or admin, be run thru some kind of validation. I have not checked all of the Nuke code yet, but I do have a feeling there are some places where it is not validated, but I could be wrong...

Along similar lines of thought, he has an article on his website about "Initializing Variables". Yes, some Nuke modules have some initialized variables....

For example:

Code:

$text = "";

OR.... a non-nuke example:

Code:

$validated = false;

--------When followed by something like this: ---------------

Code:

if (validate_user()) 


{ 


    $validated = true; 


}

After reworking Nuke to make it XHTML compliant, I remember instances where variables are not initialized.

Here is a link from Shiflett's site about this: http://shiflett.org/articles/security-corner-may2004

Here is an portion of the article:

(concerning php design...)

If you fail to design with security in mind, you're doomed to be patching security holes for eternity. One primary concern needs to be data filtering, and a good design makes it easy for developers to distinguish safe data from potentially tainted data.

For example, a naming convention can be helpful:

Code:

<?php 


$clean = array; 


 


if (valid_color($_POST['color'])) 


{ 


    $clean['color'] = $_POST['color']; 


} 


?>

A developer can get into the habit of considering everything that's not in $clean (or another safe variable, as mentioned previously) to be tainted. Good habits are worth reinforcing.

Another key to a successful design is to make certain that the data filtering logic cannot be bypassed. Achieving this depends entirely on your design, but if you initialize your variables and enforce a naming convention, any flaw in your design will cause a variable to be empty rather than have an arbitrary value set by an attacker.

.....I was just bringing up some things for discussion.

Posted: Mon Aug 28, 2006 6:35 am

OK

I revised my escape function. Now it tests if magic_quotes_gpc is on, verifies which php version is used, makes sure leading zeros are not deleted, and does the mysql_real_escape_string thing (or mysql_escape_string if php version is less than 4.3.0).

Here is what I have thus far:

Code:

function escape_string($value) {


    // Stripslashes if magic_quotes_gpc is on


    if (get_magic_quotes_gpc()) {


        $value = stripslashes($value);


    }


    // Check php version


    if(version_compare(phpversion(),"4.3.0")=="-1") {


    // Quote if not integer


    if (!is_numeric($value) || $value[0] == '0') {


       $value = "'" . mysql_escape_string($value) . "'";


    } else {


    // Quote if not integer


    if (!is_numeric($value) || $value[0] == '0') {


       $value = "'" . mysql_real_escape_string($value) . "'";


   }


   return $value;


  }

Does anyone see any programmic issues with this?

thanks...

Posted: Mon Aug 28, 2006 7:53 am

You are beyond me in coding expertise Spotted but I will say that the idea of designing from the outset for security is very appealing and necessary. The problem is that trying to retrofit this approach to Nuke is ummmm ... well ... an "oxymoron"? No, not exactly and good steps are being taken in that direction and the ideas you have put forward can perhaps be worked in but it's not like we are working with a clean slate.

It sure would be great to have filters that would distinguish between a word such as u_nion that's used in a normal context and the same word in a context where it's used as part of a hack or that allowed html tags such as body to be used in a post but did a htmlentities on them instead of just giving a security warning.

Posted: Mon Aug 28, 2006 8:40 am

lol.... I am not a coder per se, but I have modified a bunch.... For the above function I took pieces from 3 or 4 different places, and put them all together.

Yes, making code secure so one does not need security walls would be the best. But I am wondering now if some of the coding issues with Nuke are because it tries to be used by every database configuration. The function I listed I think would only work for MySQL databases, however, of course Nuke has others available.

I am not sure how many times I have been thru all the nuke code, line by line, making it pass html compliance tests, but with each time thru, I do learn a little and see where something can be made a little more secure. Amazingly, there were more than one occasion where a form had the method=GET in it.

Smile

I am on a mission now to do the following:
Filter Inputs
Escape Outputs---(not using addslahes)
Initialize Variables
XHTML compliance
CSS warning and error free themes
Using tableless site structure
Multi-browser compliance for the above

I think Nuke code could get fairly secure doing the above. I know the patch team has done some very remarkable work, but unfortunately, they had the burden of trying to make the code compliant for all the databases. I think this limits what could be done with securing the code. .....but I have been wrong many times before.

As I waltz thru the core modules again, I will look for places where htmlspecialchars could be used instead of htmlentities, but I think most of that was done with the 3.2b patch. I will do a sense check at least on this....

Posted: Mon Aug 28, 2006 8:58 am

By the way, the patch team is looking into this already (namely Evaders99). Your approach looks reasonable, however, even Shiflett makes it clear that even mysql_escape_string does not completely protect you... but, then he doesn't give the "end all" script that does. Sad

Unfortunately, PHP-Nuke was not originally designed with security in mind. (Nor did FB's even care to include the patches into the core code!) Therefore, without alot of re-write, we are left with patching...

BTW, all the "mission" items you mentioned are being worked into RavenNuke as it evolves.

registered · Posted: Mon Aug 28, 2006 12:24 pm

phpNuke security is a really a mess. While I think we need a common way to escape data, the attempt in 7.9 so far is flawed. In the ideal case, your escape function would work
1) when get_magic_quotes_gpc is turned on, and the input variable is passed through without using check_html()
2) mysql_real_escape_string is available - for mysql supported systems only
3) but does not provide any protection from rogue tags in code such as JS

For the first one, phpNuke uses stripslashes in various places where it is not needed. This leaves it prone to variables not being escaped correctly - either security issues or fudging the original text (such as windows paths c:\windows\blah\). check_html does not work correctly with addslash'd variables - and it cannot be assumed that they are all run through input with get_magic_quotes_gpc

Second, though highly unlikely, is compatibility with other databases. I'm sure probably no one uses phpNuke on anything other than MySQL. To use the mysql commands, we need to state we are removing support for such database compatibility - or integrate all the database's native escape commands into the database later.

Third, is probably an issue we can leave seperate. Unlike FB's filter() function, we need to handle all conditions seperately to ensure we have a correct filtering. Part of the problem with the Patched files in chatserv's current release is the function order, in part with magic_quotes_gpc, which provided problematic.

------------------------

I was going to start a discussion on this earlier, but here is as good a time as ever.

I want to deal with the magic_quotes issue first. It would be good to know what conditional all input variables come in, so we aren't making redudant stripslashes. From the security books, I get that get_magic_quotes_gpc is something we don't want to rely on. While this may add some security to badly coded scripts, it adds a false sense of security and promotes bad coding.

There are two approaches here:

1) phpBB uses the following: all input is magic_quotes_gpc'd - if not, there is a manual process to make them addslashed.

+ Pros

* Protection on badly coded scripts
* No extra work to add variables to queries for database security concerns

- Cons

* Each variable needs to be tracked where it comes from, and stripslash'd when displayed directly back to browser (such as on a search form)
* Further validation functions needs to take into account any magic_quotes_gpc'd variables
* No immediate code changes need to be done in the query line - may not be obvious that there are security issues

2) The opposite approach: input variables are coming in clean and unsanitized. If get_magic_quotes_gpc is used, reverse it

+ Pros

* The obvious code is added before it is being used (such as in a query), ensuring that all lines use the proper escape
* Variables can be passed for other validation (such as check_html) first without mudging the actual data
* A bunch of stripslashes is not need to make sure it is "database safe" - rather only one at the query line

- Cons
* Variables are not automatically safe to be entered in the database
* All lines must contain the proper escape

My suggestion is the second approach. I am testing code to undo get_magic_quotes_gpc and making sure it works in all cases. That would make the stripslashes in check_html unnecessary. Finally, it would put the burden to using addslashes (or mysql_escape_string) on the query lines... which is what chatserv has been doing at any rate.

I believe this will fix further issues with duplicate slashes and fit in with what the security books recommend.
We do need to discuss issues of filtering (allowing admins control of what is escaped, but that maybe for another post).

Posted: Mon Aug 28, 2006 12:28 pm

... and just a "final" two cents on my part, I don't see where the choice of a database affects this very much at all, nor does supporting multiple databases. In fact, the database abstraction part is one of the better parts of Nuke, but of course if you look at the credits he stole ... oops "took" that from PHPBB. The inputs should be filtered regardless of what database they are going into and we probably should not use database specific functions.

Posted: Mon Aug 28, 2006 2:35 pm

montego.... I thought RavenNuke would evolve in that direction. The one part I left out of "the mission" was merging SMF (Simple Machines Forums) into Nuke, or vice versa. This means removing nuke_authors and nuke_users and letting SMF do all that authentication (I use many of the SSI.php functions of SMF for that). With this approach I can delete Your_Account, News, Topics, Submit Topics, Surveys, phpBB, and Memberlist at least.

evaders... I tried to post that last function I cobbled together on that one thread on nukefixes.com, but I kept getting a 500 internal server error. Sad

Thanks for replying here!

Thanks for the heads up on function check_html. I see now that it does stripslashes, which can quickly end up in the "dead code pile".

OK.... Now to jump more into the discussion... Smile

From what I have read recently, magic_quotes_gpc is not going to be in PHP6. Sure it may be a while before it would become commonplace on servers worldwide, but in my own little world I would think making sure it is bypassed or dealt with like I put in that last function above, and then do the proper filtering/escaping would seem like maybe the best approach.

And along that line of thought, from security books and websites I have seen a strong focus on initializing variables to ensure data is clean before letting a function do its thing. I have not looked at all the modules, and right now I cannot remember, but maybe this is done on some modules already.

Nice posting evaders... thanks again!

Posted: Mon Aug 28, 2006 3:08 pm

I found another 2 cents. I had to do some googling to refresh up on my get_magic_quotes knowledge and came up with this short but informative post:

Only registered users can see links on this board! Get registered or login!

Sure seems like we should avoid it in future Nukes.

Just one other thing I'll mention: I believe that the wysiwyg editor that Kugske has so capably integrated into the latest Ravennuke versions takes a whole 'nother approach to filtering with the allowablehtml array and other features that I don't know a lot about in detail. Problem is that if you are entering a news article versus a Forum posting you are dealing with two entirely different sets of rules. It sure would be nice to be working towards an eventual reconciliation.

Posted: Tue Aug 29, 2006 12:48 am

Oh, God, my head hurts... Bang Head

too much to read and no time... sorry, will have to catch the conversation after 2.10.00 is out...

Posted: Tue Aug 29, 2006 1:02 pm

Hey montego, everyone

I do want your views on this. It will apply to the patches and RavenNuke too.
I have split it off into a seperate topic to get some kind of consensus and discussion building.

http://www.ravenphpscripts.com/postt11062.html
http://www.ravenphpscripts.com/postt11063.html