views:

90

answers:

6

Hi

I have been asked a question but I cannot even start to answer is so could some one give me an idea of were to start on how to possibly answer it ,

I am not looking for the answer just some teaching on how to answer it

here goes:

Assuming "regsister_globals" and "magic_quotes_gpc" are turned on, Whats wrong with this piece of code ? Document the possible holes , then fix them to produce a secure version ( There are 4 Errors)

$p = $_GET["p"];
if ($sp == "index.php") {

     if ($_get["id"] == 345)
          $filter - addslashes($_get["id"]);

     $sql = "SELECT * FROM users WHERE id = {$filter}";
     $row - mydql_fetch_assoc(mysql_query($sql));

     echo <<< HTML
     <html>
           ...... user details .....
     </html>
HTML. 
} else 
      include ($p);
A: 

The code is vulnerable to SQL injections because user data is not escaped. Use mysql_real_escape_string

middus
+1  A: 

Googling for "SQL-injection" and "Input validation" should get you started.

Given the environment,

"Assuming "regsister_globals" and "magic_quotes_gpc" are turned on"

I believe that this case is meant to teach you the risk of both these settings.

The code snipped actually has 4 error related to "Never trust ANY information originating from outside your script", in combination with the above mentioned php-directives.

(There is a lot more wrong than just the 4 errors; there are a couple of "-" that should be "=" and a lower-case "_get" that should be upper-case etc. but my guess is that these are just typos.)

Jacco
A: 

mydql_fetch_assoc should be mysql_fetch_assoc

Kennethvr
Lot of syntax problems in there, e.g. $filter - addslashes($_get["id"]);
middus
A: 

Try this:

<?php
$p = $_GET["p"];
if ($p == "index.php" && $_get["id"] == 345) {

 $filter = mysql_real_escape_string($_get["id"]);

 $sql = "SELECT * FROM users WHERE id = {$filter}";
 $row = mysql_fetch_assoc(mysql_query($sql));

?>
<html>
 ...... user details .....
</html>
<?php
}
else if (strpos($p, '../')===false && file_exists($p)) {
 include $p;
}

?>
Rowan
+1  A: 

This should get you started:

Hole 1: register_globals should be off - it's a security disaster.

$p = $_GET["p"];
// Where does $sp come from?
if ($sp == "index.php") {

     // What the hell? So much wrong with these two lines
     // 1. if id == 345 you don't need to addslashes
     // 2. "-" should be "="
     // 3. addslashes should be mysql_real_escape_string
     // 4. the if() should be removed so it runs every time
     if ($_get["id"] == 345)
          $filter - addslashes($_get["id"]);

     // SQL injection
     $sql = "SELECT * FROM users WHERE id = {$filter}";
     // Again with the "-" instead of "="
     // Typo in the function name
     // No error checking
     $row - mydql_fetch_assoc(mysql_query($sql));

     // No escaping of database input - vulnerable to XSS attacks
     echo <<< HTML
     <html>
           ...... user details .....
     </html>
HTML. // Should be ; not .
} else 
{
      // I can include /etc/passwd by manipulating the URL
      include ($p);
}
Greg
It is a homework question. so register_globals is "on" as part of the question. Also, I think that you might have answered just a bit to fast, because not all your comments are correct.
Jacco
A: 
<?php
    $allow_includes = array(
      'some1', 'some2'
    );
    $p = $_GET["p"];
    if ($p == "index.php") {
         if ($_GET["id"] == 345) {
              $filter = mysql_real_escape_string($_GET["id"]);
         }

         $sql = "SELECT * FROM users WHERE id = '{$filter}'";
         $row = mysql_fetch_assoc(mysql_query($sql));
 ?>
    HTML. 
 <?php
    }
    elseif ( in_array($p, $allow_includes) ) {
      include ($p);
    }
    else {
      echo "Error 404";
    }
 ?>
Berkut