userfrosting / UserFrosting

Modern PHP user login and management framework
https://www.userfrosting.com
Other
1.63k stars 367 forks source link

Auto-logout after idle period #39

Closed HedgeShot closed 9 years ago

HedgeShot commented 10 years ago

Hey guys,

It would be great security wise if there was an auto-logout after X minutes of inactivity from the user.

On a side note, I am note so much of a security expert. To stop XSS attacks, UserFrosting sanitize the inputs with the "sanitize" function, is that enough or should I add function such as xss_clean.php that can be found here: https://gist.github.com/mbijon/1098477 ?

Thanks again for the good work! Seb

lilfade commented 10 years ago

I'm not sure the xss_clean is needed because we use prepared statments with pdo and a token to increase security, but if you wanted to be sure you could add it in

lilfade commented 10 years ago

Also do you commit to github or keep it private?

lilfade commented 10 years ago

If you want to use it change

function sanitize($str)
{
    return strtolower(strip_tags(trim(($str))));
}

To

function sanitize($str)
{
  xss_clean($str);
  $str = $data;
    return strtolower(strip_tags(trim(($str))));
}

Should work just add the function to funcs.php

HedgeShot commented 10 years ago

I actually never committed anything because all the modifications I did up to now are specific to my website (mostly design and SMS-based 2FA) and r3wt pushed himself the only feature that I was looking for.

I found a way to do the auto-logout using SESSION. If this feature is of interest to anyone, I could try to commit.

Thx for the XSS, I think we can't sanitize() all inputs because we will lose some cap letter (password is never sanitized as well) and could use xss_clean on the password or other cap sensitive input. Am I correct?

r3wt commented 10 years ago

the main focus of my protection was preventing css and frame overlay attacks, as well as cross site request forgery. The actual xss protection is limited to a single line of javascript preventing the overlay attack, but so long as we sanitize user input it should be adequate for stopping the majority of attacks, however there are still attacks that rely on passing malicious data past input filters, so xss_clean.php could be a viable solution.

One thing to note though, the defeatability of the regex displayed in in xss_clean.php is questionable, given its simplistic(though thorough) routine. it only makes one iteration. I think we all remember those regex filters that were easily defeated by things like <scr<script>ipt>alert('xss!');</s<script>cript>.

This being said, it would be an excellent starting place for an advanced input filter, but this is probably overkill for small websites unlikely to devote the attention of a dedicated hacker.

i'll leave you with a few code samples that should get the job done.

escape user input in javascript


