PHP/MySQL form doesn't display output

Question

I am new to both PHP and MySQL, however I am currently learning both in order to create a basic website to administrate my RADIUS accounts. (I've switched to using MySQL for user authentication)

I have made a script that inserts user values into the database, however I need to make it so as if a user account already exists, a duplicate can't be created.

I was using a tutorial I found to try and create this, and it does work to an extent. When I add the user when there is no duplicate account, I am returned the message "Successful" however when there is a duplicate user, the page that is returned is blank, and I'm not sure what I'm doing wrong. Its workable, but annoying.

I am also curious if it is possible to create a pool of numbers (These will be IP Address's) and then have it so that when a new user is created, a number from this pool is used, and then removed from that pool. By doing this I hope I could automate assigning IPs to users without having to have someone manually add them every time a new user is created. Currently to achieve a slightly less desirable approach I made another script that displays a list of users IP address's from my table so that one can just add an IP that isn't on this list. Any advise on where I could learn to do this would be greatly appreciated, I don't know where to start.

Below is the php code that I am using, and the html code for the form. Thank you for any help or advise.

<?php

define('DB_HOST', 'localhost');
define('DB_NAME', 'test');
define('DB_USER','root');
define('DB_PASSWORD','123');

$con=mysql_connect(DB_HOST,DB_USER,DB_PASSWORD) or die("Failed to connect to MySQL: " .     mysql_error());
$db=mysql_select_db(DB_NAME,$con) or die("Failed to connect to MySQL: " .      mysql_error());


function AuthAccount()
{
    $username = $_POST['username'];
    $value = $_POST['value'];
    $query = "INSERT INTO radcheck(username, attribute, op, value) VALUES ('$username',       'Cleartext-Password', ':=', '$value')";
    $data = mysql_query ($query)or die(mysql_error());
    if($data)
    {
    echo "User added to authentication database";
    }
}

function AddAccount()
{
if(!empty($_POST['username']))   
{
    $query = mysql_query("SELECT * FROM radcheck WHERE username = '$_POST[username]'")     or die(mysql_error());

    if(!$row = mysql_fetch_array($query) or die(mysql_error()))
    {
        AuthAccount();
    }
    else
    {
        echo "Username already registered";
    }
}
}
if(isset($_POST['submit']))
{
    AddAccount();
}
?>

Sign-Up Page:

<!DOCTYPE HTML>
<html>
<head>
<title>Sign-Up</title>
</head>
<body id="body-color">
<div id="Sign-Up">
<fieldset style="width:50%"><legend>Registration Form</legend>
<table border="0">
<form method="POST" action="SignUp.php">
<tr>
<td>Username</td><td> <input type="text" name="username"></td>
</tr>
<tr>
<td>Password</td><td> <input type="text" name="value"></td>
</tr>
<tr>
<td><input id="button" type="submit" name="submit" value="Sign-Up"></td>
</tr>
</form>
</table>
</fieldset>
</div>
</body>
</html>

Security is always a concern -- what if one of these staff members accidentally posted something that destroyed your database? — Sterling Archer, Nov 18 '13 at 05:29
I think mysql* functions are deprecated, instead use mysqli* functoins — Developerium, Nov 18 '13 at 05:30
1. please refrain from using mysql* extensions , they have been deprecated , try using mysqli* or pdo* — Satya, Nov 18 '13 at 05:30

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

Your approach is riddled with problems:

You have introduced a race hazard, whereby two sessions simultaneously attempting to register the same username may both end up passing through the SELECT test before either one has proceeded to INSERT.

If you are using a transactional storage engine (such as Innodb), one can resolve this issue through proper use of transactions and locking reads, but it's far easier simply to impose a uniqueness constraint within the database and leave MySQL to do the rest:
```
ALTER TABLE radcheck ADD UNIQUE (username)
```
You can then simply go straight to INSERT and, if the username already exists, an error will be raised that you can handle accordingly.
You are not escaping your string literals when inlining them into your SQL. This is not only a (serious) security vulnerability, but it also introduces a bug whereby your code will break should someone post a username or password that contains a ' character. Passing literal values as parameters to prepared statements avoids these issues.
You are using an ancient PHP extension to access MySQL. It has not been updated since 2006 and its use has been explicitly discouraged in the PHP manual since 2011. As of PHP v5.5, it is deprecated and it will be removed from PHP altogether in a future version. You should switch to either the improved MySQL extension, PHP Data Objects or a third party abstraction layer.
You are storing plaintext passwords in your database. This not only poses a considerable risk to the security of your application in the event that your database is compromised, but it furthermore poses much wider risks to your users (since it is likely that they use the same passwords for other accounts). For their sake, you should always hash every password with salt; you should also be careful to protect their passwords between their browser and your server, e.g. using HTTPS.

As regards the assignment of IP addresses, your question provides no detail on how such addresses are chosen or for what they are used. Usually one leaves such matters to existing services such as DHCP, which only require configuring according to your policy requirements.

Thank your for your advise. I was a little unclear with the IPs but I have found a solution, in this case I can't use DHCP unfortunately. I'm now hashing my passwords and I have made a new php page and using PDO instead of mysql_* based on your advise and advise of @steven and its working great! Cheers. And now names like O'Brien don't break my script! — user3003333, Nov 18 '13 at 10:08

steven · Answer 2 · 2013-11-18T08:31:56.987

i think if(!$row = mysql_fetch_array($query) or die(mysql_error())) is not correct.

I would do it like this:

class db {
    public static function dbFactory($host, $dbase, $user, $pass) {
        $pdo = new PDO("mysql:host=$host;dbname=$dbase", $user, $pass);
        $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);        
        return $pdo;
    }
}
$db = db::dbFactory('localhost','mydbname','myusername','mypassword');

$db->beginTransaction();
try {

    $stmt = $db->prepare("SELECT COUNT(*) as cnt FROM radcheck WHERE username = :uname");
    $stmt->bindValue(':uname',$_POST[username]);
    $stmt->execute();
    $res = $stmt->fetch(PDO::FETCH_ASSOC);

    if($res['cnt'] == 0) {
        // go on ...
    }

    $db->commit();

} catch (Exception $e) {
    $db->rollBack();
}

Beware the race hazard (point #1 in my answer) still exists here. — eggyal, Nov 18 '13 at 06:11
@eggyal thanks, you are right. Tried to solve this issue by using a transaction. Do you think its secure now without adding uniqueness? — steven, Nov 18 '13 at 08:39

PHP/MySQL form doesn't display output

2 Answers2