Okay, spent a few hours trying to put this into class/function form. If somebody could look this over and advise me if this is a decent way to do it that would be great. login.php <?php include_once('../../includes/db.php'); include "class.php"; doLogin(); ?> PHP: class.php <?php include "functions.php"; class Member { //Properties public $id = 0; public $username = 'anonymous'; public $email; public $password; public $loggedin = 0; } ?> PHP: Struggling with this next bit, I can't get it to output the username in the checkLoggedin function. It outputs $myMember->username in the checkUser section though, any idea why it won't output? functions.php <?php function checkUser(){ //Connect to db include "../../includes/db.php"; //check username password $sth = $dbconn->prepare("SELECT username,active,password,salt FROM users WHERE username = :username"); $params = array("username" => $_POST[username]); $sth->execute($params); if ($row = $sth->fetch()) { //There is a user, check if they are active if ($row[active]=1){ //User is active, get salt key from db $salt = str_replace(" ", "",$row[salt]); //attempt to match submitted password with db password if ($row[password] == hash(SHA256, $_POST['password'].$salt)){ //Instantiate member class $myMember = new Member(); $myMember->loggedin = 1; //echo $myMember->loggedin; $myMember->username = $row[username]; //echo $myMember->username; checkLoggedin(); }else { echo "No match!"; } } } }//Check user function checkLoggedin(){ if ($myMember->loggedin = 1) echo 'Logged in as'; echo $myMember->username; $visibility = "hidden"; } function blankVal(){ echo 'Error! You must fill in all fields.<br />'; showForm(); } function showForm(){ echo ' <form method="post" action="#" name="submit"> <label for="username">Username</label><input name="username" type="text"><br /> <label for="password">Password</label><input name="password" type="password"><br /> <input type="submit" name="submit"> </form> '; } function doLogin(){ //Check if form submitted if(isset($_POST['submit'])){ //Check if blank value if(trim($_POST['username'])==""||(trim($_POST['password'])=="")){ //Show input error blankVal(); }else if (trim($_POST['password'])==""){ //Show input error blankVal(); }else{ //Run the function checkUser(); } } else { showForm(); } } ?> PHP:
By default, when you are inside a function, you do not have access to the outer variables. The problem you are dealing with now is called the Variable Scope. This might do the trick: function checkLoggedin(){ global $myMember; # Access Global Variable $myMember PHP:
If i was on your place guys i wouldn't use that code above. It's very vonerauble to SQL injection attacs and it's a classical example of how not to make code SQL statements.
Don't think you are quite grasping objects yet -- and putting things in the global space -- like that $myMember var -- kinda defeats the point. I just put together a 'stripped down' version of how I'd go about it -- uses PHP sessions when my own system does it's own database driven sessions instead... I tossed the files online here because it's a bit large to be putting in a post. http://www.cutcodedown.com/for_others/scottlpool2003/sessions/ Lemme break down the files for you in a order that makes sense -- starting with: http://www.cutcodedown.com/for_others/scottlpool2003/sessions/library.phps This one is good since it might help you grasp objects a bit better. First thing in it is my bomb function -- a simple die that outputs some nice markup; nothing to write home about. Next up is my messages singleton. I like to have a unified message log and it's a good opportunity to show you not just an object, but a singleton since IMHO it's a great way to handle the user... this simple one gets you ready for the bigger one. You might be asking, what is a singleton? A Singleton is a object that only has one 'instance' -- if you try to make new copies with "new" thus: $messages = new messages(); It will bomb. Instead you use the getInstance static method which the first time you call it will create/initialize the object, any subsequent times simply returns a handle to it. I declare it as 'final' so you can't modify or access it by trying to 'extend' it, and both the construct and clone methods are locked down to prevent copying. "Static" properties on objects are a bit like globals -- they always exist and are unique - but they have their scope restricted to the object they are in. Apart from getInstance there are only two other methods -- one simply adds our message to the log, the other outputs markup. Because the message list is private only these methods can access it; This is called having a 'setter' (add) and a 'getter' (show) -- why do this? It means if a cracker gets a code elevation they can't delete values from the log -- you can only add to it or show it, not change existing entries. using this singleton is pretty simple, you first get a handler to it: $log = messages::getInstance(); using :: indicates it's a 'static' method and/or property. $log->add('Testing the log'); and outputting the entire log. $log->show(); I suggest that you NOT rely on a global handle to singletons or passing their handles around and instead get them in scope when you need them. This prevents code elevations from re-assigning that variable to ther own fake BS. Next lets look at: http://www.cutcodedown.com/for_others/scottlpool2003/sessions/dbSettings.phps Which is where I store the database un/pw/dsn. As I said in my recent article over on TechTalkin I've been going a bit further than most on securing my SQL connection info. I use a function to return the values, and a second function to check against a define, and run a debug_backtrace so only the 'authorized' file AND function can call it. In this case it will 'die' unless it's called by a function named 'databaseStart' in 'demo.php'. Since usually my systems have a single index.php that handles ALL requests for a page, that's the file I'd restrict it to. To further lock this down, I'd be sure to disable the ini_ functions and make sure you have open_basedir set for subdirectories locking out read/write access to the location of dbSettings.php Moving on: http://www.cutcodedown.com/for_others/scottlpool2003/sessions/user.phps You'll notice a define up top -- this should be a static list of all data fields you want to pull from user; as such it should NEVER contain the password or any other validation/sensitive information. Good practice is to make all password handling mono-directional -- You set it in SQL, you check it in SQL, NEVER sending it back to PHP where someone could access it. (Again, try as hard as possible to make code elevations fall flat on their face) The user object is much more complex -- I made it a single object to handle logins (so you can login from/to ANY url on the system if using RewriteRules), sessions and fallback to guest. "user" is also a singleton that's 'setters' pull from the database, sessions or login only, and uses a getter to pull values preventing elevated code cracks from changing the user info. Again, keeping the private... private! The first property is used to store the user information, the second one is our instance for making this a singleton. As with all singletons we disable the constructor and clone, and have a instance handler method. The 'connect' method is there to hook up to the database and start our sessions, and calls the update() function. (which you can re-call at any time -- handy if you change the settings in the database from some other module) 'Update' is where the magic happens. I grab a handle to the log, then check if they are trying to log out. Logging out we just unset the sessions then allow the nested IF's to drop-through to the guest code at the bottom. If they're not logging out I check if they're trying to log in. My form has a hidden input on it with name="fromForm" and value="login" -- which makes it pretty simple to say YES, we came from the login form. The query to pull the login is pretty simple, and if your database makes sense (like having username a unique) it should only return one row, so a simple "if ($row = $statement->fetch())" is all we need to say yes, they logged in. Notice I'm using SHA512, a much better choice than MD5 -- just be sure your password field is VARCHAR(128) so it fits. If we do indeed get some user data, we plug it into $this->data so our 'getter' can pull the values, mimicking 'read only' behavior. It's then also put into the session with a couple extra values to help reduce the odds of session-jacking. While sure, user-agent can be spoofed and they might be at the same IP address, it's a bit like door locks on a car; it keeps the honest people out... then we just add to the log that the login was successful... and return true. Technically I don't use that return value, but I like to be consistent with how other systems work. I'm returning 'true' for logged in, 'false' for guest. If we were unable to fetch a row, log that the username/password check failed and drop through to guest. If they aren't logging out and aren't logging in, I test to see if there is already a working session. IF the extra info like user-agent and IP addy matches, we pull the user info from the database. If they don't match then it is most likely someone is trying to do a session hijack -- so we bomb them out. Likewise, if we're unable to pull a matching user row we clear the session as invalid, and drop through to guest so they can log in again. ... and finally at the bottom of update() we just plug in the guest values returning false. The last method of 'user' is 'value' -- which you just pass an array index to and it returns false if it's not in $this->data or the value if present. The final file: http://www.cutcodedown.com/for_others/scottlpool2003/sessions/demo.phps Just shows how to use it. We require_once our library files, then use that databaseStart function we check for in dbSettings to pull up the settings in local scope, create the PDO object in local scope, create our user object's instance, and connect that object to the database. We then run that function. Bottom line -- the userinfo isn't writable in global space, the database connection isn't usable in global space, making us FAR more secure than how most people go about it. The testing code below shows that we can access them anywhere, even globally, but do so safely. I pull up the messages and show them... then check if the user is guest (id<0) -- if guest show the login form, if not just show welcome text. It's a far more secure approach, again more so if you gut out the ini_set functions and set up open_basedir in your php.ini or .htaccess file. I like to disable a LOT of functions in php.ini as being stuff PHP has IMHO no real business doing... disable_functions = 'ini_alter,ini_restore,ini_set,phpinfo,chgrp,chmod,chown,fputs,fwrite, lchgrp,tmpfile,touch,link,symlink,exec,passthru,proc_open,shell_exec, system' Code (markup): Some people would disable file_get_contents and fopen -- but I find those too useful. Setting open_basedir to restrict where things can be read from and written can be just as effective once the unauthorized execution blocks on things like the database passwords are put in place... particularly if PHP does NOT have root-like permissions to the directory. (... which is why IMHO suPHP is like shooting your dog because the neighbor's cat has fleas) Hope that clarifies -- though I know it's VERY complex; security is never simple... or bulletproof. The CMS I'm writing (which I started over last week -- hell, I've been starting over every three months for two years now) is pretty much built around objects and singletons in this manner... though it's using it's own database driven session handler, extends the PDO object to use named queries pulled from json and blocks normal query/execute/exec calls, uses a rewriterule to route all non-static requests through a single index.php, etc, etc... I'm actually thinking I may end up writing a series of articles outlining what I'm doing as I go -- it seems to help keep me motivated to work on it. Case in point, just going through this dumbed down version resulted in code improvements.
What a fantastic response, thanks. Still reading over it and will come back in a few hours when I've played about with it.
Do you care to elaborate where that is vulnerable to injection attacks? If you've bothered to read through the previous posts, you will see that people are helping to improve a login system. To help others, why not highlight where you believe it to be vulnerable so people don't make mistakes? I was also under the impression that using PDO statements and binding the properties prevents injection attempts.
As Scott just said -- Prepared queries should be immune to SQL injections -- it's a hefty chunk of why there's these giant red "WARNING" boxes with a giant exclamation point in a 'sign' type triangle at the top of every mysql_ function page telling you to use mysqli or PDO instead. Your claim of SQL injections is 100% bullcookies. You may want to read up on prepared queries and how/why they should be used instead of trying to slap together a query string any old way like the steaming pile known as the PHP mysql_ functions do. http://www.php.net/manual/en/pdo.prepare.php http://www.php.net/manual/en/pdostatement.execute.php http://www.php.net/manual/en/pdostatement.bindparam.php http://www.php.net/manual/en/pdostatement.bindvalue.php The "execute" and/or "bind" functions auto-sanitize, so it's actually safe to dump $_POST into execute's array or bindparam/bindvalue. $statement = $db->prepare('SELECT * from users WHERE username = :username'); $statement->execute(array('username' => $_POST['username'])); Code (markup): COMPLETELY SAFE against injection... because you're not adding strings together like the halfwit mysql_query function, you're using functions that sanitize properly to plug in the values.
Just set the script up and works a treat I'll spend a good part of today playing with it as its on the more complex side of anything I've worked with before. Just a quick one though, how is the script deciding how the hacking attempt is made? I copy & pasted demo.php into test.php and it detects a hacking attempt. What declares a legitimate page? I found this class in dbSettings.php function dbSecurityCheck() { if (!defined('dbSettingsSent')) { define('dbSettingsSent',1); $d = debug_backtrace(); if ( isset($d[2]) && ($d[2]['function'] == 'databaseStart') && (str_replace( '/dbSettings.php', '/demo.php', str_replace('\\','/',$d[0]['file']) ) == str_replace('\\','/',$d[2]['file'])) ) return true; } die ('Hacking attempt detected for dbSettings.php!'); } PHP: And thought if I added the test.php below demo.php it would allow it but it still detects it as a hack. Thanks again. EDIT That was a silly question. Simple session_start() worked. Now in my previous script, wherever I needed user info e.g. on the account page, I'd run a brand new query checking the session username to match the user in the DB. This is a total waste as the user info is already in the user class if I'm reading it correctly? How can I quickly pass the users details such as username, user_id from the class to say, demo.php or account.php?
So long as you bind parameters to a prepared statement they are escaped. If you don't believe me, your free to see for yourself. Because Experience beats assumptions. I only solved his problem. Classes are a whole other beast alltogether.
Not sure what adding a session_start would have to do with anything, unless you are destroying the security model. The entire thing is designed to only allow ONE file (demo.php) to be able to call it. You want to call it from something else you need to REPLACE demo.php in the check -- this is because the code is designed for a system where ALL requests are routed through a SINGLE index.php If you are trying to build pages that use multiple different .php files the user can call directly, my code is NOT meant for that... ever. I generally consider such an approach to site building to be sloppy, redundant and quite often insecure as well.