PHP Login system - Best Practice

[Thor]

Hello, can someone please guide me in the right direction here.

I have a user ( hard coded in the DB which is the admin this admin user is admin becuase in my DB I have a column called isAdmin which is a Boolean and is set to 1 )

Now my question is, would the following be fine for normal user registration ie what are the security risks if any for handling normal registrations like this:

"INSERT INTO users(userName, userEmail, userPassword, isAdmin) VALUES('" . $name . "', '" . $email . "', '" . md5($password) . "', '0')")

Thus by default all users are set to 0, I went into the DB to make myself an Admin by changing 0 to 1

Is this safe and fine or should I get a whack and do it different?

EDIT:

See this post for clarification -> http://mybroadband.co.za/vb/showthr...t-Practice?p=18623245&viewfull=1#post18623245

 

[Necropolis]

Are you sanitising your inputs?

Otherwise that is open the a SQL Injection attack...

 

[Thor]

Are you sanitising your inputs?

Otherwise that is open the a SQL Injection attack...

I am yes sir.

 

[Necropolis]

I am yes sir.

Then it shouldn't be an issue.

Another option would to have a default value of 0 for that isAdmin column and exclude it from your insert statement altogether.

Also - why not use stored procs?

 

[DA-LION-619]

I like the ASP.NET(Identity) approach.

3 tables.
A Role table, which would contain your roles like Admin.
A User table, that would contain your users.
A UserRole table that would relate your user to their respective roles.

 

[Thor]

Then it shouldn't be an issue.

Another option would to have a default value of 0 for that isAdmin column and exclude it from your insert statement altogether.

Also - why not use stored procs?

Not sure, guess I haven't experimented with it enough. Should definitely look into it.

As for default to 0 yea good thinking that slipped my mind I will do that.

I like the ASP.NET(Identity) approach.

3 tables.
A Role table, which would contain your roles like Admin.
A User table, that would contain your users.
A UserRole table that would relate your user to their respective roles.

I like it, so much to learn with SQL and thinking in terms of Database stories if I can call it that.

 

[scudsucker]

Thor, your query is vulnerable to SQL injection no matter how carefully you've tried to sanitise the inputs.

I have no experience in MySql stored procedures (I assume you are using MySQL) but if you do not use Sprocs, please use parameterised queries/prepared statements:

http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

If MySQL stored procedures are anything like TSQL stored procedures you should look into them; while its unlikely you'll benefit from the higher speed of the query they do introduce another level if complexity for a would-be SQL injection attack

 

[Thor]

Thor, your query is vulnerable to SQL injection no matter how carefully you've tried to sanitise the inputs.

I have no experience in MySql stored procedures (I assume you are using MySQL) but if you do not use Sprocs, please use parameterised queries/prepared statements:

http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

If MySQL stored procedures are anything like TSQL stored procedures you should look into them; while its unlikely you'll benefit from the higher speed of the query they do introduce another level if complexity for a would-be SQL injection attack

Vulnerable how exactly? (Genuine question, not being an but hole)

Your not going to get through my mind fck of a regex mind field.

I'll put this query with my validation and sanitation on a demo site with something in the db and then see if anyone can hack it via SQL injection and retrieve what is in the DB

Thoughts?


Also yes I'm using Mysql and also have not tried stored procedures in it hence why I don't have any at the moment I still need to learn how on Mysql.

 

[Thor]

Thor, your query is vulnerable to SQL injection no matter how carefully you've tried to sanitise the inputs.

I have no experience in MySql stored procedures (I assume you are using MySQL) but if you do not use Sprocs, please use parameterised queries/prepared statements:

http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

If MySQL stored procedures are anything like TSQL stored procedures you should look into them; while its unlikely you'll benefit from the higher speed of the query they do introduce another level if complexity for a would-be SQL injection attack

Wait a minute. After reading that link, I then use prepared statements already it seems since I do that with PDO (not in this example, but production is PDO)

 

[Thor]

Thor, your query is vulnerable to SQL injection no matter how carefully you've tried to sanitise the inputs.

I have no experience in MySql stored procedures (I assume you are using MySQL) but if you do not use Sprocs, please use parameterised queries/prepared statements:

http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

If MySQL stored procedures are anything like TSQL stored procedures you should look into them; while its unlikely you'll benefit from the higher speed of the query they do introduce another level if complexity for a would-be SQL injection attack

