PHP database fetch issue

#1

One of my hosting servers deprecated the earlier versions of PHP and left me with one website that is not working. I successfully migrated all of my other sites over but this one is giving me trouble. It's a simple connect script that checks users against a database and if they are valid users, it will send them to one page, and if they are not, it will send them to a failed login page. The present script is this: (which has been working fine fine for 10+ years!)

session_start();
$host="xxx"; // Host name
$username="xxx"; // Mysql username
$password="xxx"; // Mysql password
$db_name="xxx"; // Database name
$tbl_name="xxx"; // Table name

$link= mysql_connect("$host", "$username", "$password",)or die("cannot connect");
mysql_select_db($link, "$db_name")or die("cannot select DB");

$myusername=$_POST['username'];
$mypassword=$_POST['password'];

$query="SELECT * FROM users WHERE username='$myusername' and password='$mypassword'";
$result=mysql_query($query);
$rowAccount=mysql_fetch_array($result);

if($rowAccount){
$_SESSION['id'] = $rowAccount['level'];
$_SESSION['username'] = $rowAccount['username'];

header("location:xxx.php");
exit;
}
else {
header("location:xxx.php");
}

I changed it to this and the connection script works with the new mysqli but it seemes the fetch array is not working. No matter what I put into the username/password, it redirects me to the failed login page. 

session_start();
$con = mysqli_connect('xxx', 'xxx', 'xxx', 'xxx');

$myusername=$_POST['username'];
$mypassword=$_POST['password'];

$result=mysqli_query($con,"SELECT*FROM users WHERE username='$myusername' AND password='$mypassword'");
$rowAccount=mysqli_fetch_array($result);

if($rowAccount){
$_SESSION['id'] = $rowAccount['level'];
$_SESSION['username'] = $rowAccount['username'];

header("location:home.php");
exit;
}

else {
header("location:error.php");
}

And yes I know, some people prefer to use Object Orientated or PDO includes, but this is just such a simple script that i want to get working. I'm sure it's something obvious!

Thanks!

#2

That code is wide open to potential SQL Injection attacks, also passwords should never be stored in plain text form, they should always be hashed

#3

The obvious reason is that the if($rowAccount) is not testing as truthy. So somewhere upstream from that something is failing.

Yes, you should at least take advantage of bound parameters while you're updating the code.

#4

SELECT*FROM should be SELECT * FROM

#5

Or even:-

SELECT level, username FROM...

I think PDO is actually simpler to use than mysqli, give it a go, you may be pleasantly surprised.

And while you are working on it, definitely up your security, prepared statements, hashed passwords are the very least you must do.

#6

I find it interesting that you mentioned a deprecated/obsolete version of php yet no one has asked what version of php the prod environment has. I especially find that interesting because ScallioXTX typically is right on top of these things. No php version no nothing. So I think we should probably start with the version of php that was on your old server config and the one that is on your new server config?

#7

Well, if the mysql extension is no longer there it means they migrated from 5.x to 7.x, since the mysql extension was axed in 7.0.
And the difference between 7.0, 7.1 and 7.2 isn't really big and don't include any changes this rather simple script would use. So I don't have to ask, because the unavailability of the mysql extension already gives me all the information I need :slight_smile:

#8

Thanks for the replies everyone. I do have security stripslahes etc. in place, I just didnt copy them to this code on here.

So, I have tired
$query="SELECT * FROM users WHERE username='$myusername' and password='$mypassword'";
$result=mysqli_query($query);
$rowAccount=mysqli_fetch_array($result);

and

$result=mysqli_query($con, "SELECT * FROM users WHERE username='$myusername' and password='$mypassword");
$rowAccount=mysqli_fetch_array($result);

and

$result=mysqli_query($con, "SELECT level, username FROM users WHERE username=? and password=?");
$rowAccount=mysqli_fetch_array($result);

and same result. Doesnt work. No matter what I put into the UN/PW screen on the webpage, it will bring me to the login failed page. Could it be something in the fetch array statement?

#9

Slow down..... Putting an i at the end of mysql will not work. This is not a profession where you can simply slap things on whatever you think is right and expect it to be good quality or at the very least, do what you expect it to do.

You have to actually learn and understand what each part of the code does. Since you are attempting to use prepared statements in mysqli_*, I suggest doing it the proper way. Stop using procedural and start using the OO version. What typically happens in 99.99% of cases is that people who use procedural will never use it correctly. They will always slap the variable directly into the query and this is where SQL Injections happen. PHP has nothing to do with SQL Injections. The person who wrote the garbage looking code is at fault. SQL Injections doesn't come from PHP, it comes from the person writing the PHP code.

