0

Super late edit (IMPORTANT): Please NEVER use this question as a tutorial of any sort. The examples shown here are an absolute nightmare open to any attack. I am unable to delete the question but do yourself a favor and close this tab. If you put some parts of the PHP code provided here your website is open to the simplest SQL injection -- if you manage to make it run because the methods shown here were deprecated even at the time of this question being asked. The accepted answer has all of the possible dangers for your website if you put this code in any page. Thanks.

Everything I want is a input with value of a MySQL row:

 <?php
  ob_start();
  session_start();
  require_once 'dbconnect.php';
  $results = mysqli_query($con,"Select * FROM users WHERE userName                             ='$username'");
 while ($name = mysqli_fetch_array($results))
 {
 extract($row);
 }

  // if session is not set this will redirect to login page
  if( !isset($_SESSION['user']) ) {
   header("Location: index.php");
   exit;
  }
  // select loggedin users detail
  $res=mysql_query("SELECT * FROM users WHERE userId=".$_SESSION['user']);
  $userRow=mysql_fetch_array($res);
 ?>
 <!DOCTYPE html>
 <html>
 <head>
 <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
 You just have logged in, so now you can go to your personal folder:<br>
 <?php
 if (isset($_POST['bt']))
 {
header("Location: http://yougle.rf.gd/" . $_POST['folder']);
 }
 ?>
 <html>
 <form id="form1" name="form1" method="post" action="<?php echo                $_SERVER['PHP_SELF']; ?>">
     <input type="text" name="folder" id="folder" value=<?php echo      $name[userName] ?>>
<input type="submit" name="bt" id="bt" value="Go To" />
</form>
</body>
</html>
<?php ob_end_flush(); ?>

And as you can see i have the form... But the field is empty... I've checked all the varibles (is it the same in the DB) and everything seems okay.

  • I really hope this code is nowhere near the public internet because you're exposing yourself to some extremely serious risks. – tadman Jan 04 '17 at 02:33
  • @tadman It's modified code of Coding Cage **by me** and some W3Schools code. Also get some parts from *here*. That's all. –  Jan 05 '17 at 00:26
  • W3Schools is full of really, really bad code and I'd strongly encourage you to find other sources. The road you're going down here only ends in disaster. – tadman Jan 05 '17 at 01:10
  • 1
    **WARNING**: Writing your own access control layer is not easy and there are many opportunities to get it severely wrong. Please, do not write your own authentication system when any modern [development framework](http://codegeekz.com/best-php-frameworks-for-developers/) like [Laravel](http://laravel.com/) comes with a robust [authentication system](https://laravel.com/docs/5.3/authentication) built-in. At the absolute least follow [recommended security best practices](http://www.phptherightway.com/#security) and **never store passwords as plain-text**. – tadman Jan 05 '17 at 01:10
  • **WARNING**: When using `mysqli` you should be using [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [`bind_param`](http://php.net/manual/en/mysqli-stmt.bind-param.php) to add user data to your query. **DO NOT** use string interpolation or concatenation to accomplish this because you have created a severe [SQL injection bug](http://bobby-tables.com/). **NEVER** put any form of user-supplied data directly into a query, it can be very harmful if someone seeks to exploit your mistake. – tadman Jan 05 '17 at 01:11

1 Answers1

2

I hardly know where to start with this one.

  1. You're open to SQL injection. You should not be using mysql_* functions. They are deprecated and removed from current PHP versions. Instead use PDO or mysqli with prepared statements and bound parameters.
  2. You are (I'm assuming) unnecessarily buffering the output as a way to get around the fact that you are unnecessarily sending a header() after content has been echoed.
  3. Your HTML is atrocious. Two <html> tags, no <body> tag, etc.
  4. You have a syntax error here: echo $name[userName] because userName is not a defined constant. Try enabling error reporting and you should see this.
  5. And finally for the icing on the cake you have an XSS vulnerability by echoing $_SERVER['PHP_SELF'] straight to the browser. http://www.html-form-guide.com/php-form/php-form-action-self.html explains how using htmlentities to escape this data can mitigate the issue.

Personally I prefer to err on the side of caution and bind all variables that aren't hard-coded into the script to the queries and properly escape everything before echoing it to the browser so that I never have to think about what else might be manipulating the values along the way.

Community
  • 1
  • 1
Mike
  • 23,542
  • 14
  • 76
  • 87
  • Does using unescaped $_SESSION values in SQL really constitute an sql injection vulnerability? Not that I disagree about moving away from deprecated functions. Or for that matter erring on the side of safety ( and syntax validity ) and _always_ using prepared statements for sql. But $_SESSION is in the control of the programmer, so if it's escaped going in, it seems like it would be safe. – erik258 Jan 04 '17 at 00:04
  • @DanFarrell I'm assuming `$_SESSION['user']` is the username, which is under the control of the user. – Mike Jan 04 '17 at 00:05
  • Hey, can you take a look at the whole website with the special test acc? –  Jan 04 '17 at 00:09
  • @mike You can't assume anything when you put data in a query. Just because it's coming from $_SESSION doesn't give it magical properties. You could have a bug that allows people to jam arbitrary things in there. Escape everything. No exceptions. – tadman Jan 04 '17 at 02:41
  • @tadman I realize that. I apologize if my comment above made it sound ambiguous. I've updated the answer again. Feel free to add more if you wish. – Mike Jan 04 '17 at 02:57
  • 1
    A year later, I am accepting this answer to match the header in the question. I am unable to delete the question so adding a safety header was the best option @tadman –  Jul 11 '18 at 03:05