INSERT INTO SQL result in empty elements being inserted

Question

Here is my code:

function register($user, $pass) {
    //check if username exists
    $login = query("SELECT username FROM login WHERE username='%s' limit 1", $user);
    if (count($login['result'])>0) {
        errorJson('Username already exists');
    }
       //try to register the user
       $result = query("INSERT INTO login (username, pass) VALUES('%s','%s')", $user, $pass); 
        if (! $result['error']) {
        //success
        login($user, $pass);
        } else {
    //error
         //errorJson('Registration failed');
              errorJson($result['error']);
        }
}



function login($user, $pass) {
    $result = query("SELECT IdUser, username FROM login WHERE username='%s' AND pass='%s' limit 1", $user, $pass);

    if (count($result['result'])>0) {
        //authorized
        $_SESSION['IdUser'] = $result['result'][0]['IdUser'];
        print json_encode($result);
    } else {
        //not authorized
        errorJson('Authorization failed');
    }
}

I call register with username and password and both fields in the DB are empty. The insertion takes place normally because the primary key which is auto incremented is being increased- just the fields are empty.

This is how I created my username.

CREATE TABLE `login` (
  `IdUser` int(11) NOT NULL AUTO_INCREMENT,
  `username` varchar(45) NOT NULL,
  `pass` varchar(45) NOT NULL,
  PRIMARY KEY (`IdUser`)
) ENGINE=MyISAM DEFAULT CHARSET=utf8;

and this is my query function:

//executes a given sql query with the params and returns an array as result
function query() {
    global $link;
    $debug = false;

    //get the sql query
    $args = func_get_args();
    $sql = array_shift($args);

    //secure the input
    for ($i=0;$i<count($args);$i++) {
        $args[$i] = urldecode($args[$i]);
        $args[$i] = mysql_real_escape_string($link, $args[$i]);
    }

    //build the final query
    $sql = vsprintf($sql, $args);

    if ($debug) print $sql;

    //execute and fetch the results
        $result = mysql_query($sql, $link);
    if (mysql_errno($link)==0 && $result) {

        $rows = array();

        if ($result!==true)
        while ($d = mysql_fetch_assoc($result)) {
            array_push($rows,$d);
        }

        //return json
        return array('result'=>$rows);

    } else {

        //error
        return array('error'=>'Database error');
    }
}

I have cheked that the $user, and $pass contain the correct value before executing the INSERT INTO.

Answer 1

You're not getting your results in the database because you're using sprintf syntax without calling sprintf. Try:

$sql=sprintf("INSERT INTO login (username, pass) VALUES('%s','%s')", $user, $pass);

$result = query($sql);

Edit: on further inspection, it looks like you're doing something unusual with your query function. I would guess your $args array doesn't have the values you expect it does. Do a print_r on $args and see if it's not the same as you expect.

Answer 2

You should use some kind of escape on your vars before using them in a query to avoid injection attacks. Also, you really shouldn't be using plain text passwords, it really is very bad security approach, even using md5 or sha, neither are considered secure anymore (although better than plain text, but only with very strong passwords due to the many dictionaries).

  `pass` varchar(45) NOT NULL,

The extensions $query, $mysql_real_escape_string (etc) are deprecated as of PHP 5.5.0, and you should use mysqli or PDO. Using mysqli (or PDO) will also help resolve your issues you have in your functions, as they provide the means already in the PHP libraries to encapsulate your queries in classes. Also, accompanied with you using prepared statements you'll resolve your security issues.

I know this doesn't directly answer your question (and I'll likely get downvoted as a result) I feel swapping to mysqli or PDO will resolve your issues as it's a much better approach, it's much more secure, once you know it it's just as easy to use plus they're built in libraries and so security updates and new features etc will be included in future PHP updates.

PDO vs mysqli are choice based on requirements (etc), but I use mysqli and while a learning curve, found it pretty easy in the end. I use it with crypt/blowfish and save passwords in the database with salts. Each password should have it's own unique random salt and saved in the DB with the encrypted password (no need to hide the salts).

It's used by Wordpress, Drupal and most other professional software and web companies as it's the strongest and safest password managing system.

Blowfish takes the pain out of the need of making salts and hashing, as all this is created in returned password all in one go. It creates the encrypted password, salt and returns the entire string that you just store in the DB. Then using the functions already provided you can check the passwords for logins etc.

Info about why to make the change from MD5 etc: http://php.net/manual/en/faq.passwords.php

It's pretty simple, example:

require('PasswordHash.php');//you download this file, and just include it
//it contains all the hashing engine etc

// $PostPassword is the one they entered in a form (etc)
$CreateHash = new PasswordHash(8, FALSE);
$HashPassword = $CreateHash->HashPassword($PostPassword);

// $HashPassword is the hashed and salted password you store in the db
// (should always be 60 chars, check with strlen)
// Don't use your own salt, it's not worth it 
// and you end up having to use it/store it/remember it
// when checking their password for login etc.
// just use the built in blowfish random salting algos


// Then to check their pass (ie login)
// Query and select their password from DB ($DbPass)
// with their username entered in the login form
// Check it against the password they entered in form
// (once hashed again of course to match the salted/hashed DB one)

$CreateHash = new PasswordHash(8, FALSE);

$CheckPass = $CreateHash->CheckPassword($PostPassword, $DbPass);
if ($CheckPass)
  {
    // password matches
  }
else
  {
    //not match, tell them to try again etc
  }

// you can use various checks on this, mainly check if the 
// library exists (to avoid php errors etc)
if(defined("CRYPT_BLOWFISH") && CRYPT_BLOWFISH) 
  {
    // do all your password stuff
  }

Pretty simple!