-2

I'm working on a Login System for my website, however when I insert all the information (User, password, Email) Into the databse for the sigm-up function, it is setting all the variables to 0. I know that it is not an issue with passing variables, because I have echo'd the Username and Password and they are at what they are meant to be. The code for my sign-up page is as follows:

<?php

include '../includes/conn.php';
include 'salt.php';

if($_POST['signup']){

    $user = $_POST['user'];
    $pass = $_POST['pass'];
    $cpass = $_POST['cpass'];
    $email = $_POST['email'];

        if($pass == $cpass){
            $hpass = create_hash($pass);

            $query = $conn->prepare("INSERT INTO Users (Name, Password, Email) VALUES (?, ?, ?)");
        $query->bind_param('sss', $user, $hpass, $email);
        $query->execute();

            if($query){
                $msg = 'Account created successfully, please check your email to verify it.';
            }else{
                $msg = 'There was an error creating your account: ' . $conn->error . ', please try again later';
            }

            //echo $user . ' ' . $pass . ' ' . $hpass . ' ' . $email;

        }else{
            $msg = 'Passwords do not match.';
        }
}

?>

<html>

    <head>

        <title>DiscFire Softworks - Login test</title>

        <link rel="stylesheet" type="text/css" href="../includes/ie-styles.css">

        <style type="text/css">

            @import url('../includes/styles.css');

        </style>

    </head>

    <body>

        <div class="body">

            <img src="../images/header.jpg" />

            <div class="navbar">

                <?php

                    $query = $conn->prepare("SELECT Name FROM pages ORDER BY ID asc");
                    $query->execute();
                    $query->bind_result($name);

                    while($query->fetch())
                    {
                        echo '<a href="/?page=' . $name . '">' . $name . '</a>';
                    }

                ?>

            </div>

            <?php
                echo '<p>' . $msg . '</p>';
            ?>

            <form method="POST" action="index.php" id="sign-up">

                <input type="hidden" name="signup" value="1"/>
                <label for="user">Username: </label>
                <input type="text" style="width: 30%; margin-left: 59px;" name="user"></textarea>
                <br />
                <label for="pass">Password: </label>
                <input type="password" style="width: 30%; margin-left: 60px;" name="pass"></textarea>
                <br />
                <label for="cpass">Confirm Password: </label>
                <input type="password" style="width: 30%; margin-left: 1px;" name="cpass"></textarea>
                <br />
                <label for="user">Email: </label>
                <input type="text" style="width: 30%; margin-left: 90px;" name="email"></textarea>
                <input type="submit" />
            </form>

        </div>

    </body>

</html>

Here is the SQL Structure, as requested by @Prix:

http://prntscr.com/2vsmjv