function escape(text){
    return text.replace(/&/g,"").replace(/</g,"").replace(/>/g,"").replace(/"/g,"").replace(/'/g,"");
}

without those 4 charachters its very hard to pass working malicious code to the server. you can escape backticks for good measure if you like, but you should also sanitize input server side as well, since there is no guarantee a request is coming from your website. for all you know, some dick-wad is posting to your login script with curl. As far as sanitizing input in php, i find the following useful for multi-purpose usage in most cases(don't use on passwords though.)

escape input in php

function security($value) {
   if(is_array($value)) {
      $value = array_map('security', $value);
   } else {
      if(!get_magic_quotes_gpc()) {
         $value = htmlspecialchars($value, ENT_QUOTES, 'UTF-8');
      } else {
         $value = htmlspecialchars(stripslashes($value), ENT_QUOTES, 'UTF-8');
      }
      $value = str_replace("\\", "", $value);
   }
   return $value;
}
HedgeShot commented 10 years ago

Thx r3wt for the tip.

Actually the website will deal with bitcoin addresses so it better has to be quite secured (i saw that you are working on some bitcoin related project, correct?)

Here is the code to auto-logout after X minutes that is working on my setup:

// AUTO LOGOUT AFTER 15 MIN

// How long is the user allowed to be idle?
$idle = (15 * 60); // In Seconds

// See if the User has been idle for too long
// If he/she has, then log them out.
// Otherwise, update their status.
if(isset($_SESSION['time'])){
    // How long from his/her last visit?
    $time_elapsed = (time() - $_SESSION['time']);

    if($time_elapsed < $idle){
        // Update user's activity
        $_SESSION['time'] = time();
    }else{
        // He/She has been idle for too long 
        unset($_SESSION['time']);
        $location = "logout.php";
        header("Location: " .$location);
        die();
    }
}else{
    // This is his/her first visit.
    $_SESSION['time'] = time();
}  
alexweissman commented 10 years ago

Just to clarify, prepared SQL statements are about preventing SQL injection, not XSS. XSS is a different sort of attack, where you trick a logged in user into loading a malicious javascript file. This is usually done by the malicious user submitting their script as data (such as in forum post) which is then rendered (and run) on a page that the victim loads.

To prevent this, you have to convert any <script> tags that users submit into literal text. This is the purpose of PHP's htmlentities function (http://php.net/manual/en/function.htmlentities.php).

The class models/class_validator.php automatically does this for all POSTed and GETed data, so this should be used at the beginning of every API page to clean the data.

r3wt commented 10 years ago

@HedgeShot Yeah, i'm building a cryptocoin exchange. The validation with security should be sufficient(imo). however, you will also need to validate bitcoin addresses are base58, like this litte beauty that Gavin Andresen wrote:


function decode_base58($btcaddress)
{
    // Compute big base58 number:
    $chars="123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";
    $n = "0";
    for ($i = 0; $i < strlen($btcaddress); $i++) {
        $p1 = strpos($chars, $btcaddress[$i]);
        if ($p1 === false) return false;
        $n = bcmul($n, "58");
        $n = bcadd($n, (string)$p1);
    }
    // Peel off bytes to get checksum / hash / version:
    $checksum = "";
    for ($i = 0; $i < 4; $i++) {
        $byte = bcmod($n, "256");
        $checksum = chr((int)$byte) . $checksum;
        $n = bcdiv($n, "256");
    }
    $hash = "";
    for ($i = 0; $i < 20; $i++) {
        $byte = bcmod($n, "256");
        $hash = chr((int)$byte) . $hash;
        $n = bcdiv($n, "256");
    }
    $version = (int)$n;

    // Make sure checksum is correct:
    $check = hash('sha256', hash('sha256', chr($version).$hash, true), true);
    if (substr($check,0,4) != $checksum) return false;

    return array($version, $hash, $checksum);
}

You can use it like so:


if(decode_base58($address))
{
    //valid address
}else{
    //not valid address
}
HedgeShot commented 10 years ago

Thx @r3wt for the bitcoin validation address. I am currently doing in JS via the blockchain.info API (yeah blockchain.info sucks with all their downtime) and I check again server side in php for allowed characters (but not if the address is actually valid). I think "bcmul", "bcadd" functions don't work on my server. I implemented SMS-based authentication for updating the bitcoin address update for added security with anti-brute force measures.

r3wt commented 10 years ago

@HedgeShot Right on, nothing wrong with using blockchain if you're only doing bitcoin. SMS authentication is good, and while were on the subject is there one you'd recommend? I would like to use Authy but if i understand correctly it doesn't support sms authentication, only smartphone authy apps are supported. i could be wrong though.

r3wt commented 10 years ago

Also, here's a thought, and i learned this the hardway. For bruteforce protection, don't rely on sessions to increment until ban time. rather, store the ip of the request in the database. when an ip has 10 or more entries, ban the ip. Some customers will complain on getting locked from account, so you will have to show an alert on failed logins of how many attempts they have remaining.

HedgeShot commented 10 years ago

@r3wt I am using Nexmo, it seems quite reliable and cheap - €0.033 / sms for Switzerland, only €0.0048 for USA.

For bruteforce portection, I store an index in the DB and increment it at every trials. I lock the user out for X minutes if too many attempts. I could use IP address to allow the correct user to log in tho, thx for the suggestion! At the moment, I use this protection only to protect the bitcoin address change, I guess this could be also useful for the regular login.

alexweissman commented 9 years ago

Alright, time to close this. Autologout is discussed in #66. XSS sanitization, for the most part, will be handled on the output side by Twig, not the input side. SQL sanitization of course is handled by the PDO library.