djcubez Posted June 10, 2008 Share Posted June 10, 2008 This is more of a "i want your opinion" thread than a "i need help" one but I couldn't find a better spot to put this. Basically, I'm using a new system for members logging into my website because the last one was buggy, too complex, and not that secure. Login.php $_SESSION[login] = "$row[id]"; $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $query = "INSERT INTO login (uid, ip, sid) VALUES ('$row[id]','$ip','$sid')"; $row[id] corresponds to the user's id from the "members" database. Include.php (on every page) if($_SESSION[login]) { $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $uid = $_SESSION[login]; $query = "SELECT * FROM login WHERE uid='$uid'"; $result = mysql_query($query); $row = mysql_fetch_array($result); if(($row[sid] != $sid) OR ($row[ip] != $ip)) { $session_destroy(); } } Basically, if the session isn't destroyed, they get access to member privileges. Opinions? Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 1) I wouldn't store the IP like that, because I know for a fact that AOL users can change IPs on every REQUEST they make to a page. =/ Also, you should use ' ' or " " around your array keys, just in case. Quote Link to comment Share on other sites More sharing options...
MadTechie Posted June 10, 2008 Share Posted June 10, 2008 Heres a Quick clean up, see comments <?php $uid = (int)$row['id']; //filter $_SESSION['login'] = $uid; //no const's session_regenerate_id(); //regenerate the Session ID $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $query = "INSERT INTO login (uid, ip, sid) VALUES ('$uid','$ip','$sid')"; ?> <?php if($_SESSION['login']) { //No Cont's session_regenerate_id(); //regenerate the Session ID $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $uid = $_SESSION['login'];//No Cont's $query = "SELECT * FROM login WHERE uid='$uid' AND sid='$sid' AND ip='$ip' "; $result = mysql_query($query); $num_rows = mysql_num_rows($result); if($num_rows == 0) { $_SESSION['login'] = 0; //crear to be sure $session_destroy(); } } ?> Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 Why are you doing $session_id and not PHP_SESSID or session_id()? Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 1) I wouldn't store the IP like that, because I know for a fact that AOL users can change IPs on every REQUEST they make to a page. =/ Also, you should use ' ' or " " around your array keys, just in case. What would you recommend than instead? Also, the code is based off a script/article written in 2003 so that's probably how I got the $session_id() use. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Feel bad bumping this but now I need something secure to replace it with. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Alright, I just read this post: http://www.phpfreaks.com/tutorial/sessions-and-cookies-adding-state-to-a-stateless-protocol Which brings all of this into a better perspective. I understand now why the regenerate session id was used in Techie's correction. Looks like PHP has changed quite a bit since I've been writing the damned code lol. By the way, I'm still looking for suggestions for a simple, secure login system. Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 1) I wouldn't store the IP like that, because I know for a fact that AOL users can change IPs on every REQUEST they make to a page. =/ Also, you should use ' ' or " " around your array keys, just in case. What would you recommend than instead? Also, the code is based off a script/article written in 2003 so that's probably how I got the $session_id() use. I would personally recommend storing the IPs in an 'ip' table and associate them with user id's so you can tell which users connected from which IPs if you ever had to, BUT I wouldn't identify them with their IP. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Aiite thanks man, I was just trying to escape using cookies because can't some computers disable them? I'll probably just stick with sessions. Quote Link to comment Share on other sites More sharing options...
discomatt Posted June 10, 2008 Share Posted June 10, 2008 I usually force cookies. They're so much harder to steal than passing ids in the query string. If the user doesn't want to set up a white list, then they can't log in. I wouldn't potentially compromise the security of other users to allow one paranoid/lazy person that blocks ALL cookies Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Yea but I'm designing this site for a company that wants any nobody to be able to sign up without problems. So if some stupid yahoo doesn't know they have cookies disabled, it's my fault that company loses that customer. Of course, that seems like a rare scenario. Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 Yea but I'm designing this site for a company that wants any nobody to be able to sign up without problems. So if some stupid yahoo doesn't know they have cookies disabled, it's my fault that company loses that customer. Of course, that seems like a rare scenario. Everyone has cookies on by default pretty much. You'd have to intentionally turn them off. I'd personally put something under the form that says Cookies must be enabled to use this site. Read more... And have link to tiny info page on why you need them. And put a lock (like the SSL icon) next to the Cookies must be enabled... thing. They'll be like "OH I NEED COOKIES". Quote Link to comment Share on other sites More sharing options...
discomatt Posted June 10, 2008 Share Posted June 10, 2008 Most large and popular sites require cookies... especially when security is an issue. The yahoo that is too stupid to sign up is the same one that'll post a session-id laced query string to an attacker... compromising their account (which you'll get blamed for ) and potentially other nasty things. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Most large and popular sites require cookies... especially when security is an issue. The yahoo that is too stupid to sign up is the same one that'll post a session-id laced query string to an attacker... compromising their account (which you'll get blamed for ) and potentially other nasty things. Good points, guess I'll go cookies. Plus, I don't think it'll matter all to much in the future because were going to be purchasing SSL. Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 Good points, guess I'll go cookies. Plus, I don't think it'll matter all to much in the future because were going to be purchasing SSL. 1) SSL doesn't protect against SQL injections and the like, it just prevents packet sniffing. 2) Use sessions, not cookies. Although sessions use one cookie (to store the SESSID), they are more secure, faster, and easier to work with. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 Good points, guess I'll go cookies. Plus, I don't think it'll matter all to much in the future because were going to be purchasing SSL. 1) SSL doesn't protect against SQL injections and the like, it just prevents packet sniffing. 2) Use sessions, not cookies. Although sessions use one cookie (to store the SESSID), they are more secure, faster, and easier to work with. No I meant I'm going to use cookies in combination with sessions. Quote Link to comment Share on other sites More sharing options...
DarkWater Posted June 10, 2008 Share Posted June 10, 2008 What else are you using cookies for? Quote Link to comment Share on other sites More sharing options...
discomatt Posted June 10, 2008 Share Posted June 10, 2008 I think he means forcing sessions to use cookies... or possibly using cookies to 'remember' a user. Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 10, 2008 Author Share Posted June 10, 2008 I think he means forcing sessions to use cookies... or possibly using cookies to 'remember' a user. Yea like a remember me checkbox Quote Link to comment Share on other sites More sharing options...
djcubez Posted June 11, 2008 Author Share Posted June 11, 2008 Heres a Quick clean up, see comments <?php $uid = (int)$row['id']; //filter $_SESSION['login'] = $uid; //no const's session_regenerate_id(); //regenerate the Session ID $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $query = "INSERT INTO login (uid, ip, sid) VALUES ('$uid','$ip','$sid')"; ?> <?php if($_SESSION['login']) { //No Cont's session_regenerate_id(); //regenerate the Session ID $sid = $session_id(); $ip = $_SERVER['REMOTE_ADDR']; $uid = $_SESSION['login'];//No Cont's $query = "SELECT * FROM login WHERE uid='$uid' AND sid='$sid' AND ip='$ip' "; $result = mysql_query($query); $num_rows = mysql_num_rows($result); if($num_rows == 0) { $_SESSION['login'] = 0; //crear to be sure $session_destroy(); } } ?> Tried using this code and got: Warning: session_regenerate_id() [function.session-regenerate-id]: Cannot regenerate session id - headers already sent in /home/advanced/public_html/test/login.php on line 25 Not to mention that the user doesn't even stay logged in. Quote Link to comment Share on other sites More sharing options...
MadTechie Posted June 13, 2008 Share Posted June 13, 2008 please read the HEADER ERRORS post also $session_id(); should be session_id(); Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.