mysql_real_escape_string on registration page

Question

I was hoping someone could help me with a small script I am writing, my main goal is to just make a registration page as secure as possible and thought the best place to start would be using mysql_real_escape_string however it wont keep I just keep getting error at line 1 so here's my code:

<?php
if(isset($_POST['submit'])){
    if($_POST['username'] == "" || $_POST['password'] == ""){
        header("Location: tryagain.php");
        exit; 
    }else{
        mysql_connect("localhost", "root", "Root") or die(mysql_error());
        mysql_select_db("test") or die(mysql_error());
        $username = $_POST['username'];
        $password  = $_POST['password'];
        $sql = sprintf("INSERT into login(id,username,password) values('','%s','%s'", mysql_real_escape_string($username), mysql_real_escape_string($password));
        $result = mysql_query($sql) or die(mysql_error()) ;
        echo "Congratulations it worked woooo";
    }
}
?>

and heres the html

<form method="post" action="sql.php">
    <table>
        <tr>
            <td>
                <input type="text" name="username"/>
            </td>
            <td>
                <input type="text" name="password"/>
            </td>
            <td>
                <input type="submit" name="submit" value="submit">
            </td>
        </tr>
    </table>
</form>

If i change the $sql statment to this the code works fine

$sql = "INSERT into login(id,username,password) values('','$username','$password')";

Can anyone see what I've done wrong :S it works perfectly fine when I adjust it to log in using real escape.

Also what other methods can I use to validate data? I plan on making the if statements check for only number and letters, and just prevent any special characters all together. Thanks.

On a side note, yes I know mysqli and pdo should be used not mysql sadly were I'm at they don't use them.

The `mysql_*` functions are **no longer maintained** and shouldn't be used in any new codebase. It is being phased out in favor of newer APIs. Instead you should use [**prepared statements**](https://www.youtube.com/watch?v=nLinqtCfhKY) with either [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli). — tereško, Dec 17 '13 at 17:02
Guys come on, i even state why i have to use that :/ id much rather use pdo but cant — jphillip724, Dec 17 '13 at 17:05
@JohnConde `mysql_query` in general is an extremely bad idea, but in this case the values are properly escaped. Not everyone has the luxury of working for a company that uses 21st century technology. — tadman, Dec 17 '13 at 17:05
Last but not least, storing passwords in clear is the best way to make your server worth hacking. — Álvaro González, Dec 17 '13 at 17:05
@jphillip724 Which error are you getting at line 1? Can you copy and paste it? — Homer6, Dec 17 '13 at 17:08
@ÁlvaroG.Vicario that was just a quick example typed up for this — jphillip724, Dec 17 '13 at 17:10
@Homer the error was just the generic check your manual for the correct syntax, its fixed now thought :) — jphillip724, Dec 17 '13 at 17:11
Yes, that's a MySQL error, not a PHP error, your *SQL syntax* was wrong. — Marcel Korpel, Dec 17 '13 at 17:12

score 4 · Accepted Answer · answered Dec 17 '13 at 17:03

4

Your SQL statement is broken:

