Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm VERY new to PHP and have begun writing a basic login for a website that uses data from a Microsoft SQL server to display information on the web. Below are 3 snippets of code (two functions from the same file and one controller file that calls them.)

Connect to MSSQL to validate user (from phplib)

With this, I take the login credentials stored in a MySQL database, and use them to connect to a MSSQL database. (As seen in the "createUser" case of the controller).

function PDOConn(&$IPAddress, &$userAdmin, &$passAdmin, &$Database, $sess_client)
{
    global  $host, 
            $username, 
            $password, 
            $db_name, 
            $tbl_name_client;

    try {  

            $conn = new PDO("mysql:host=$host; dbname=$db_name", $username, $password); 
            $stmt = $conn -> prepare("SELECT    stuff
                                      FROM $tbl_name_client 
                                      WHERE client = :clientId");
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt->execute(array('clientId' => $sess_client));
            $result = $stmt->fetchAll();

                if ( count($result) == 1 ) 
                    {
                        foreach($result as $row) 
                            {
                                $IPAddress = $row['IP'];
                                $userAdmin = $row['Username'];
                                $passAdmin = $row['Password'];  
                                $Database  = $row['database'];
                            }
                    } 
                else if (count($result) > 1)//Primative error handling
                    {
                        exit( "<h1>There are multiple rows in the clients table with the same clientID, which is really strange...</h1>");
                    }
                else  
                    {
                        exit( "<h1>ID isn't in the table.  No rows returned.  Sorry bro...");
                    }
        }  

    catch(PDOException $e) 
        {  
            echo 'ERROR:   '. $e->getMessage();  
        }   

}

Check Function (from phplib)

This is the function that get called from the main login screen. function PDOCheckLogin($myusername, $mypassword) { global $host, $username, $password, $db_name, $tbl_name;

    try {  
            $conn = new PDO("mysql:host=$host; dbname=$db_name", $username, $password); 
            $stmt = $conn->prepare("SELECT * FROM $tbl_name WHERE username = :myusername");
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt->execute(array('myusername' => $myusername));
            $result = $stmt->fetchAll();
                if ( count($result) == 1 ) 
                    {
                        foreach($result as $row) 
                            {
                                $validpass = $row['password'];
                                $hashsalt  = substr($validpass, 0, 128); //get the salt from the front of the hash
                                $testhash  = hash('sha512', $hashsalt.$mypassword);
                                $testhash  = $hashsalt.$testhash;

                                if($testhash === $validpass)
                                    {
                                        $sess_empno    = $row['employeeno'];
                                        $sess_client   = $row['ClientId'];
                                        $sess_username = $row['username'];
                                        $_SESSION['sess_empno']    = $sess_empno;
                                        $_SESSION['sess_client']   = $sess_client;
                                        $_SESSION['sess_username'] = $sess_username;
                                        header("location:../index.php");
                                    }
                                else
                                    {
                                        exit("<h1>WRONG PASSWORD >:(</h1>");
                                    }
                            } 
                    }
                else if (count($result) > 1)
                    {
                        exit( "<h1>There are multiple users with that username in the database...Whoops...</h1>");
                    }
                else  
                    {
                        exit( "<h1>That's not a username...</h1>");
                    }
        }  
    catch(PDOException $e) {  
            echo 'ERROR:   '. $e->getMessage();  
        }   

}

My...Well Whatever this is...Controller I guess.

This receives posts from various forms and then calls the correct functions.

<?php
ob_start();
include_once('../phplib.php');
 session_start();
 $form = $_POST['form'];
 switch ($form) {
    case 'login':  //When the User Logs in
             $myusername=$_POST['myusername']; 
             $mypassword=$_POST['mypassword'];

                PDOCheckLogin($myusername, $mypassword);//Do this function
        break;
    case 'createUser':  //MSSQL verification for the user for user creation
            $client=$_POST['clientId']; 
            $email=$_POST['email'];

                PDOConn($IPAddress, $userAdmin, $passAdmin, $Database, $client);//Get the login credentials from the database
                    $Login = $userAdmin;
                    $Password = $passAdmin;
                $conn=mssql_connect($IPAddress,$Login,$Password); //Connect to the database
                    if (!$conn )
                    {
                          die( print_r("Unable to connect to server", true));
                    }
                mssql_select_db($Database, $conn);

                    $sql="SELECT stuff FROM table WHERE email = '$email'"; //Make sure your email is in the MSSQL database
                    $rs=mssql_query($sql);
                    $count=mssql_num_rows($rs);
                         if ($count == 0 || $count > 1)
                         {
                            echo("Email Not Found or Valid");//You liar...
                         }
                         else{
                              while ($row = mssql_fetch_array($rs))
                                  {
                                     $EmpNo = $row['employee_number'];  
                                  }
                             }

                PDONewUser($EmpNo, $email, $client, $tempSess);
        break;

    default:
        # code...
        break;
 }
ob_end_flush();

?>

I'm not thrilled with the state of my mssql_connect() code but I really don't know the alternative as the PDO drivers are extremely complicated and, from what I gather, experimental.

I salt my passwords (so I'm ahead of LinkedIn) but that's about it. I'm looking into measures to stop Brute force and XSS but I want to make sure this isn't completely broken first.

Is this secure? Is there a better way to be doing thing? OOP probably would be a smart move but I really do struggle with the concepts. I wrote my first line of HTML code in February of this year and had no real computer experience prior to that.

My Controller file currently points to 5 different cases (better than my old approach of five files point to different functions, which in turn was better than 5 files that didn't even have functions, they just ran procedural code and then redirected as necessary.) Is that bad coding?

Any suggestions would be great.

share|improve this question
    
Minor formatting issue -- hit enter after the line "This is the function that get called from the main login screen." –  Nick ODell Nov 17 '12 at 5:39
add comment

2 Answers

First off, welcome to Code Review. Answers here may be a while in coming. Usually I'm better about getting to these questions but I've been extremely busy the last week and so missed this. Hopefully you're still watching this.

Learning MySQL is a great way to get comfortable with the idea of SQL, but PDO or MySQLi are the ways to go. MySQL is soon to be deprecated. So heads up. To the best of my knowledge, PDO is not expiramental, but then again, my knowledge is limited when it comes to SQL. That's not to say that my answer won't be full of good information, only that I can't help you with specifics in this regard. I've helped people with PDO and MySQLi code on here before without needing to know how it works. I do normally suggest they seek further opinions, but that's a general disclaimer about anything seen on the internet.

Anyways, without further ado...

Globals and Security

Are bad. If you want a piece of information to be available across scripts, then either pass it through POST, GET, SESSION, or COOKIE. Which depends on the type of information being passed. For instance, anything private should only be passed via post, and is usually done via customer to server and you will have no control over it. Anything public can be passed with get, and saved with cookies. Anything session specific to a customer should be passed with sessions.

So your comment about being ahead of the game compared to LinkedIn was wrong. Your passwords are freely available on the default global environment, unsalted and unprotected. Besides, I think the specific instance with LinkedIn was not that they weren't salting their passwords, it was that they were using SHA to do it. Though I may be misremembering my facts here. I am by no means a security expect, so, if you'll pardon the pun, take my advice with a grain of salt.

Functions and OOP

Functions should be specialized. In other words they should do only one thing, and do it well. So PDOConn() should only be worried about connecting to the database. A separate function should be concerned with preparing and executing statements. This is the first step to learning OOP, a subject that baffles many. Once you are familiar with this concept, and the DRY principle, then you might be ready for OOP.

Naming conventions say that only the first letter in every word should be capitalized and only on interior words. At least when using camelcase. So your function should actually be pdoConn(). Only classes should capitalize the first letter in the first word, and even then it would only be PdoConn(). Think of it like this. A class is an object, or noun, a function is an action, or verb. You capitalize nouns, you don't capitalize verbs.

Exiting a Script

Exit is used to tell PHP to cancel a script. If you want to set some output, you should do so before calling exit, usually with echo, though some people prefer print. While the way you are doing it is technically OK, it is usually better to separate the tasks.

Storing and Retrieving Passwords

I can't imagine a reason to have a password stored on every row of a table unless that table was the user credentials table. In which case you should not be traversing the entire table to get a single login, you should instead retrieve the row specific to your user and compare their information to that single row. Besides, on the off chance that two customers share a password, you will be faced with the real possibility of a user getting the wrong account credentials.

Breaking the Loop

When you have found what you were looking for in a loop, you should break from it to prevent further iterations. This saves processing power. Now, before you say, "But I did, look at that header()!", know that you did not. In order to properly terminate a script after calling a header with the location flag, you must also call the exit() function immediately afterwards, else the script will continue to run. Additionally, when using a header, it is proper to use an absolute path instead of the relative one.

Output Buffering

Asides from you calling an include before the session_start() I see no reason for this. You are not processing the information buffered, you are simply dumping it. I'd remove this buffer and just move the include to below the session.

Another Security Concern

You are using direct user input without validating or sanitizing it. This is a concern for XSS. If you have PHP version >= 5.2 you can use PHP's filter_input() function, for example:

$email = filter_input( INPUT_POST, 'email', FILTER_SANITIZE_EMAIL );

Controllers

I'm assuming you are talking about the OOP approach MVC here. A controller, normally, only focuses on one view (case). However, I can see that using the same controller for similar views, such as adding/removing a user, is understandable and I don't really see anything wrong with it. But I would limit that as much as possible. If the controllers start getting too large or difficult to follow, then you'll know you should start separating them. I wouldn't really worry about this yet, at least not until you have everything else sorted out. If this part works, leave it for now and come back to it when the rest does.

share|improve this answer
    
I see what you're saying about a lot of this, but one thing that I can tell you didn't understand is that my MYSQL is used to store credentials for MICROSOFT SQL SERVER (Which I should probably stop referring to as MSSQL, my apologies.) I don't ever use mysql functions, but I am forced to use MSSQL functions (the PDO drivers for which I believe are experimental). One question is, for the MySQL connection credentials (as in, connecting to the servers DB), if I shouldn't use globals, what should I use instead? –  MobyD Jul 19 '12 at 20:57
    
Ah, my bad, my dyslexia kicked in, that and I'm so used to seeing MySQL. Completely missed the difference. Thought it odd that you mentioned that then used PDO as well. No, MSSQL is the proper term for it, that was my mistake. –  mseancole Jul 19 '12 at 21:15
add comment

CSRF

Interesting fact: If I can get somebody to navigate to a webpage I control, then I can use their browser to mass-create users. I think they might be able to brute-force logins, but I'm not sure how to detect a successful login.

OWASP guide to CSRF

Salting

These days, site-specific salts are pretty much pointless. If somebody attacks the hashes for your site, it will be with a GPU, not a rainbow table. On the other hand, user specific salts still very much have a point - it means that if somebody wants to attack your passwords, they have to do it one account at a time.

I can't tell if you use random per-user salts or not, because I don't see any code for producing that salt in the first place.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.