Basic system login (improvements)

[eluser]Renato Tavares[/eluser]
I built a very simple login system, because only need to prevent unregistered users from accessing certain page.

The question is that using "pure PHP", would this system with many lines of code, but with CodeIgniter I did it with only 50 lines of code. I'm worried because I thought extremely small, I believe there must be some error in my logic or any breach of security that I'm leaving out.

If possible someone check out what I did and tell me if everything is fine and no security problems. Really if you're all right, I'll be very impressed with CodeIgniter, I'm new to the tool but I'm really enjoying it.

Model

Code:

<?php  if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class Login_model extends CI_Model {

    public function __construct() {
        parent::__construct();
    }

    public function check($username, $password) {

       // $this->db->select('user_name, user_email, user_id'); // Omitir isso é um SELECT (*)
        $this->db->from('users_data');
        $this->db->where('user_email = ' . "'" . $username . "'");
        $this->db->where('user_password = ' . "'" . $password . "'");
        $this->db->limit(1);

        $query = $this->db->get();

        if ($query->num_rows() == 1) {

            return $query->result_array();
        } else {

            return FALSE;
        }
    }

}

/* End of file login_model.php */
/* Location: ./application/models/login_model.php */

controller

Code:

<?php if (!defined('BASEPATH')) exit('No direct script access allowed');

class Login extends CI_Controller {

    public function index() {
      
        $this->load->library('form_validation');

        $this->form_validation->set_rules('user_username', 'Usuário', 'trim|required|xss_clean');
        $this->form_validation->set_rules('user_password', 'Senha', 'trim|required|xss_clean|callback_check_database');

        $this->form_validation->set_error_delimiters('<div class="alert alert-error">', '</div>');

        if ($this->form_validation->run() == FALSE) {

            $this->load->view('static/header');
            $this->load->view('pages/login');
            $this->load->view('static/footer');
        } else {

            redirect('painel', 'refresh');
        }
    }

    private function check_database($password) {

           $this->load->model('login_model', '', TRUE);

        $username = $this->input->post('user_username');

        $result = $this->login_model->check($username, $password);

        if ($result) {

            foreach ($result as $value) {
                foreach ($value as $key => $v) {
                    $this->session->set_userdata($key, $v);
                }
            }
            $this->session->set_userdata('logged_in', TRUE);

            return TRUE;
        } else {

            $this->form_validation->set_message('check_database', 'Invalid username or password');
            return FALSE;
        }
    }

}

/* End of file login.php */
/* Location: ./application/controllers/pages/login.php */

Everything right with my code? What can I improve? Something that bothers me personally is that foreach inside another.

[eluser]Aken[/eluser]
1) Your WHERE calls can be written simply as $this->db->where('username', $username);

2) Don't use xss_clean on passwords.

3) Related, your passwords need to be encrypted somehow. This is a big security issue, both for user's privacy and for your website. Existing authentication libraries for CI take this into account. If you're set on writing your own, you should read up on password security in PHP. Because what you have is pretty much unacceptable. Sorry!

4) Regarding your nested foreach() loops, it does look ugly, and it's not something you should rely on. First, you're storing your user's details as session data. Another bad idea -- you're putting their username AND plain text password right in a spot that a hacker can access. Also, if your database ever changes, but you don't update your application and a column it expects no longer exists, you're in for some fun updating things.

Similarly, when you're pulling only one row from a database, use row() instead of result(). Then you will only have a single array or object returned, instead of a nested array.

There are some other small, nitpicky things, but those are what you can and should focus on.

[eluser]Renato Tavares[/eluser]
Ohh, sorry.

I use a helper to encrypt the passwords, just forgot to put it there.

Code:

<?php

function hash_data($data = NULL, $length = NULL) {

    $salt = 'J)_#^|wM[)hXmimimimimimimimim+;F%U+:d';


    if ($data == NULL) {
        $data = uniqid(hash("sha512", rand()), TRUE);
    }

    $hash = hash("sha512", $data . $salt);

    if ($length == NULL) {
        return $hash;
    } else {
        return substr($hash, 0, $length);
    }
}

I did not know it was wrong to use xss_clean, of course it would disrupt the user to choose passwords with special characters but I did not realize that.

On the loop, put yourself in the session only basic data (name, email, logged) would be no problem right?

Any suggestions for improving this loop?

[eluser]Aken[/eluser]
Your hash is okay, but using the same salt for each person isn't all that great. As I said, read around, there's tons about the subject. Here's one place to start.

I still don't think using the data from the select() portion is right. I think you should explicitly set whatever userdata keys you want -- not rely on what's returned from the query. And as I said, if you use row() instead of result_array(), you won't have to do loops at all. Read the user guide for returning DB results.

[eluser]Renato Tavares[/eluser]
Thanks Aken, for helping me.

Your tips will help is much to improve the safety and quality of my code.