1

I am trying to create a signup form that allows potential users to input their first and last name, a username and a password. With this password, I want to use PHP's hash function and some "salts" to make the password secure. To do so, I created a function inside my PHP script to save on redundancy. To accomplish this, I changed variables $connection, $iniSalt, and $endSalt to global variables - thinking this would allow me to use them inside the function addUser(). Can someone tell me what I am doing wrong here? Any advice on how to go about this would be greatly appreciated. Here is my code:

global $connection;
$connection = new mysqli($db_host, $user, $pass, $db);

if ($connection->connect_error) {
    die($connection->connect_error);
}

$query = "CREATE TABLE users (
firstname VARCHAR(32) NOT NULL,
lastname VARCHAR(32) NOT NULL,
username VARCHAR(32) NOT NULL UNIQUE,
password VARCHAR(32) NOT NULL
)";

$result = $connection->query($query);
if (!$result) {
    die($connection->error);
}

global $iniSalt, $endSalt;
$iniSalt = "xb&z*";
$endSalt = "nb!@";

function addUser($conn, $firstname, $lastname, $username, $password) {
    $token = hash('ripemd128', "$iniSalt$password$endSalt");
    $query = "INSERT INTO users VALUES('$firstname', '$lastname', '$username', $token)";
    $result = $connection->query($query);
    if (!$result) {
        die($connection->error);
    }
}

addUser($connection, 'Bill', 'Murray', 'bmurray', 'mysecret');
addUser($connection, 'Jacki', 'Hughes', 'jhughes', 'somepw');
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
zhughes3
  • 467
  • 10
  • 22
  • global belongs inside the function. – Funk Forty Niner Mar 16 '16 at 15:39
  • 1
    **Red Flag** => `password VARCHAR(32)` that tells us you're not using a safe hashing function, or you've set it too low. and if `$token` is a string, you need to quote that too, and seems to be a string. That would have thrown you a syntax error right there, or will "once" that query fires up. – Funk Forty Niner Mar 16 '16 at 15:40
  • @Fred-ii- with your Red Flag statement...are you implying that I need to make the number of VARCHAR's much greater than 32 because of the hash is will return? – zhughes3 Mar 16 '16 at 15:43
  • The length for `ripemd128` seems fine, but if you're planning on using your present password system, I wouldn't use that, but `password_hash()` if your system's 5.5 or the compatibility pack if less than 5.5 and planning on going live with this. I made an edit to my comment up there too. – Funk Forty Niner Mar 16 '16 at 15:44
  • this question has been tagged with sql-injection, but this tag is irrelevant to the problem. Howver, if you want a protection, you have to use prepared statement for your queries. – Your Common Sense Mar 16 '16 at 16:06
  • @YourCommonSense Why did you just downvote my answer now? yeah, I know it's you. I've been keeping my eye on you ever since you closed the question. Sneaking suspicion you would have. You went from 100,481 to 100,480 in a blink of an eye. I have a good mind to reopen the question. And you thought I wasn't looking, and doing in my back. *Rich* – Funk Forty Niner Mar 16 '16 at 16:47
  • Love it when he hides behind a veil of *whatever he's got in his brain* I wish won't be reproducing. – Funk Forty Niner Mar 16 '16 at 16:56
  • 1
    @Fred-ii- sorry about the downvote problems. You helped me tremendously. Thank you for taking the time to give me a detailed answer. Learning this web programming with a heavy Java background - so its really helpful to have a seasoned PHP programmer looking out for me. Best. – zhughes3 Mar 16 '16 at 16:59
  • 1
    @zhughes3 Thanks but you don't need to apologize for that guy. He's just obtuse and special in his own little way. Let him downvote, the community will see who's right and who's wrong. and he... well, he's just plain "out of line". We go back a long way *lol* and not in a good way neither. *pffft*. I was glad to have "genuinely" helped you. It's "honest" and it's "pure". Enjoy coding, *cheers* – Funk Forty Niner Mar 16 '16 at 17:02

3 Answers3

2

You need to declare global scope for those variables inside your function:

function addUser($conn, $firstname, $lastname, $username, $password) {
    global $connection, $iniSalt, $endSalt;
    // Then use those global variables
    ...
}

Document for global keyword here: http://php.net/manual/en/language.variables.scope.php

Tuan Duong
  • 515
  • 3
  • 7
2

That's not how globals work. Put global $connection; at the top of your addUser() function to use it in your function.

function addUser($conn, $firstname, $lastname, $username, $password) {
    global $connection;
    ...
}
Brian Ray
  • 1,482
  • 3
  • 20
  • 20
0

Besides the other answers given in regards to the placement of global, and that I also pointed out in comments about its placement:

Sidenote: Consult my footnotes.

The $token variable is a string and it also must be quoted.

$query = "INSERT INTO users VALUES('$firstname', '$lastname', '$username', $token)";

When your query fires up, you will get a syntax error for it.

So, quote the variable:

$query = "INSERT INTO users VALUES('$firstname', '$lastname', '$username', '$token')";

ripemd128 produces a string, not an integer

ripemd128 32 789d569f08ed7055e94b4289a4195012

Also, if you're planning on going live with this, you'd be better off using password_hash() or the compatibility pack for it.

It's much safer.

References:

If and when you do decide to use password_hash() or crypt, it is important to note that if your present password column's length is anything lower than 60, it will need to be changed to that (or higher). The manual suggests a length of 255.

You will need to ALTER your column's length and start over with a new hash in order for it to take effect. Otherwise, MySQL will fail silently.


Footnotes:

Read up on variable scope:

If you're looking to protect against SQL injection which is something worthwhile doing, consult the following:

Community
  • 1
  • 1
Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141