0

I've been looking around and can't find a place that is showing me an effective way to do this. Currently I have a query that runs when the user submits a form:

$query = "UPDATE user SET username='$_POST[username]',
nicename='$_POST[nicename]', 
email='$_POST[email]', 
password=(SHA1)'$_POST[password]', 
position='$_POST[position]', 
race='$_POST[race]', 
type='$_POST[type]' WHERE username=$_SESSION[admin_login]";

I'm not sure on how to get this to actually work correctly. Sorry if it's been asked before, but I can't find a good solution to this anywhere. Thanks in advance for any help.

Morgan Green
  • 980
  • 2
  • 8
  • 22
  • What are you trying to achieve? – janenz00 Oct 06 '12 at 07:52
  • The user has a form to fill out that has a spot for all of their information. After submission it is supposed to update all of their profile information. – Morgan Green Oct 06 '12 at 07:54
  • 1
    **Your code is vulnerable to SQL injection.** You *really* should be using [prepared statements](http://stackoverflow.com/a/60496/623041), into which you pass your variables as parameters that do not get evaluated for SQL. If you don't know what I'm talking about, or how to fix it, read the story of [Bobby Tables](http://stackoverflow.com/questions/332365/xkcd-sql-injection-please-explain). – eggyal Oct 06 '12 at 08:00
  • Suggest you read [The Definitive Guide To Forms based Website Authentication](http://stackoverflow.com/a/477578/623041). – eggyal Oct 06 '12 at 08:02

3 Answers3

2

First of all entire thing is wrong : Why?

Because first of all you need to sanitize the input, which you are not doing, atleast you should use mysqli_real_escape_string like this :

$nicename = mysqli_real_escape_string($connect, $_POST['nicename']);

Reference

Secondly you should encrypt the password before you use it in your query like assign your encrypted password to a variable and than use it in your query, like this :

$hashed_pass = sha1($_POST['password']);

//Query goes here

and last but not the least instead of using super global $_SESSION variable directly in your query, use concatenate it.. like this

WHERE username='".$_SESSION[admin_login]."'";
Mr. Alien
  • 140,764
  • 31
  • 277
  • 265
  • to clarify - you can use `WHERE username=$_SESSION[admin_login]` but it is not considered clean code to do so because you should quote your associative indexes $_SESSION['admin_login'] vs $_SESSION[admin_login]. Plus it leaves the query wide open to SQL injection attacks. – Geek Num 88 Oct 06 '12 at 08:01
  • One could also use MySQL's [`SHA1()`](http://dev.mysql.com/doc/en/encryption-functions.html#function_sha1) function, if so desired. – eggyal Oct 06 '12 at 08:01
  • @GeekNum88 and eggyal thanks for your info, I edited :) learnt something new, actually if I dont use concatenate for super globals php shows me errors.. – Mr. Alien Oct 06 '12 at 08:03
  • I'm going to try setting all the post functions into variables and then after that I will read up on mysqli escape strings. I've heard of it plenty, but yet to find out what it's really for essentially. (Don't you love us self taught?) Thanks for the info and hopefully I can get something working. – Morgan Green Oct 06 '12 at 08:05
  • @MorganGreen As a suggestion, start accepting correct answers – Mr. Alien Oct 06 '12 at 08:06
1

Firstly, always remember Little Bobby Tables. Inserting data like that can lead to SQL injection attacks just like in that cartoon. I'd highly suggest you use prepared statements, this is a feature in both PDO and MySQLi which are methods of reading and writing to a database using PHP, some info on: PDO and some info on: MySQLi.

Whichever you choose to go with doesn't really matter, it's more about personal preference. I like PDO, so here's an example of binding the data and then executing your query using PDO:

$dbh = new PDO("mysql:host=$host;dbname=$dbname", $user, $pass);  

$password = sha1($_POST[password]);

$stmt = $dbh->prepare("UPDATE user SET username = :username, nicename = :nicename, email = :email, password = :password, position = :position, race = :race, type = :type WHERE  = :username");
$stmt->bindParam(':username', $_POST['username']);
$stmt->bindParam(':nicename', $_POST['nicename']);
$stmt->bindParam(':email', $_POST['email']);
$stmt->bindParam(':password', $password);
$stmt->bindParam(':position', $_POST['position']);
$stmt->bindParam(':race', $_POST['race']);
$stmt->bindParam(':type', $_POST['type']);
$stmt->bindParam(':username', $_SESSION['admin_login']);

$stmt->execute();
Stu
  • 4,090
  • 20
  • 42
1

$_POST and $_GET arrays can contain dangerous data, so you need prepare data from these arrays before inserting them into DB.

First, you need typecast values to right data types. In PHP you can use followed constructions: (string) for string data, (int) and (float) for numeric data, (bool) for boolean data.

Field email necessary checked for valid email, use Regex for it.

Follow code is sample of checking data:

<?php
    $link     = mysqli_connect('localhost', 'my_user', 'my_password', 'my_db');

    $username = mysqli_real_escape_string($link, (string) $_POST['username']);
    $nicename = mysqli_real_escape_string($link, (string) $_POST['nicename']);
    $email    = mysqli_real_escape_string($link, (string) $_POST['email']);
    $email    = preg_replace( '/^[_a-zA-Z0-9-]+(\.[_a-zA-Z0-9-]+)*@[a-zA-Z0-9-]+(\.[a-zA-Z0-9-]+)*\.(([0-9]{1,3})|([a-zA-Z]{2,3})|(aero|coop|info|museum|name))$/', $email );
    $password = sha1((string) $_POST['password']);
    $position = mysqli_real_escape_string($link, (string) $_POST['position']);
    $race     = mysqli_real_escape_string($link, (string) $_POST['race']);
    $type     = mysqli_real_escape_string($link, (string) $_POST['type']);
    $admin    = $_SESSION['admin_login'];

    $query = "UPDATE `user` 
              SET `username`='$username',
                  `nicename`='$nicename', 
                  `email`='$email', 
                  `password`='$password', 
                  `position`='$position', 
                  `race`='$race', 
                  `type`='$type' 
              WHERE `username`='$admin'";

    mysqli_query($link, $query);
    mysqli_close($link);
doktorgradus
  • 637
  • 1
  • 5
  • 13
  • So going off the mysqli escape string it looks like it needs two paramters. One being the string and one the connection info. I used `code`$username = mysqli_real_escape_string((string, $con) $_POST['username']);`code` and it's a syntax error and won't run; any ideas by chance? :( – Morgan Green Oct 06 '12 at 08:46
  • Awesome. :) I kind of understand it, but it's going to take some getting used to. I appreciate the help. :) – Morgan Green Oct 06 '12 at 09:11