0

I have a old system without any check for sql injection and I want to add mysqli_real_escape_string() every time user intract with the DB.

All of the system is built under index.php. the page look like that:

if (!isset($_GET['p'])) {
    $_GET['p'] = 'main';
}

if (!file_exists($_GET['p'].".php"))    {
    echo "The page you are looking for isn't exist.";
}   else    {   
    if (logs()) {
        include($_GET['p'].".php");
    }
    else    {
        include('not_register.php');
    }
}

I thought of just adding this code in the top oh index.php and I wanted to be sure I'm not messing up with anything so i'm asking here.

foreach ($_POST as $name => $val)   {
    $_POST[$name] = mysqli_real_escape_string($db, $val);
}

This code running every reload of page will have any negative influence?

thx.

OfirH
  • 651
  • 1
  • 8
  • 19
  • 2
    Your code is vulnerable to code injection read here http://www.ultsec.com/php-code-injection-attack.php – Fabio Mar 22 '14 at 10:30
  • 1
    Your code may be vulnerable to [remote file inclusion](https://cwe.mitre.org/data/definitions/98.html). – Gumbo Mar 22 '14 at 10:30
  • As far as I know `file_exists()` works only for files on the same server am I wrong? And how is it vulnerable to code injection? didn't I fix it with the `foreach` loop? – OfirH Mar 22 '14 at 10:32
  • @OfirH There are [wrappers](http://php.net/wrappers) that also support `stat` functionality, like `ftp` or `ssh`. – Gumbo Mar 22 '14 at 10:33
  • Can I fix it and save the current include in `index.php`? or I will have to stop using `index.php` as main page? – OfirH Mar 22 '14 at 10:36
  • @OfirH Just check the value of `$_GET['p']` whether it holds an expected value. – Gumbo Mar 22 '14 at 10:41
  • I have about 25 files on the system. I searched google for function that checks if the file is on the server but I couldn't find one. so the best way will write a function which does it right? – OfirH Mar 22 '14 at 10:44
  • @OfirH You may want to have a look at [Preventing Directory Traversal in PHP but allowing paths](http://stackoverflow.com/q/4205141/53114). – Gumbo Mar 22 '14 at 10:59
  • > Try this answer... http://stackoverflow.com/a/33477677/3944217 May help you – Edwin Thomas Nov 02 '15 at 12:35

2 Answers2

2

This:

I want to add mysql_real_escape_string() every time user intract with the DB.

Is a good goal for legacy mysql code that is using mysql_* functions. However, this:

foreach ($_POST as $name => $val) { $_POST[$name] = mysqli_real_escape_string($db, $val); }

is a different thing. You're not adding it to every time user interacts with DB, you're adding it done every time, before anything else has been done. The function needs a connection to the database, so if you don't have such a connection in a page that uses these variables, you immediately hit into issues. Furthermore, you can break any handling of those variables that might not expect the values to be escaped at this point - they should be escaped for DB usage, so immediately before using them with the database, not before that.

Also, as others have noted, your code is vulnerable to injections with your include pattern. file_exists can be used with network shares, file paths as well as some url wrappers. To quote file_exists manual entry:

As of PHP 5.0.0, this function can also be used with some URL wrappers. Refer to Supported Protocols and Wrappers to determine which wrappers support stat() family of functionality.

Even without url wrappers, a malicious user can use your include to directly include some server configs and other files you don't want to be included.

eis
  • 51,991
  • 13
  • 150
  • 199
  • So if I'll will make this loop before any user DB interat instead of insert it into variables It should be fine right? – OfirH Mar 22 '14 at 10:38
  • @OfirH didn't understand that. You shouldn't be looping POST, you should only escape the variables in the context of your DB queries. – eis Mar 22 '14 at 10:45
  • I know. But It's a browser game. 95% if not all of the `$_POST` variables is with context with the DB. – OfirH Mar 22 '14 at 10:47
  • Well, you don't want to be breaking stuff for the other 5% either. Use a database abstraction layer or bound parameters for this. That's the correct way which won't have the side effects mentioned. – eis Mar 22 '14 at 10:48
0

First.

Running this question an a loop will help noone. In fact, you just reinvented a notorious magic quotes feature, that is already removed from the language. For a reason.

Second.

It is NOT good goal for legacy mysql code that is using mysql_* functions. Just because this function has nothing to do with injections at all.

If you have only 25 pages - just review them all and rewrite SQL handling code properly

Community
  • 1
  • 1
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • it's a bit better than magic quotes, since mysql_real_escape_string actually uses database for the escaping, instead of hardcoded escapes that magic quotes did. Still, it's not a good thing to do. – eis Mar 22 '14 at 10:52
  • Also, if you claim that it is not a good goal even for legacy code to use mysql_real_escape_string/mysqli_real_escape_string whenever interacting with the database, you might want to add some rationale why it would not be so. – eis Mar 22 '14 at 10:53