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 posted a question few weeks back, on making a PHP Login Script. Most of you guys told me not to use global variables and especially for something like MySQLi connection object as it may be insecure. I was also advised to switch to classes. I did so. And here is my code -

class Page {

private $unauthorized_source=0;
var $signin_stat=0;
var $current_user=0;
var $incorrect=0;
protected $conn=0;

function __construct() {
    $this->conn=new mysqli('127.0.0.1', 'root', 'short', 'untitled');
    $this->current_user=new User(0, $this->conn);
}
function signin($u,$p) {
            /*
             * the username stored in $u is first escaped using htmlspecialchars() and then is passed to this function
             * the password is first hashed using mkPass() [defined below] and then passed to this function
             */
    $q_string="SELECT uid FROM users WHERE (emailid='$u' OR uname='$u') AND AES_DECRYPT(pass,CONCAT(uname,'$p'))='$p'";
    $q=$this->conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0]; //getting uid
        $this->current_user=new User($uid, $this->conn); //creating $current_user object
        $this->signin_stat=1;
        $this->set_session($uid, $u, $p, $this->conn); //setting session
        return true;
    } else {
        $this->signout(0);
        $this->incorrect=1;
        return 0;
    }
}

function set_session($uid, $u, $p) {
    $sid=sha1($uid+time()+$_SERVER['REMOTE_ADDR']);
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$this->conn->query($q_string);
    $_SESSION['untitled']=$sid;
    $_SESSION['untitled_u']=$u;
    $_SESSION['untitled_p']=$p;
    return $q?true:false;
}

function signout($uid) {
    $sid='NULL';
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$this->conn->query($q_string);

    $this->signin_stat=0;
    $this->current_user=0;

    $_SESSION['untitled']='';
    unset($_SESSION['untitled']);
    $_SESSION['teenoblog_first']='';
    unset($_SESSION['teenoblog_first']);
    unset($_POST);
    session_destroy();
}

function mkPass($pass) {
    $p1 = sha1($pass);
    $salt = substr($p1,0,22);
    $finalPass = crypt($p1,"$2a$11$".$salt."$");
    return $finalPass;
}


function run() {
            /*
             * this function logs in the user automatically if the session is set
             */
    session_start();
    if(isset($_SESSION['untitled']) && isset($_SESSION['untitled_u']) && isset($_SESSION['untitled_p'])) {
        $sid=$_SESSION['untitled'];
        $u=$_SESSION['untitled_u'];
        $p=$_SESSION['untitled_p'];
        $q_string="SELECT uid FROM users WHERE sid='$sid' AND (uname='$u' OR emailid='$u') AND AES_DECRYPT(pass, CONCAT('$u', '$p'))='$p'";
        $q=$this->conn->query($q_string);
        if($q->num_rows==1) {
            $c=$q->fetch_row();
            $uid=$c[0];
            $this->current_user=new User($uid, $this->conn);
            $this->current_user->update_online_stat($this->conn);
            $this->signin_stat=1;
            return true;
        } else {
            signout(0);
            return false;
        }
    }
}
}

Later in other webpages I'll need to access MySQLi connection object outside the class Page that is why I want to declare a t. Is that a security issue?

share|improve this question

1 Answer 1

No. The visibility has little to do with security with that respect. (PHP Document on Visibility)

In short:

  • Public: Accessible by anything, within the object or outside.
  • Private: Accessible only by that object.
  • Protected: Accessible only by that object and objects that extend that object.

Give a method (or property) only the scope it needs. A DB class needs to be able to send and receive information, and would need public methods (but NOT properties, they should be private or protected at most). Remember, not declaring it as public/private/protected means it will be public.

Other tips for your code:

  1. Make your db information/setup its own class and use that class everywhere else.
  2. Use a PDO system and not the mysql* functions in PHP. This is the best thing you can do to work on increasing security.
share|improve this answer
1  
A DB class needs to be able to send and receive information, and would need public methods (and properties) This sentence worries me. As a rule of thumb, I don't make member variables public unless changing them at any point can't break the execution of the class, and it's very rare for that to be the case in DB classes. –  MrLore Jan 17 '13 at 19:07
1  
@MrLore I agree. As soon as posted it and started thinking about it, a DB class should only need public methods for it's properties at most. It's properties should be private or protected. Updated answer with this. –  MECU Jan 17 '13 at 19:08
    
How does declaring a variable as global affect security? –  ShuklaSannidhya Jan 18 '13 at 11:10
1  
Do some searching on global variables in php: stackoverflow.com/a/5166527/84162 I'm going to try an analogy: private is inside your body. protected in is your pocket. public is your shirt. global is a whiteboard you hold above your head with the information on it. –  MECU Jan 18 '13 at 15:51

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.