Why doesn't true = true

W

Wang191

[H]ard|Gawd
2FA
Joined
Nov 30, 2000
Messages
2,026
PHP: 
		function updatePassword($id, $username, $oldpassword, $password){
				global $databaseHostName, $databaseUserName, $databaseUserPassword, $databaseName;

				$connect = mysql_connect($databaseHostName, $databaseUserName, $databaseUserPassword);
				mysql_select_db($databaseName, $connect);
				$query = "UPDATE users SET password='$password' WHERE id='$id' && password = '$oldpassword'";
				if(mysql_query($query)){					
					return true;
				}
				mysql_close($connect);
		}

PHP: 
				if(updatePassword($id, $username, $oldpassword, $password) == true){
					$f = 'update successful';
				}else{
					$e = 'update failed';
				}

No matter what happens I always get "update successful"
even when I don't put in the right password thus forcing the query to not update.

What am I missing?
 
the query is still technically successful, as in there wasn't a syntax error. you probably want to look into mysql_affected_rows(). i do hope you intend to properly escape the variables you insert into your query to prevent injection.

also beware that if the query is successful mysql_close() is never called and it might not be a bad idea to explicitly return false just to make it less ambiguous (in php null is returned by default if you don't specify which would evaluate to false in the conditional so while the behavior is ultimately the same adding return false explicitly just indicates your intent).
 
Thanks everyone.
I'll make those changes.
I did look into mysql_affected_rows() before but it didn't seem to work. I guess I didn't dig deep enough yet.

As for escaping the inputs I plan to implement that as soon as I get the rest of the functionality down. I put the code for all user input fields at the top of my pages so I didn't miss any of them.
 
I got it working. I was using mysql_affected_rows() improperly before. Thanks!
 
what are the accepted practices for accounting for SQL injection?
I was planning on escaping all user input and anywhere I may have used $_GET for checking values.
I have been reading that escaping the inputs is not enough though. Some suggest limiting input characters to a certain subset. a-z 0-9 and @.
For this application I don't see a problem with that but I don't know if that's how I should proceed. I'm sure there are scenarios I'm not thinking of where I would want more characters.
 
Wang191 said:
what are the accepted practices for accounting for SQL injection?
I was planning on escaping all user input and anywhere I may have used $_GET for checking values.
I have been reading that escaping the inputs is not enough though. Some suggest limiting input characters to a certain subset. a-z 0-9 and @.
For this application I don't see a problem with that but I don't know if that's how I should proceed. I'm sure there are scenarios I'm not thinking of where I would want more characters.
Click to expand...

Valid email addresses can also contain a + sign.

Limiting passwords to alphanumeric only is a dick move ;)
 
ah yes the + sign.

Yeah I do hate it when I can't use special characters in a password field. This is going to get tricky.
 
aside from a max length, i don't see any reason to restrict what you can enter for a password, unless you want to enforce stuff like at least one letter and one number but that's kind of a separate thing.

as far as avoiding sql injection, i suggest using php's mysqli (i for improved) extension, specifically look into prepared statements. here's a good article: http://devzone.zend.com/article/686
 
Tawnos said:
Valid email addresses can also contain a + sign.

Limiting passwords to alphanumeric only is a dick move ;)
Click to expand...
It gets worse- strictly speaking, an email address can contain an apostrophe. Someone at work has one. Yeah, he's caused me a lot of headaches. :)
 
Wang191 said:
what are the accepted practices for accounting for SQL injection?
I was planning on escaping all user input and anywhere I may have used $_GET for checking values.
I have been reading that escaping the inputs is not enough though. Some suggest limiting input characters to a certain subset. a-z 0-9 and @.
For this application I don't see a problem with that but I don't know if that's how I should proceed. I'm sure there are scenarios I'm not thinking of where I would want more characters.
Click to expand...

If you are escaping inputs then you are already doing it ghetto. There's a reason parameterized queries exist. Since you are using PHP, at least use MYSQLI so you can take advantage of the parameterized queries. SQL injection should be a thing of the past.
 
Sgraffite said:
If you are escaping inputs then you are already doing it ghetto. There's a reason parameterized queries exist. Since you are using PHP, at least use MYSQLI so you can take advantage of the parameterized queries. SQL injection should be a thing of the past.
Click to expand...

This ^

Paramaterized queries will put in formatting you need and scrub against injection.

As for SQL injection... it *should* be programming 101 but it's how Sony was compromised....


I hope you're not storing your passwords in plaintext in your DB?
They should be stored as salted, irreversible hashes.
 
I barely know php and sql, but mysql_close($connect) never executes when the function returns true; not sure if this is problematic or not.
 
I never realized that upon return that it didn't finish executing the function. I will change the way that happens.

As for the passwords, the first thing I taught myself when I started this project was to salt and hash the passwords before they go into the database.
 
You must log in or register to reply here.
Back
Top