Sorry for the confusion this is what it will look like in production:

$statement = $db->prepare("INSERT INTO users (`name`, `email`, `password`, `isAdmin`) VALUES (:name, :email, :password, :isAdmin)");
$statement->execute(array(':name' => $name, ':email' => $email, ':password' => $password, ':isAdmin' => $default_zero));

 

[Thor]

Can some of you run this script?

I get between 10 and 11 on my various servers

<?php
/**
 * This code will benchmark your server to determine how high of a cost you can
 * afford.
 */
$timeTarget = 0.05; // 50 milliseconds 

$cost = 8;
do {
    $cost++;
    $start = microtime(true);
    password_hash("test", PASSWORD_BCRYPT, ["cost" => $cost]);
    $end = microtime(true);
} while (($end - $start) < $timeTarget);

echo "Appropriate Cost Found: " . $cost . "\n";
?>

I assume 10 would be good enough or should I risk it and push it to 12.

Reason I am asking - I use md5 for local development out of habit but will be using bcrypt on the production app ( I have not used bcrypt yet so need to go read about it.)

Hence the above code since I figured this should be sufficient to at least replace my md5 way: (See below as a temporary measure using bcrypt)

// Creating a hash
$hash = password_hash($password, PASSWORD_DEFAULT, ['cost' => 12]);
// If I omit the ['cost' => 12] part, it will default to 10

// Verifying the password against the stored hash  
if (password_verify($password, $hash)) {
    // Success! Log the user in here.
}

 

[Thor]

A+ I like bcrypt!

I will make this PDO before I use it.

 

[Mr Scratch]

 

[Necropolis]

Thor, your query is vulnerable to SQL injection no matter how carefully you've tried to sanitise the inputs.

How do you figure that?

Assuming what Thor is using to sanitise is solid then there is 0 change of SQL injection attack...

 

[Thor]

How do you figure that?

Assuming what Thor is using to sanitize is solid then there is 0 change of SQL injection attack...

This is what I have thus far, trying not to complicate it unnecessary but I want to ad more stuff as I learn. Also, all of it will be PDO.


On the form I also do:

<form method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

 

[IndigoIdentity]

You might not like this input very much but my 2c on best practice would be that you should not be rolling your own, most especially if you do not fully understand all of the intricacies that are involved.

In my opinion, you'd be far better off in the long run digging into terms like Token based Authentication and OAuth than spending this time trying to roll something new.

With that said though, seems like you're making progress at learning the fundamentals so don't let me stop you from doing that.

(sidenote) To avoid further speculation, it would be useful to provide us with the code that you are using to sanitize those values in the SQL query before you made use of the prepared statement. There's a good answer on SO which would be relative to this.

 

[Thor]

You might not like this input very much but my 2c on best practice would be that you should not be rolling your own, most especially if you do not fully understand all of the intricacies that are involved.

In my opinion, you'd be far better off in the long run digging into terms like Token based Authentication and OAuth than spending this time trying to roll something new.

With that said though, seems like you're making progress at learning the fundamentals so don't let me stop you from doing that.

(sidenote) To avoid further speculation, it would be useful to provide us with the code that you are using to sanitize those values in the SQL query before you made use of the prepared statement. There's a good answer on SO which would be relative to this.

I have added the code just before you posted. Click here for the code

As for the rest of your post 100% agreed I use Laravel for everything, but that is monkey see monkey do so I do little pet projects like this to just understand what is involved etc.

 

[Necropolis]

This is what I have thus far, trying not to complicate it unnecessary but I want to ad more stuff as I learn. Also, all of it will be PDO.
View attachment 399907

On the form I also do:

<form method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

Surely there are pre-built PHP input sanitation functions that would do the job for you?

Oh and the English in your string is atrocious.

 

[_kabal_]

simple form based authentication is fine. no need to overkill with token based, or oauth.

those can be added as an additional layer anytime anyway.


please stop abbreviating things. what is the point of omitting the `e` in `usr_id` and `usr_name`?


also, pick a style for variable names, and stick to it. `$succcessmsg` vs `$password_error`. that is not consistent.

http://www.php-fig.org/psr/psr-1/
http://www.php-fig.org/psr/psr-2/

 

[Thor]

Surely there are pre-built PHP input sanitation functions that would do the job for you?

Oh and the English in your string is atrocious.

Not sure what you are talking about? I am using pre-built php input sanitation

and what English in what string?