INSERT into login(id,username,password) values('','%s','%s'

Should be

INSERT into login(id,username,password) values('','%s','%s')

answered Dec 17 '13 at 17:03

Rayne

2,620
20
28

It's just missing a bracket? This would be caught with proper error handling. – tadman Dec 17 '13 at 17:04
Thanks this worked, and @tadman yeah im suprised it didnt show up, im using error_reporting(E_ALL); ini_set('display_errors', '1'); so that should display all errors :S – jphillip724 Dec 17 '13 at 17:07
Maintaining a codebase like this can't be good for your health. `mysql_query` promotes all kinds of extremely dangerous habits. Don't forget that kind of error reporting only reports PHP errors, not database errors. This why things like [Doctrine](http://doctrine-project.org/) and [Propel](http://propelorm.org/) exist, insulating you from this ugliness. – tadman Dec 17 '13 at 17:08
@tadman It's not a syntax error in PHP, but an SQL syntax error, that's why PHP shouts something about 'line 1'. – Marcel Korpel Dec 17 '13 at 17:09
@tadman With respect, maybe you can ease off on the criticism. What you've done is make the guy feel bad when we had already picked up on your disgust for the codebase 10 minutes ago. Yeah, we get it, the mysql_* interface is being deprecated. But some people have to work to make money. And, if he's told to do it a certain way, you have to respect his right to earn a wage. Ideals are only possible when there's food in your belly. After all, this is a community to help people, not to bash them or to demonstrate your intelligence by how witty and cutting your sarcastic remarks can be. – Homer6 Dec 17 '13 at 17:18
Lastly, if you are rough with people when they ask for help, it will scare them into not asking questions. And from that, codebases everywhere will suffer. Seriously, I meant with respect, your reputation on SO means that you've helped a lot of people and everyone appreciates that. But, a gentler approach is more ideal. Thanks for you consideration. – Homer6 Dec 17 '13 at 17:22
@Homer6 I do try to be civil, but there's this unending deluge of people struggling with `mysql_query` when there's far better alternatives. In this case there's a business requirement to use it, which is unfortunate. Remember, `mysql_query` is a relic from the Windows 95 era, yet people *insist* on using it. I do try to be as gentle as possible, but I will not go so far as to *condone* developing code this way. This short snippet in the question is rife with **serious** issues. – tadman Dec 17 '13 at 17:23
If you have any ideas on how to motivate people to drop `mysql_query` whenever possible or practical, I'd like to know. It seems extremely hard to move people to PDO, but I have seen a lot of encouraging signs after beating this drum. Sorry about the noise, but it is necessary. – tadman Dec 17 '13 at 17:24
like i said tadman, i'd rather be using PDO, however no one else knows anything about it and wont move, im also trying to update an old application with 0 sql protection, 0 sanitisation etc other devs don't have a clue about this and this is my first job (Even if temporary) im still at uni and even the uni is teaching mysql, could easily switch my mysqli atleast, hell they even use front page for year 1 and 2 and keep in mind this is secotlands "leading" univesirty for this, awards and everything – jphillip724 Dec 17 '13 at 17:32
1

Fair enough. Maybe, if we had a good article on all of the reasons not to use mysql_* and also are provided with simple, boilerplate examples of what to use instead and why, that would help sway people. That way, when the issues surfaces again, everyone could link to it. It could also be presented to others (by developers that wanted to change their codebase) in an organisation as a strong argument to convince people that would otherwise be slow to agree. – Homer6 Dec 17 '13 at 17:32
Or you could take a page from my book: just do it your way and if you get fired (which you won't), you can find a new job. But more often than not, they like what you've done and adopt it anyways. I think one programming practice has helped me more than any other: "Anytime you touch a keyboard, write the absolute best code that you possibly can (with strong commenting)." – Homer6 Dec 17 '13 at 17:39
1

@Homer6 There *are* examples of good practice on this side, like in http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php and http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php More boilerplate code is in http://bobby-tables.com/php.html – Marcel Korpel Dec 17 '13 at 23:31

score -1 · Answer 2 · answered Dec 17 '13 at 17:05

-1

Not sure what is your issue, I'm thinking sprintf. Maybe try this :

    $username = mysql_real_escape_string($_POST['username']);
    $password  = mysql_real_escape_string($_POST['password']);
    $sql = sprintf("INSERT into login(id,username,password) values('','%s','%s', $username, $password)");

answered Dec 17 '13 at 17:05

Eric

9,870
14
66
102

Not to mention you spam comments without answering the question, we don't need this. – Eric Dec 17 '13 at 17:15
Raz found the problem and posted an answer. This bit of broken code isn't helping. – tadman Dec 17 '13 at 17:22

mysql_real_escape_string on registration page

2 Answers2