Page 1 of 2 12 Last
  • Jump to page:
    #1
  1. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0

    I need help with security


    hello everyone, im 14 years old and
    i've been programming php for about 7 months now, I started oop php for about 1 month ago and mysqli too. it all looks fine, and i've created a cms.
    But my "friend" keeps finding sql vulnerables, it's in the register. When looking if a user exists in the database, then kill the script.
    He says that he has found about 21 sql vulnerables in the cms, and I don't know if it's true or not.

    this is how my code looks like right now. my "friend" says that he can hack it whenever he wants. So i'm kinda scared that my code is not fully secure. he wanted me to pay him $50, and he'll show me how to secure it, but i'm not sure if he's just trolling me.


    Anyways here's my code:

    pastebin.com/G1edaCkL

    Sorry for my bad english, i'm from sweden.
  2. #2
  3. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by pumpkin
    hello everyone, im 14 years old and
    i've been programming php for about 7 months now, I started oop php for about 1 month ago and mysqli too. it all looks fine, and i've created a cms.
    But my "friend" keeps finding sql vulnerables, it's in the register. When looking if a user exists in the database, then kill the script.
    He says that he has found about 21 sql vulnerables in the cms, and I don't know if it's true or not.

    this is how my code looks like right now. my "friend" says that he can hack it whenever he wants. So i'm kinda scared that my code is not fully secure. he wanted me to pay him $50, and he'll show me how to secure it, but i'm not sure if he's just trolling me.


    Anyways here's my code:

    pastebin.com/G1edaCkL

    Sorry for my bad english, i'm from sweden.
    Sorry everyone, i made a failed a bit, i mixed up stmp or what its called.
    here's the new code:


    public $rusername;
    public $checkUser;
    $existsUsername = $this->connection->real_escape_string(trim(htmlspecialchars(strip_tags(stripslashes($this->rusername)))));
    $this->checkUser = $this->connection->prepare("SELECT * FROM users_table WHERE username=?");
    $this->checkUser->bind_params('s' , $existsUsername);
    $this->checkUser->execute();
    $results = $this->checkUser->get_result();

    if($results->num_rows > 0){
    die("user exists!");
    }
  4. #3
  5. No Profile Picture
    Contributing User
    Devshed Loyal (3000 - 3499 posts)

    Join Date
    Jul 2003
    Posts
    3,232
    Rep Power
    593
    Don't use pastebin for your code, post it here. That is what this forum is for. When you post it be sure to use [ PHP ] tags. See the sticky at the top of this forum that says READ THIS BEFORE POSTING.

    As for your security concerns, you are using prepared statements so you are off to a good start. I expected Jacques1 to reply to you first because he has written a good guide on the biggest security mistakes. Read it here and here. You might also check the sticky at the top of this forum that says LIST OF PHP GUIDES AND FAQs for additional tips and hints for both good programming practices and security.
    There are 10 kinds of people in the world. Those that understand binary and those that don't.
  6. #4
  7. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by gw1500se
    Don't use pastebin for your code, post it here. That is what this forum is for. When you post it be sure to use [ PHP ] tags. See the sticky at the top of this forum that says READ THIS BEFORE POSTING.

    As for your security concerns, you are using prepared statements so you are off to a good start. I expected Jacques1 to reply to you first because he has written a good guide on the biggest security mistakes. Read it and You might also check the sticky at the top of this forum that says LIST OF PHP GUIDES AND FAQs for additional tips and hints for both good programming practices and security.
    oh sorry, i've not used forums alot so i'm kinda noob-ish.

    and thank you very much, i'll check it out right now!
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Hi pumpkin,

    first of all, you need new friends. What your "friend" does is called extortion in some contexts.

    Secondly, 10 lines of code don't tell us anything about your CMS as a whole.

    This particular piece of code is immune against SQL injections, because you're using prepared statements. If you use this technique everywhere in your code and never insert raw variables into query strings, there is no risk of SQL injections.

    However, you seem to be very confused about what to do:

    PHP Code:
    $this->connection->real_escape_string(trim(htmlspecialchars(strip_tags(stripslashes($this->rusername))))) 
    Um, what on earth is this? Some kind of "Fire all weapons!" attempt of gaining security by simply applying every single function you've ever heard of? This doesn't work. It will actually corrupt your data.

    You don't need 10 wrong tools. You need one right tool. And you've already found it: prepared statements. This is the most secure and reliable way of protecting the application against SQL injections.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  10. #6
  11. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    58
    Rep Power
    1
    This isn't a technical answer, but you should not pay your friend $50 before he demonstrates he can compromise your systems.

    Back up all your data and files, and then say "Go ahead. Compromise it. Show me you can do it." If he can, maybe it is worth paying him to show you the vulnerabilities.

    The way you've described the situation sounds worse than Cryptolocker... he's asking for ransom before he's even kidnapped your systems!
  12. #7
  13. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by Jacques1
    Hi pumpkin,

    first of all, you need new friends. What your "friend" does is called extortion in some contexts.

    Secondly, 10 lines of code don't tell us anything about your CMS as a whole.

    This particular piece of code is immune against SQL injections, because you're using prepared statements. If you use this technique everywhere in your code and never insert raw variables into query strings, there is no risk of SQL injections.

    However, you seem to be very confused about what to do:

    PHP Code:
    $this->connection->real_escape_string(trim(htmlspecialchars(strip_tags(stripslashes($this->rusername))))) 
    Um, what on earth is this? Some kind of "Fire all weapons!" attempt of gaining security by simply applying every single function you've ever heard of? This doesn't work. It will actually corrupt your data.

    You don't need 10 wrong tools. You need one right tool. And you've already found it: prepared statements. This is the most secure and reliable way of protecting the application against SQL injections.
    yes, i was adding alot of s*** into the code because i thought it would help a bit, some of those codes i don't understand.
    i know mysqli_real_escape_string() and htmlspecialchars.
    anyways..
    i've updated the code:

    PHP Code:

    public $rusername;
                public 
    $checkUser;
                
    $existsUsername $this->connection->real_escape_string(htmlspecialchars(strip_tags(($this->rusername))));
                
    $this->checkUser $this->connection->prepare("SELECT * FROM users_table WHERE username=?");
                
    $this->checkUser->bind_params('s' $existsUsername);
                
    $this->checkUser->execute();
                
    $results $this->checkUser->get_result();
                
                if(
    $results->num_rows 0){
                    die(
    "user exists!");
                } 
    the problem is that he's like "ignoring" if num_rows is equal to one. so that's the big problem, because he can register with same username as the administrator and just change the website however he wants. But it was when i used like; $this->connection->query(); instead of prepare, bind params and then execute it. So am I good to go or should i change somthing?
  14. #8
  15. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Originally Posted by pumpkin
    i've updated the code
    Did you? I still see an insane nesting of useless functions which will corrupt your data.

    I repeat: You need prepared statements and nothing else. Throw away all the other stuff. For example, what on earth does HTML-escaping have to do with an SQL query? Those are two completely different languages.



    Originally Posted by pumpkin
    the problem is that he's like "ignoring" if num_rows is equal to one. so that's the big problem, because he can register with same username as the administrator and just change the website however he wants. But it was when i used like; $this->connection->query(); instead of prepare, bind params and then execute it. So am I good to go or should i change somthing?
    We cannot possibly analyze your whole CMS based on 10 lines of code.

    This particular code is secure. But that doesn't tell us anything about the rest of the application.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  16. #9
  17. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    58
    Rep Power
    1
    Random P.S. This isn't a security issue, necessarily, but instead of selecting *, you may want to actually select the fields you need... I don't know how big your table is, but if your table does end up growing, you'll be doing a lot of unnecessary fetching with every query you have. Along those lines, if there is supposed to be only one match, maybe throw in a LIMIT 1 at the end of the query?

    Comments on this post

    • Jacques1 agrees
  18. #10
  19. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by Jacques1
    Did you? I still see an insane nesting of useless functions which will corrupt your data.

    I repeat: You need prepared statements and nothing else. Throw away all the other stuff. For example, what on earth does HTML-escaping have to do with an SQL query? Those are two completely different languages.





    We cannot possibly analyze your whole CMS based on 10 lines of code.

    This particular code is secure. But that doesn't tell us anything about the rest of the application.
    i'll remove it. sorry!

    here's the whole register:
    PHP Code:
    public $rusername;
    public 
    $rpassword;
    public 
    $rrepassword;
    public 
    $remail;

    public 
    $checkEmail;
    public 
    $checkUser;

    public 
    $rsuccess;

    public 
    $userIP;


    public function 
    userRegister(){
        
        if(isset(
    $_POST['rbutton'])){

                

                
                
    $this->rusername $this->connection->real_escape_string(strip_tags($_POST['rusername']));
                
    $this->rpassword $this->connection->real_escape_string(strip_tags($_POST['rpassword']));
                
    $this->rrepassword $this->connection->real_escape_string(strip_tags($_POST['rrepeatpass']));
                
    $this->remail $this->connection->real_escape_string(strip_tags($_POST['remail']));
                
                
                switch(
    $this->rusername && $this->rpassword && $this->rrepassword && $this->remail){
                    
                    case 
    "":
                         
                        die(
    "you cannot leave form empty!");
                        
                        break;
                        
                        default:
                        continue;
                    
                }
                
                
                
                if(
    strlen($this->rusername) > 25){
                    
                    die(
    "Username too long!");
                    
                }
                if(
    strlen($this->rusername) < 3){
                    
                    die(
    "Username too short!");
                    
                }
                
                if(
    strlen($this->rpassword) < 6){
                    
                    die(
    "Password must be minimun 6 character");
                    
                }
                
                
                
                if(
    filter_var($this->remailFILTER_VALIDATE_EMAIL) === false){
                    
                    die(
    "E-mail is invalid");
                    
                }
                
                
                
                
    $this->checkEmail $this->connection->query("SELECT * FROM users WHERE user_email='" $this->remail "'");
                
                if(
    $this->checkEmail->num_rows 0){
                    
                    die(
    "E-mail already exists!");
                    
                }
                
                
    $existsUsername $this->connection->real_escape_string($this->rusername);
                
    $this->checkUser $this->connection->prepare("SELECT * FROM users_table WHERE username=?");
                
    $this->checkUser->bind_params('s' $existsUsername);
                
    $this->checkUser->execute();
                
    $results $this->checkUser->get_result();
                
                if(
    $results->num_rows 0){
                    die(
    "user exists!");
                }
                
                 if(!
    preg_match('/^[a-z\d_]{2,20}$/i'$this->rusername)) {
                    die(
    "username is invalid");
                 }
                 
                 
                 if(
    $this->rpassword == $this->rrepassword){
                    
                    
    $motto 'hej';
                    
    $this->userIP $_SERVER['REMOTE_ADDR'];
                    
                    
                    
    $this->rpassword sha1(md5($this->rpassword));
                    
                    
    $this->rsuccess $this->connection->query("INSERT INTO users_table(username, password, email, register_ip) VALUES('" $this->rusername "','" $this->rpassword "','" $this->remail "','" $this->userIP ."')");
                    
                    echo 
    'You have successfully registred!';
                    
                    
                 }
                
                
                
                
            
        }    
        
        
        


    I haven't updated the whole code yet.

    and thank you very much for helping me.
  20. #11
  21. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by aysiu
    Random P.S. This isn't a security issue, necessarily, but instead of selecting *, you may want to actually select the fields you need... I don't know how big your table is, but if your table does end up growing, you'll be doing a lot of unnecessary fetching with every query you have. Along those lines, if there is supposed to be only one match, maybe throw in a LIMIT 1 at the end of the query?
    thank you i'll fix it!
  22. #12
  23. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    All right, this code looks much less good then the snippet from before.

    For some reason, you're now stuffing the variables directly into the query strings and execute them with query(). While you do escape the variables, this is much less secure than prepared statements and can indeed lead to SQL injections if you forgot the escaping occasionally.

    You said your "friend" can register with the same name as an administrator. That points to a huge problem. How do you identify users? By the username or the email address? Either way, you must make sure those are unique by setting a UNIQUE constraint in the database table. It's not enough to check this in the application.

    This check is also subject to race conditions: Let's say user A and user B try to register with the same new name at the same time. They both get told that the name isn't taken yet, so they try to insert the row. However, the UNIQUE constraint doesn't allow duplicate names, so only one of the queries will succeed, depending on which one is faster. The other query will crash.

    The problem is that there's a delay between checking the name and inserting it. During that time, somebody else may take the name. A possible solution would be to first try to insert the row. If this works, it's all good. If it fails due to a constraint violation, you know that somebody has already taken the name (maybe a millisecond ago).

    This is a common problem, so don't blame yourself for not having considered it. 99% of all registration scripts have the same problem.

    There are some other issues:

    Using SHA-1 and MD5 for the passwords is laughably weak. A standard gamer PC can "crack" those in a matter of minutes simply by trying out all possible character combinations until they've found the password. Since you hash the passwords directly, it may even be possible to find them via Google, because all simple hashes have already been computed somewhere in the world. You need a real password hashing algorithm, namely bcrypt. This algorithm is intentionally slow and also adds a random string to the password so that it's not possible to simply precompute the hashes. If you have PHP 5.5, bcrypt is already built-in. Otherwise, you need the password_compat library.

    The switch statement in line 27 makes no sense at all. The purpose of switch is to check a variable against different values. You don't have that. I guess you want an if statement instead.

    Anyway, I have to give you props for actually caring about security. This is very rare nowadays. Your code is also pretty good given that you've only been doing this for a few month.
    Last edited by Jacques1; November 26th, 2013 at 11:45 PM.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  24. #13
  25. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,868
    Rep Power
    368
    you mention "friend" in quotes, he is not a friend is he? just an acquaintance?
  26. #14
  27. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by Jacques1
    All right, this code looks much less good then the snippet from before.

    For some reason, you're now stuffing the variables directly into the query strings and execute them with query(). While you do escape the variables, this is much less secure than prepared statements and can indeed lead to SQL injections if you forgot the escaping occasionally.

    You said your "friend" can register with the same name as an administrator. That points to a huge problem. How do you identify users? By the username or the email address? Either way, you must make sure those are unique by setting a UNIQUE constraint in the database table. It's not enough to check this in the application.

    This check is also subject to race conditions: Let's say user A and user B try to register with the same new name at the same time. They both get told that the name isn't taken yet, so they try to insert the row. However, the UNIQUE constraint doesn't allow duplicate names, so only one of the queries will succeed, depending on which one is faster. The other query will crash.

    The problem is that there's a delay between checking the name and inserting it. During that time, somebody else may take the name. A possible solution would be to first try to insert the row. If this works, it's all good. If it fails due to a constraint violation, you know that somebody has already taken the name (maybe a millisecond ago).

    This is a common problem, so don't blame yourself for not having considered it. 99% of all registration scripts have the same problem.

    There are some other issues:

    Using SHA-1 and MD5 for the passwords is laughably weak. A standard gamer PC can "crack" those in a matter of minutes simply by trying out all possible character combinations until they've found the password. Since you hash the passwords directly, it may even be possible to find them via Google, because all simple hashes have already been computed somewhere in the world. You need a real password hashing algorithm, namely bcrypt. This algorithm is intentionally slow and also adds a random string to the password so that it's not possible to simply precompute the hashes. If you have PHP 5.5, bcrypt is already built-in. Otherwise, you need the password_compat library.

    The switch statement in line 27 makes no sense at all. The purpose of switch is to check a variable against different values. You don't have that. I guess you want an if statement instead.

    Anyway, I have to give you props for actually caring about security. This is very rare nowadays. Your code is also pretty good given that you've only been doing this for a few month.
    Thank you very much for the help, you're the best!

    and the "friend" i was talking about, he WAS my friend until he learned more about hacking. he's also coding php just that he has been doing php for about 3 years now. but yea i'll probably get his ip and then block him from the site.

    i'll update the code soon.
    thank you again and everyone else who is helping me out!!
  28. #15
  29. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,868
    Rep Power
    368
    you dont want to block him via his IP.. he can use proxy server to get in..
    you want to UPGRADE your website as in tighten the security.

    Comments on this post

    • badger_fruit agrees
    • Jacques1 agrees
Page 1 of 2 12 Last
  • Jump to page:

IMN logo majestic logo threadwatch logo seochat tools logo