possible error

[i3130002]

Hi At first let me say thanks for your great codes. I was looking around and saw this

 $sql = "SELECT * FROM users WHERE user_name = '" . $user_name . "' OR user_email = '" . $user_email . "';";
$query_check_user_name = $this->db_connection->query($sql);

                if ($query_check_user_name->num_rows == 1) {
                    $this->errors[] = "Sorry, that username / email address is already taken.";

line (91 to 95) of classes\Registration.php

What if the user and also the email where exist ? i suggest to use if ($query_check_user_name->num_rows > 0) {

Let me know if its a good idea

[panique]

Hey there, this should work fine as username and email are unique columns, so it's never possible to have more than one result... but correct me if i'm wrong here!

[i3130002]

What if some one uses bob username and alex email ? it will result of two rows

[panique]

ah okay now i got it :) do you want to send a pull request to master branch (so you are listened as constributor to the project), if not i can do that too

[i3130002]

wow. thanks man it will be my first contribution .

[i3130002]

if i am correct then we may have a problem with the rest of the code . we can't do something like $result_row = $result_of_login_check->fetch_object(); and then $result_row->user_name; cause it maybe two records

but as you written ==1 it is correct but it may give a wrong error

[bobrocha]

Why is there no use of prepared statements? This is completely open to SQL injection.

[i3130002]

There is one at line 66 of classes\login.php $user_name = $this->db_connection->real_escape_string($_POST['user_name']); And i guess as the password will be hashed we will have no issue with login or register. am i wrong ?

[panique]

@bobrocha At the time of writing (around 2011) it was intentionally written in pure mysqli statements + FILTERING as PDO was not suitable for beginners to use in that era, filterering happens like @i3130002 says. This filtering is exactly like the most upvoted and commonly accepted answer on Stackoverflow regarding preventing SQL injection says! From today's perspective for sure not the way to go, but as this script is only the most simple implementation of something that's not even necessary anymore since PHP 5.5 has been released, it's not a big problem I guess.

If you this should be rewritten please gimme a short comment or a pull-request!

[bobrocha]

@panique mysqli has been available since 2004 or so, and I can only assume that prepared statements were also available at that time, especially by 2011 (7 years later). PDO was available 1 year after mysqli was released if my memory is correct. The most up-voted answer on this matter on stackoverflow emphasizes using prepared statements: http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-ph

The way to go is to use prepared statements and sanitize/escape the output.

I am not sure how much of the current code depends on statements that would require prepared statements. It could be that the whole thing needs to be re-written from scratch taking into consideration the use of prepared statements and sanitizing the output.

The use of this project otherwise, leaves one at risk.

[i3130002]

@bobrocha may you help me to see where we have a security issue ? (if you can address file and line it will be great ) thanks

[panique]

@bobrocha there was a lot of discussion between lots of people and lots of work that ended up in the lines you see, so simpyl saying "its wrong" is not correct here! the project has implemented EXACTLY what the most upvoted answer said at that time, and that was to use "mysql_real_escape_string()" (it's now the second most upvoted answer, and it has been edited and improved too). I know what you want to say, but your current perspective is something totally different that it was 5 years ago. Also, PDO didn't work out of the box in most shared hosting environments at that time, that's why basically every tutorial from this era still uses mysql/mysqli. The very first version of this project actually HAD prepared statements :) Also let me say that the current code clearly sanitizes SQL inputs!

Anyway, IT security is a hole without a bottom, so even when using PDO stuff like second-level-SQL-attacks are still possible under certain conditions, and even when using db-related stuff in the best way possible according to CURRENT best practices, then somebody comes and say "hey when are the cookies not encrypted, why are you still using Apache, PHP sucks in general when it comes to security, and your forms are open for this exotic JS overflow exploit"...

So, what would you recommend ? Rewriting parts of the project or hiding the repo ? I really dont have time to work on this right now

[bobrocha]

@panique What stackoverlfow question are you talking about? I gave a link to mine and the answer clearly states the use of prepared statements: http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php

This question and answer is from 2008. last I checked this project started in 2013 and even in 2011 prepared statements were still available.

You don't have to use PDO, mysqli offers prepared statements too, if your database is in MySql. http://php.net/manual/en/mysqli.prepare.php

Even OWASP puts prepared statements at the top: https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet

It clearly states: "The use of prepared statements with variable binding (aka parameterized queries) is how all developers should first be taught how to write database queries."

My recommendation is wherever a variable is used in the query/sql, that code needs to be updated with prepared statements.

Example with variable: SELECT username FROM users WHERE email = $email

Example with prepared statements in MYSQLi SELECT username FROM users WHERE email = ?

or using PDO: SELECT username FROM user WHERE email = :email

[panique]

@bobrocha In 2010/11 this stackoverflow-page looked different and the now #2 answer was #1 then, answers's content and ranking change over time...

[bobrocha]

@panique I still don't see the stackoverflow question you are talking about. Your links don't work.

[i3130002]

Sorry for interopt as i read it is better to use to mysqli parameters ir etc but special characters are safe two, although it won't suggest as you may miss one input. So can someone tell me is there a security issue in this code?

On Mon, Aug 22, 2016, 2:43 AM Robert Rocha notifications@github.com wrote:

I still don't see the stackoverflow question you are talking about. Your links don't work.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/panique/php-login-minimal/issues/29#issuecomment-241285622, or mute the thread https://github.com/notifications/unsubscribe-auth/ALEJyAFLM2oohfsEMduAj-Fh_9keNqnWks5qiM2dgaJpZM4JnbVn .

Raf@jah

[faiqadi]

hew, why i got trouble when i want to hosting this code?

[DevFelixDorn]

Details? And you may have to open a new issue.

[mateuslacorte]

You must provide enough information for us to help, and yeah, open a new issue...

[panique]

@faiqadi hey, what's the exact error ? please note that this project is really old now, it might be easier to work with Laravel or Symfony today :)

[faiqadi]

ah,sorry it was my error, i'm forget to change databases :v, this code is awesome