Is this Secure?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Well, according to your current code, there is no way for cookies to be set. Therefore, you'd have to "login" for every page you wanted to view. Somethings missing from the picture.

And yeah, sessions are a good idea.
foobar
Forum Regular
Posts: 613
Joined: Wed Sep 28, 2005 10:08 am

Post by foobar »

Dark[NSF] wrote:
foobar wrote:
Dark[NSF] wrote:
for SQL it doesn't, but it will prevent some javascript on random html tags, or some XSS.
Which has nothing to do with your question. Ambush commander already gave you the answer.

I was refering to security as a whole, strip_tags() is probably a good place to start. i'm not saying he's wrong.
Oh allright. It just seemed like you were pulling stuff from the blue. Confused me a bit.
Dark[NSF]
Forum Newbie
Posts: 18
Joined: Thu Oct 06, 2005 9:25 am
Location: Palm Bay, FL
Contact:

Post by Dark[NSF] »

Ambush Commander wrote:Well, according to your current code, there is no way for cookies to be set. Therefore, you'd have to "login" for every page you wanted to view. Somethings missing from the picture.

And yeah, sessions are a good idea.

alright, here you go. why should i use sessions? just for security purposes?

Code: Select all

// Login to Account
function acc_login()
{
  if ( ( isset( $_POST[ 'login' ] ) ) && ( isset( $_POST[ 'password' ] ) ) )
  {
    if ( bverify_login(0) )
    {
      setcookie( "tj_email", $_POST[ 'login' ], time(  ) + 36000 );
      setcookie( "tj_password", $_POST[ 'password' ], time(  ) + 36000 );
      header( "Location: account.php?message=login_correct" );
    }
    else
      header( "Location: account.php?message=login_incorrect" );
  }
  else
    header( "Location: account.php?message=login_blankfield" );
}

// log me out
function acc_logout(  )
{
  setcookie( "tj_email", null, time(  ) - 36000 );
  setcookie( "tj_password", null, time(  ) - 36000 );
  header( "Location: index.php?message=logout" );
}
Sequalit
Forum Commoner
Posts: 75
Joined: Wed Oct 12, 2005 9:57 pm
Location: Texas

Post by Sequalit »

1. Security.
2. Users may have cookies disabled.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Well, point two is mostly null considering how sessions usually work.

I suggest you first read up on what sessions actually are: Session Handling Functions and $_SESSION

Here's how I go about authentication. It's not usable in its current state, but it should help you get some ideas.

I view authentication as a four question process:

1. Is the user logging out?
2. Is the user logging in?
3. Does the user have a valid session?
4. Does the user have a remember me token?

Each of these questions is associated with a particular action that determines whether or not it's valid, and then what to do in response.

Code: Select all

<?php

//directly handles input... the input should be passed to it via the input
//controller, and this be part of the ApplicationController that figures out
//whether or not a special command has to be issued.
class Filter_Authentication extends Filter
{
    
    var $_plugin;
    var $_mapper_user;
    
    function Filter_Authentication() {}
    
    function execute(&$registry) {
        parent::execute(&$registry);
        $this->_plugin =& $this->_registry->getPlugin();
        $this->_mapper_user =& $this->_registry->getMapperUser();
        if($this->assertLogout()   && $this->executeLogout())   return;
        if($this->assertLogin()    && $this->executeLogin())    return;
        if($this->assertSession()  && $this->executeSession())  return;
        if($this->assertRemember() && $this->executeRemember()) return;
        $this->_registry->setSession(new Session_Null());
    }
    
    //these look like application states
    function assertLogout() {
        return isset($_POST['logout']);
    }
    function assertLogin() {
        return isset($_POST['user']) && isset($_POST['pass']);
    }
    function assertSession() {
        return isset($_COOKIE[$this->_plugin->getCookieSession()]);
    }
    function assertRemember() {
        return isset($_COOKIE[$this->_plugin->getCookieToken()]);
    }
    