Thanks in advance!

  • 1
    It looks like you have significant SQL injection vulnerability. Also, why check for the existing user first and tell the end user that an account already exists? That is generally frowned upon from a security standpoint and actually wastefully adds an extra query call against the database. Simply go for the inset, and if it fails because of unique contraints, just tell the user that the account cannot be created. Can you show a dump of your query before it is executed? – Mike Brant Feb 24 '14 at 18:16
  • 1
    why are you using `textarea` instead of `input` fields? You know they can use enter and several other character that can break your login system if not properly sanitized? Also you should make `Name` a unique field so you don't have to waste a query just to know if its open or not. You're already using MySQLi it seems why not use prepared statements so you can be safe against injection? There is no point moving from mysql_* library into MySQLi if you're going to repeat the same mistakes. – Prix Feb 24 '14 at 18:18
  • Looks like you have significantly nonsensical table declaration. – Your Common Sense Feb 24 '14 at 18:20
  • @prix: and exactly how does switching to an input field prevent injection problems? You realize that textarea and input are fundamentally identical once the form's submitted? they're both just `key=value` in the data stream. – Marc B Feb 24 '14 at 18:26
  • @programmingturtle Please look at using this library for db connection/interaction. http://www.doctrine-project.org/projects/dbal.html – Jarrod Nettles Feb 24 '14 at 18:26
  • @MarcB no they aren't as some of the input differ either way read the entire comment before complaining I do list prepared statements. – Prix Feb 24 '14 at 18:27
  • OK, I've changed it to and – ProgrammingTurtle Feb 24 '14 at 18:33
  • So much you can work on this. 1) [Make the `Name` field UNIQUE](http://stackoverflow.com/a/5038052/342740) 2) [Remove your first query as it is now useless and properly handle the errors to see if Name is already taken](http://stackoverflow.com/a/8449628/342740) 3) [Use prepared statements or properly sanitize your data to prevent SQL injection](http://www.php.net/manual/en/mysqli.quickstart.prepared-statements.php) 4) [Properly handle the error messages so you know what is going on](http://us.php.net/mysqli_error) – Prix Feb 24 '14 at 18:37
  • None of this really helps me... also I don't want to use a library. @Mike Brant I'm planning on doing that afterwards :) nobody but me has access so far – ProgrammingTurtle Feb 24 '14 at 18:40
  • You are already using a library, that's what MySQLi is. Yeah knowing what the real error is doesn't help, properly preventing SQL injection doesn't help, avoiding unnecessary database querying doesn't help, then I guess you don't need help. **Not to mention the fact your insert is always 0 could be due to the fact your data is not sanitized breaking your query.** – Prix Feb 24 '14 at 18:41
  • @Prix made it unique, made it handle the errors, I'll just update the code – ProgrammingTurtle Feb 24 '14 at 18:45
  • @Prix what do you mean by sanitized? Sorry, I only started PHP and MySQLI a few months ago, don't know everything yet. – ProgrammingTurtle Feb 24 '14 at 18:46
  • @ProgrammingTurtle You really should not succumb to the thought of "I will add security afterwards.". It should be a consideration right from the start, unless you just enjoy re-designing your applications needlessly. – Mike Brant Feb 24 '14 at 18:47
  • @ProgrammingTurtle then read all my past comments including the links on each so you can understand. – Prix Feb 24 '14 at 18:48
  • With every example for prepared statements that I find, none of them show me how to have more than one parameter. Can anyone help? – ProgrammingTurtle Feb 24 '14 at 19:01
  • [The very first example over here shows it](http://br2.php.net/manual/en/mysqli-stmt.bind-param.php) – Prix Feb 24 '14 at 19:42
  • @Prix OK, I've started using prepared statements, but yo still haven't answered my question. Can you start answering my REAL question now? – ProgrammingTurtle Feb 25 '14 at 19:05
  • @ProgrammingTurtle I've already told you on my previous comments where the issue might be however without proper coding, error handling, it will not be possible to pinpoint the issue. You've changed it to prepared but you're still not using error handlers or anything else. – Prix Feb 25 '14 at 19:07
  • @Prix done the first 3, onto the 4th – ProgrammingTurtle Feb 25 '14 at 19:08
  • @ProgrammingTurtle also if you can post your MySQL table structure that did help too just to make sure it matches what you're trying to insert into it. – Prix Feb 25 '14 at 19:09
  • @ProgrammingTurtle you cannot insert string into int field that's why they come out as 0 hahahaha :) change it to varchar – Prix Feb 25 '14 at 19:21
  • Oh... It seems my user and pass colums decided to make themselves int's... well that solves that problem – ProgrammingTurtle Feb 25 '14 at 19:22
  • Oh also, it seems I accidentally made h=the password column rather than users unique... *facepalms* – ProgrammingTurtle Feb 25 '14 at 19:25
  • @MikeBrant: _“why […] tell the end user that an account already exists? That is generally frowned upon from a security standpoint […]”_ – while that is generally true from a _pure_ security standpoint … _“Simply go for the inset, and if it fails because of unique contraints, just tell the user that the account cannot be created”_ – just telling them that with no explanation that they should chose another user name is not user friendly at all. I think one has to make some sort of compromise here. – CBroe Feb 25 '14 at 19:27
  • @ProgrammingTurtle check this example: http://pastebin.com/xNS6ZiF6 it is untested just to illustrate the unique field when failed and error handling. – Prix Feb 25 '14 at 19:33
  • @CBroe I would disagree. I would simply message the user something like "Unable to create account. Please try a different username/password combination." Very to the point and tells the user exactly what they need to do. – Mike Brant Feb 25 '14 at 23:28
  • Well, they probably have chosen their favorite username already, so they will just try with a different password – and get the same message again. Frustrating, not clear why this is happening … and either leaving the site after a couple of fruitless attempts or write an email to support … – CBroe Feb 26 '14 at 11:13
  • @CBroe and Mike Brant, I agree with CBroe, why go through all the hassle of getting an e-mail then having to reply, when you could just tell the user what to do? – ProgrammingTurtle Feb 27 '14 at 16:07
  • hasn't anyone bothered posting the solution as an answer yet? – ProgrammingTurtle Mar 04 '14 at 20:30

1 Answers1

-1

Seems I set the row's to integers... whoops.