PHP/MySQL Registration -- I'm a newb

Question

Okay, as I said, I have no experience whatsoever in this kind of code. I just know the very, very basics of PHP and a slight impression as to how MySQL works.

So this is the code I'm using (password blocked out with stars)

  <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
"http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<title>P.S.</title>
<link type="text/css" rel="stylesheet" href="http://www.ps.niu-niu.org/ps.css" /> 
</head>
<body>
 <div id="main">
HERE'S THE MAIN PART!
<?php 

 mysql_connect("localhost, "niuniu_ps", "**********") or die(mysql_error()); 

 mysql_select_db("niuniu_ps") or die(mysql_error()); 


 if (isset($_POST['submit'])) { 



 if (!$_POST['username'] | !$_POST['pass'] | !$_POST['pass2'] ) {

        die('You did not complete all of the required fields');

    }





    if (!get_magic_quotes_gpc()) {

        $_POST['username'] = addslashes($_POST['username']);

    }

 $usercheck = $_POST['username'];

 $check = mysql_query("SELECT username FROM users WHERE username = '$usercheck'") 

or die(mysql_error());

 $check2 = mysql_num_rows($check);





 if ($check2 != 0) {

        die('Sorry, the username '.$_POST['username'].' is already in use.');

                }




    if ($_POST['pass'] != $_POST['pass2']) {

        die('Your passwords did not match. ');

    }





    $_POST['pass'] = md5($_POST['pass']);

    if (!get_magic_quotes_gpc()) {

        $_POST['pass'] = addslashes($_POST['pass']);

        $_POST['username'] = addslashes($_POST['username']);

            }





    $insert = "INSERT INTO users (username, password)

            VALUES ('".$_POST['username']."', '".$_POST['pass']."')";

    $add_member = mysql_query($insert);

    ?>




 <h1>Registered</h1>

 <p>Thank you, you have registered - you may now login</a>.</p>

 <?php 
 } 

 else 
 {  
 ?>



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

 <table border="0">

 <tr><td>Username:</td><td>

 <input type="text" name="username" maxlength="60">

 </td></tr>

 <tr><td>Password:</td><td>

 <input type="password" name="pass" maxlength="10">

 </td></tr>

 <tr><td>Confirm Password:</td><td>

 <input type="password" name="pass2" maxlength="10">

 </td></tr>

 <tr><th colspan=2><input type="submit" name="submit" 
value="Register"></th></tr> </table>

 </form>


 <?php

 }
 ?>

</div>
<div id="reflection"></div> </body>

When I go to ps.niu-niu.org/, it shows up as: Parse error: syntax error, unexpected T_STRING in /home/niuniu/public_html/ps/index.php on line 27

I edited your code before reading the question, and I think I just edited out the error. You forgot a double quote (") after localhost in the mysql_connect function. It's not line 27 though, but line 27 leads to a blank line in your code if I'm correct. — Vincent Savard, Aug 13 '11 at 00:59
Also, `|` is the bit or operator, you're probably looking for `||` which is the or operator. — Vincent Savard, Aug 13 '11 at 01:02
Amy, you seriously need to read about SQL injection attacks. You set $usercheck directly from the $_POST collection and then use it in a query that isn't safely escaping the string (either manually or by using a parameterised query). This could lead to a disaster if I were to type "*'; DROP TABLE users;" into the username field. — Fenton, Aug 13 '11 at 01:04
@Vincent Savard - I have removed the over-enthusiastic edit, which was a bit confusing. It is always better to answer, rather than correct the error in the question. — Fenton, Aug 13 '11 at 01:06
@Sohnee: Don't get me wrong, it was a mistake. I didn't read the question first (because I hate looking at poor colored code :p) so just fixed it, in retrospective I should have removed it right away instead of commenting. Thanks for doing it. — Vincent Savard, Aug 13 '11 at 01:08
@Amy Dee, Why would you change the accepted answer to this question? I took the time to read your question, find the problem you were having and then post the answer which you accepted. Then you read another response, which was really a comment that had already been made by others, which had nothing to do with the question you asked and chose to change the accepted answer. However true the comments made by the second responder they are not an answer to your question at all. — Night Owl, Sep 10 '11 at 17:33

score 3 · Answer 1 · answered Aug 13 '11 at 00:58

It looks like you are missing a quote around localhost in your connection string...

 mysql_connect("localhost, "niuniu_ps", "**********") or die(mysql_error());

should be...

 mysql_connect("localhost", "niuniu_ps", "**********") or die(mysql_error());

Also this line...

if (!$_POST['username'] | !$_POST['pass'] | !$_POST['pass2'] ) {

should be...

if (!$_POST['username'] || !$_POST['pass'] || !$_POST['pass2'] ) {

the single pipes are bitwise operators and wont give the expected results. Probably not related to your error though.

score 1 · Accepted Answer · edited May 23 '17 at 12:26

This doesn't specifically answer your question of "why doesn't this work", but it addresses a much more serious concern: your entire system is a gaping security hole. I'm sorry, but it's true.

  $insert = "INSERT INTO users (username, password)

            VALUES ('".$_POST['username']."', '".$_POST['pass']."')";

  $add_member = mysql_query($insert);

It appears that you're inserting the password into the database as plain, unencrypted text. This is a very bad idea. If your database is somehow compromised, then all of your users' passwords are right there, and can easily be stolen. Most users use the same password on many sites, so not only is your own site compromised, but your user's credentials to other sites may be as well.

Furthermore, your code is wide-open to SQL injection. You are not doing anything to sanitize the input when checking usernames. It would be trivial for an attacker to insert some code into the username box instead of a username, and get all of your database records. If we set the username as ' or '1'='1, then your query becomes:

SELECT username FROM users WHERE username = '' or '1'='1'

This will return all usernames, and the vulnerability could be further abused to reveal all of your unencrypted passwords! Not a very safe way to be treating your customers' data, and dangerous because your data could all be stolen and then erased, all without logging in.

The fact of the matter is that many developers — even those with self-professed experience, let alone somebody who's barely used PHP before — store passwords incorrectly. It would be much safer for both the safety of your application and your users for you to scratch writing a custom authentication system, and use something that's well established and works. There is a Stack Overflow post on PHP libraries for user authentication (don't listen to the guy who says to roll your own - his own example is rife with vulnerabilities, proving my point).

This is all true but not at all related to the original question so how can the accepted answer to a question start with "This doesn't specifically answer your question"? — Night Owl, Aug 25 '11 at 14:56
@NightOwl the question asker chooses the accepted answer. Your answer does indeed provide the solution to the implied question of "why am I getting this error", but the original question _doesn't even ask a question_ - it just dumps some code and an error, and the question is implied. My answer points out a much more important problem, namely that the whole code block is risky. The asker probably felt that was more important feedback. It's a matter of looking at the bigger picture. — nhinkle, Aug 26 '11 at 22:55

PHP/MySQL Registration -- I'm a newb

2 Answers2