    //these look like commands
    function executeLogout() {
        session_start();
        header('Cache-Control: private, pre-check=0, post-check=0, max-age=0');
        if ($this->assertRemember()) {
            $cookiebits = $this->parseTokenCookie();
            if ($cookiebits) {
                $MapperToken = $this->_registry->getMapperToken();
                $token =& $MapperToken->find($cookiebits[1]);
                if ($token && ($token->getValue() === $cookiebits[2])) {
                    $token->delete();
                }
            }
            setcookie($this->_plugin->getCookieToken(),'',0);
        }
        $session =& Session::newFromSession();
        if ($session) {
            $session->delete();
            $this->_registry->setSession(new Session_Null());
        }
        Template::refresh('logout');
        return true;
    }
    function executeLogin() {
        if (empty($_POST['user']) || empty($_POST['pass'])) return false;
        if (!arep($_POST['user'])) return false;
        $user =& $this->_mapper_user->findByName($_POST['user']);
        if (!$user) return false;
        if (!$user->authenticate($_POST['pass'])) return false;
        session_start();
        header('Cache-Control: private, pre-check=0, post-check=0, max-age=0');
        $session =& Session::newFromUser(&$user);
        $this->_registry->setSession(&$session);
        $session->update();
        Template::refresh('login', &$user);
        return true;
    }
    function executeSession() {
        session_start();
        header('Cache-Control: private, pre-check=0, post-check=0, max-age=0');
        $session =& Session::newFromSession();
        if (!$session) return false;
        if ($session->isExpired()) {
            $session->delete();
            return false;
        }
        $session->setLastaccess();
        $session->update();
        $this->_registry->setSession(&$session);
        return true;
    }
    function executeRemember() {
        $cookiebits = $this->parseTokenCookie();
        if (!$cookiebits) return false;
        $MapperToken = $this->_registry->getMapperToken();
        $token =& $MapperToken->find($cookiebits[1]);
        if (!$token) return false;
        if ($token->getValue() !== $cookiebits[2]) return false;
        session_start();
        header('Cache-Control: private, pre-check=0, post-check=0, max-age=0');
        $session =& Session::newFromUser($token->getUser());
        $this->_registry->setSession(&$session);
        $session->update();
        return true;
    }
    
    //this looks like domain logic
    function parseTokenCookie() {
        $cookie = $_COOKIE[$this->_plugin->getCookieToken()];
        $cookiebits = explode('-', $cookie);
        if (sizeof($cookiebits) != 3) return false;
        if ($cookiebits[0] != '1') return false; //version number
        if ($cookiebits[1] == '' || $cookiebits[2] == '') return false;
        if (!is_numeric($cookiebits[1])) return false;
        return $cookiebits;
    }
    
}

?>
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

When dealing with the Database, no strip_tags() has nothing to do with it. That is the job of mysql_real_escape_string()

strip_tags() is for output that is being sent to the user agent. IMO it's better practice to use htmlemtities().

Another stage you might want to consider for authentication or validation is the email address the user enters. A simple regex function to determine if the address is in valid email address format :)
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

strip_tags is essentially useless in security except in very limited and predictable scenarios. Not in everything, but largely a 10 year old can input a string which neatly avoids stripping from that function. As a general rule, if something is not supposed to contain html, use htmlentities() to escape it prior to output. Remember many XSS attacks utilise malformed html, take advantage of buggy html rendering, or try and trick filtering systems with various encoding tactics.

For the database use the native database escaping functions (if they exist); mysql_real_escape_string, pg_escape_string, pg_escape_bytea, etc. One point to remember for these, is that you should read up on how magic_quotes (a PHP input filter that escapes input - not all that popular since it interferes with later escaping) effects these.

You should use sessions to store information (not absolutely required in a cookie) on the server which is needed from page request to page request.
User avatar
shiflett
Forum Contributor
Posts: 124
Joined: Sun Feb 06, 2005 11:22 am

Re: Is this Secure?

Post by shiflett »

Dark[NSF] wrote:

Code: Select all

if (isset($_COOKIE['tj_email']) && isset($_COOKIE['tj_password']))
{
    $input_email = $_COOKIE['tj_email'];
    $input_password = $_COOKIE['tj_password'];
}
Authenticating users on every single request is a bad idea for a number of reasons. The top two that come to mind are:

1. You're exposing the authentication credentials in every request, likely even those for embedded resources.
2. You're making replay attacks particularly easy.

Encrypting these cookies does not eliminate both of these concerns. The approach itself is flawed, and it's always best to address the root cause of a problem rather than a symptom.
Dark[NSF] wrote:

Code: Select all

header("Location: index.php?message=no_permissions");
The Location header requires an absolute URL. Most browsers are friendly and try to deduce what you mean, but IE has been known to have trouble properly handling cookies and malformed headers.

Hope that helps.
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Post by josh »

shiftlett, your points about authentication are completely valid, but I would like to elaborate by saying by not checking some kind of credential on every page request you open up your users in a way.

Let's look at this scenario

User A log into his account and sets his password to "password"
User B asks User A for his password (and for whatever reason he gives it to him)
User A realizes what he has done and changes his password

but too late, User B is already logged in

Now of course you can prevent User B from completely taking over the account (by prompting him for his "old password" on the change password page), but now User B is free to trash the account in other aspects.

My proposed solution to this concern would be a randomized token hashed with the user's [hashed] password. Store the hashed password and whatever random string was used to generate the token in the session. You regenerate the token on each page request if you like. Now when the user changes his password the token is no longer valid.

I would personally never sacrifice an extra layer of security because of one call to the database, and I'm not clear on how this would be any more prone to a replay attack? Some kind of credential has to be sent on each page request regardless of the method being used (unless for some reason you are trusting the user's IP). At least this way a replay must be done before the user makes a new page view or logs out. With a standard session a replay can be done anytime before the session expires, right?

I'm not preaching here, I'm just trying to learn why this solution wouldn't work. Or would it?
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Well, if you just store all the (totally randomly generated) tokens in a db and then invalidate them all when a user changes their password...
Post Reply