You cannot use placeholders for regular queries. They require prepared statements.

This is not part of stmt so you will indeed get an error if you happen to start using prepared statements.

#10

Yes, i understand that this is old and outdated code, but when it was built 15 years ago, it was hi-tech! :slight_smile:

I also don't do this side of coding professionally so I am a bit in the dark as to the newer ways to do things. For right now, I just need something to get this page back up and running and then I can work on updating to the the newer style. I tried a script a friend gave me but it didn't work either. See below: I am willing to try to make either one of the styles to work but of course I need them yesterday.. ha ha .. 

session_start();

$_SESSION['isAuth']=false;

$con = new mysqli(xxx', 'xxx',
'xxx', 'xxx');

$myusername=$_POST['username'];
$mypassword=$_POST['password'];

$stmnt=$con->prepare("SELECT level,username FROM users WHERE username=? and password=?");
$stmnt->bind_param("ss", $myusername,$mypassword);
$stmnt->execute();

$result=$stmnt->get_result();

if($result->num_rows===1){
	$_SESSION['isAuth']=true;
	$row = $result->fetch_assoc();
	$_SESSION['id'] = $row['level'];
	$_SESSION['username'] = $row['username'];
	
}else{
	if($result->num_rows!==0){
		die("Weirdness!");
	}
}

$con->close();

if($_SESSION['isAuth']){
     header("location:home.php");
}

else {
header("location:error.php");
#11

See how the example code looks? That should give you a big clue as to one thing that's wrong.

I can't speak for others, and nothing personal, but personally I have no inclination to help anyone do something that I know is the wrong way. I see no reason why it shouldn't be brought up to date especially while it's being reworked. I would be the last to discourage anyone from exploring unorthodox approaches, but I am more a professional (as in always do the best I can) than I am a pragmatist (do only enough until it "works").

That said, if you are willing to bring the code up to date to get the page up rather than patching it over, I'll be glad to help.

#12

You are exactly correct and I know that also so yes, I will do it the correct way. That being said, the new code I posted with the statements now works perfectly fine once I added the final } in place.. DOH!!!!!

#13

Ok. It looks like your friend knows a little bit of what he's doing. But it's no entirely correct. The first issue I can already see is the $_POST variables that are just sitting there. If this file gets accessed directly, then you'll get an Undefined Index error message pertaining to those 2 $_POST variables. For this, you need to check whether or not the request was through a POST request.

The second thing I can see that is dangerous and wrong is that the passwords are stored as plain text. Since you said you don't code professionally, I will give you the benefit of the doubt and explain to you why this is dangerous and wrong.

Story time

So many moons ago when PHP was first created and no one really cared about security, people used to create their own ways of dealing with passwords. Some used to just store them as plain text. Plain text means that they are being stored as you see them. So for example, if the password is something like password123. That's exactly what you see in the database. There's nothing to protect them. If a hacker was able to get into your database and look at those passwords, then you are screwed.

A while back, Reddit had a huge backlash because of this exact problem. What they thought was to store user passwords as plain text so that it would be easier for the user to see when they requested a password reset. However, when their database was hacked, the entire user base was at risk. Well, beyond at risk. They were screwed. If you used the same email and passwords for other sites, then good game.

Others have learned to use password hashing algorithms. Password hashing algorithms in its nature is an extremely difficult topic to get into. They also call this Cryptography. Cryptography requires a huge amount of knowledge in security and the different algorithms that exist today. This isn't your type of "Hey, let's mash this password with that password". No. You have to understand what amount of bytes are required to make a strong hashing algorithm, you have to understand what length is required for one, and so forth. The topic is out of this range for you and me because even if you "think" you have created a strong password hashing algorithm, it's actually going to be super weak compared to one that cryptography experts make. I don't want to be going too deep into this because you won't understand it so I'll just suggest you to use the default password_hash() and password_verify() functions.

The next problem I can see is that you are trying to use fetch_assoc, but fetch_assoc isn't part of stmt. stmt is prepared statements.

There are more problems, but I'll just let that information sink in first.

#14

Thank you very much for that info and we (he) is looking into making those changes. I'll repost with what he came up with. He is not a pro either. He knows enough to be dangerous.. I know enough to be super dangerous. lol..

#15

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.