6 replies to this topic

[RalphLeMouf]

Hey there. I am finalizing all of the errors on my forms. For example, on my login form where the user is prompted to enter their email and password, I have the errors working in order when the password and emails don’t match or it’s not a valid email, BUT if I were to type in an email that is NOT in the database, it only poses error password because it recognizes it as a valid email.
I would prefer not to write any new functions if possible and tie it into my existing login function.
Here is the model and controller for said module:

//MODEL


public function validate_home_login($data)
{
// TAKING THE DATA FROM THE MODEL AND CHECKING IT AGAINST THE STORED INFO IN THE DB
$query = $this->db->where($data)->get('users', '1');

if($query->row())
{
return $query->row();
$valid_email = $row->email;
}
}

//CONTROLLER


public function validate_credentials_login()
{
  $this->load->library('session');
  $this->load->helper(array('form','url'));
  $this->load->model('user_model', 'um');
  $this->load->library('encrypt');
  $this->load->library('form_validation');


  $this->form_validation->set_rules('email_login', 'Email', 'trim|required|valid_email|'); 
  $this->form_validation->set_rules('password_login', 'Password', 'trim|required');


  if ( $this->form_validation->run() === TRUE ) 
  {   
   $user = $this->um->validate_home_login(array('email' => $this->input->post('email_login')));

// if $user exists
   if ( $user ) {
// if the password is correct
    if ( $user->password == $this->encrypt->sha1( $user->salt .  $this->encrypt->sha1($this->input->post('password_login'))) && $user->email == $this->input->post('email_login') ) 
    {
     $this->session->set_userdata(array('email' => $this->input->post('email_login')));
     redirect('account/dashboard');

// if the password is incorrect
    }else{
        $this->form_validation->run() == FALSE;
  $data['password_error'] = 'The password field is invalid.';
    }

// if $user doesn't exist
   }else{
$this->form_validation->run() == FALSE;
$data['email_error'] = 'This email is invalid.';

}
  }

 $data['main_content'] = 'home/home_page';
 $this->load->view('includes/templates/home_page_template', $data);   

   }

thanks in advance

Edited by RalphLeMouf, 05 December 2012 - 01:52 PM.

[Christian F.]

You actually don't want to give any other message than "e-mail and/or password invalid", so I recommend removing the "invalid e-mail" error message. Reason for this is that if you tell an attacker that an e-mail they tried is valid, but the password is wrong, they can focus their efforts on attacking only the password. Instead of having to try to guess the right combinations of both.
The users know their e-mail, and password, and thus can easily check which of them was wrong.

I'd also like to give you a general advice on your coding style, to help make your scripts a bit more readable. That is to test for failure/error conditions, and exit early.
Unfortunately it seems a lot of people only test for success conditions, and thus ends up having to nest their code a lot more than what's strictly necessary. Something which makes the script harder to read, especially as it grows bigger, as you can't easily attach the error messages to the condition tests.

In addition to that it is not a good idea to manipulate user input in any form, this includes trimming it.
Passwords in particular are extremely bad to manipulate, as you're effectively decreasing their entropy, which makes them easier to crack. Who's to say that adding a space to the start or end of your password is a bad thing?

That said I've altered your code a bit, to give you an example of what I've outlined in this post. Also added a few comments to explain what I've done, as well as noting some things that should/needs to be done.

Spoiler

class test {
    public function validate_credentials_login () {
        // CF: If no validation needed (VERY BAD), just return success immediately.
        if ($this->form_validation->run () !== TRUE) {
            return true;
        }

        $this->load->library ('session');
        $this->load->helper (array ('form', 'url'));
        $this->load->model ('user_model', 'um');
        $this->load->library ('encrypt');
        $this->load->library ('form_validation');

        // CF: Remove "trim" from the rules, for the reasons outlined in the post.
        $this->form_validation->set_rules ('email_login', 'Email', 'trim|required|valid_email|');
        $this->form_validation->set_rules ('password_login', 'Password', 'trim|required');

        $user = $this->um->validate_home_login (array ('email' => $this->input->post ('email_login')));

        // CF: TODO: Remove this, for the reasons outlined in the post.
        // if $user doesn't exists
        if (!$user) {
            $this->error['email_error'] = 'This email is invalid.';
            return false;
        }

        // CF: Check if the password is correct
        $hash = $this->encrypt->sha1 ($user->salt . $this->encrypt->sha1 ($this->input->post ('password_login')));
        if ($user->password != $hash || $user->email != $this->input->post ('email_login')) {
            // if the password is incorrect
            $this->error['password_error'] = 'The password field is invalid.';
            return false;
        }

        // CF: Successfully logged in, add the neccesary user data to the session.
        // CF: TODO: If not done already, regenerate the session ID.
        $this->session->set_userdata (array ('email' => $this->input->post ('email_login')));
        return true;
    }
}


// CF: TODO: Move the code below this line into the calling function. It has nothing to do with validating the credentials.
if ($user->validate_login ()) {
    redirect ('account/dashboard');
    die ();
}

// CF: TODO: Add the error array from $User to $Data.
$data['main_content'] = 'home/home_page';
$this->load->view ('includes/templates/home_page_template', $data);

I've prefixed my comments with "CF:", and the todo-notices with.. Well, you can probably guess that one.

Edited by Christian F., 05 December 2012 - 07:50 PM.

[RalphLeMouf]

Great feedback! I really appreciate it. As I'm still shaky and new with this and code-igniter all together, is there anyway you could re-write what I wrote the way you think it should be? I'm not trying to get you to write my code for me, I just think it would be easier for me to understand what your trying to get across rather than me putting it together based off of your recommendations and being usure of exactly what you mean.

thanks

[Christian F.]

Try clicking on the "Spoiler show" button.

Edited by Christian F., 06 December 2012 - 02:09 PM.

[RalphLeMouf]

well yeah I did. I tried that code chunk but it's posing errors

[Christian F.]

Of course it does, that's why I left the TODOs in there. Also, I've obviously not tested it as I don't have access to the rest of your code or your DB.
You'd just have to pick at the errors, and see if you can't clear them up. Paying close attention to what hey say, and if you find them difficult to understand; Search online. Should you still have problems, then post the error messages here, along with your updated code, and we should be able to help.

[Psycho]

In addition to that it is not a good idea to manipulate user input in any form, this includes trimming it.
Passwords in particular are extremely bad to manipulate, as you're effectively decreasing their entropy, which makes them easier to crack. Who's to say that adding a space to the start or end of your password is a bad thing?

I agree with you on the basic statement of not manipulating input - except for trimming. I advocate always trimming input, unless there is a valid reason not to. And, of course, passwords are one of those exceptions. But, for "normal" inputs: username, emails, person name, address, phone, etc. leading and trailing spaces wound not make sense and the space bar is likely pressed as a mistake in those instance (the space bar is pressed 6 million times every second!). There isn't anything inherently wrong with not trimming, but that would require adding additional validation logic since you wouldn't want an input of nothing but spaces for things such as names, addresses, etc.