1
\$\begingroup\$

On most my $_post data inputted on my site I use the following php:

$example = $_POST['textfield'];
$example = strip_tags($example);
$example = mysql_real_escape_string($example);

And then I would interact with the MySQL database...

Is this 'secure' / 'safe'?

Any major exploits to the above code?

-How could I make it secure?

Thanks alot.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ I believe it's considered more secure to do prepared statements using PDO. \$\endgroup\$ – Jared Farrish Jul 10 '11 at 19:01
  • 2
    \$\begingroup\$ Wow @Jared Farrish, I use to do straight PHP->MySQL calls the 'old school' way, but now after you pointed out PDO and I looked into it a bit, it looks quite awesome. Next project I start, I'm using this! \$\endgroup\$ – CenterOrbit Jul 11 '11 at 0:10
  • 1
    \$\begingroup\$ PDO's a good suggestion, but not necessarily the only game in town. A key principle to extract from the suggestion: Security can't be something you have to remember to do EVERY TIME you write a db interaction (or filter user input for other uses, or whatever). Build your app in such a way that validation/cleansing is automatic and uses the same code every time. \$\endgroup\$ – grossvogel Jul 11 '11 at 0:22
1
\$\begingroup\$

I recommend getting yourself a simple, simple function that uses PDO and parameterizing of queries by default behind the scenes.

Here are sample use cases for query functions that use PDO in the background that I use all the time:

$iterable_resultset = query('select * from users where email = :email and username ~* :username or user_id = :user_id', array(':email'=>$email, ':username'=>$username, ':user_id'=>(int)$user_id));
// Results a resultset that is able to be foreached over.

$multidimensional_array = query_array('select * from users where email = :email and username ~* :username or user_id = :user_id', array(':email'=>$email, ':username'=>$username, ':user_id'=>(int)$user_id));
// Results in a multidimensional array with each subarray corresponding to a database row, or null.


$row_of_data = query_row('select * from users limit 1');
//Results in a single dimensional array or null.

$item = query_item("select max(score) from players where username ~* :username", array(':username'=>$username));
// Returns a single piece of information, or null.

There's a lot of advantage from a refactoring viewpoint to having simple function-based parameterizing of sql queries. PDO is often verbose, and sometimes when you're securing bad code you need simple, terse functions to fit in the place of old mysql_query() calls and the like.

In your specific case, I think that you are making the mistake of escaping before it's time, which can often introduce weird bugs.

$untrusted_input = @$_REQUEST['password'];

is all that you should do when getting the input. Don't strip tags, don't do anything until it comes time to -use- that untrusted input in sql or in html:

  • In the sql, parameterize it while using the sql, filter/modify the data based on the circumstances.
  • When outputting to html, escape it then, e.g. via .htmlentities($example). or using a templating engine.

Here's an example of your code turning a non-existent get/post variable (that would normally result in null) into an empty string, for just the first of many subtle things that happen when you escape too early: http://ideone.com/r7bSr

In general, when you're first getting used to the security issues that php can throw your way, I really recommend using a template engine (e.g. smarty) to help you keep your business logic and your display templates separate. A lot of people will argue about whether you need a templating engine with php because php is a templateable system already. Engine or not doesn't matter, but a template-based approach to separate your manipulation logic from your display logic is very necessary, which most programmers would probably agree on. And personally, I found it easier to learn how that separation benefits php when I was first starting out by just learning how a templating engine does things, before I could understand how to apply those principles to native php without a templating engine.

TL;DR; - Set up a templating engine, it'll get you using best practices faster.

\$\endgroup\$
0
\$\begingroup\$

It doesn't seem like there are any security issues with your code. But like grossvogel said, you shouldn't try to implement security by constantly writing that function every time you have a query, because maybe you'll forget. What I would recommend is that you write a helper function that automatically escapes the input for you. That or you could use prepared statements.

\$\endgroup\$
-1
\$\begingroup\$

Apart from what has already been mentioned, a crucial part you're missing is a magic_quotes check and taking according action, this means reversing all its effects. (unless you do that at the beginning after your script starts).

If enabled, magic_quotes will escape all quotes with a backslash, and each backslash with a backslash.

Read more about magic_quotes here.

\$\endgroup\$
5
  • \$\begingroup\$ This is a bad idea for a lot of reasons. The first one is stated directly in the link you provided: "Warning This feature has been DEPRECATED as of PHP 5.3.0. Relying on this feature is highly discouraged.". Other than that, magic_quotes isn't secure at all. \$\endgroup\$ – PiZzL3 Jul 11 '11 at 22:34
  • \$\begingroup\$ @PiZzL3: I didn't suggest the feature. I suggested being aware of it. That's why you need to reverse its effects! "taking according action" Maybe I should have made it more clear. \$\endgroup\$ – phant0m Jul 11 '11 at 22:43
  • \$\begingroup\$ He's not even using it in his code, so why would that need reversing? You should just say to never use magic_quotes and never use mysql_real_escape_string (both methods have publicly known flaws and can easily be exploited, even when used together(which can cause more problems and exploits to appear that were previously do-able by themselves)). He should use PDO or other methods of using prepared statements, along with very specific custom filtering depending on each data type he's working with (examples: numbers, letters a-z only, etc..). \$\endgroup\$ – PiZzL3 Jul 11 '11 at 23:39
  • \$\begingroup\$ "He's not even using it in his code" Apparently you don't know what it is. I have never seen anybody using magic_quotes in his code... You cannot always control whether magic_quotes is enabled or not. That's why your script has to adapt and reverse everything magic_quotes has done in case it is enabled. Using PDOs does not save you from enabled magic_quotes. \$\endgroup\$ – phant0m Jul 11 '11 at 23:43
  • \$\begingroup\$ @PiZzL3: See, someone in exactly this situation, only a few minutes ago. An ignorant already posted a very rude answer telling him to just disable magic_quotes and use PDO...: stackoverflow.com/questions/6661406/… \$\endgroup\$ – phant0m Jul 12 '11 at 8:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.