SitePoint Sponsor

User Tag List

Results 1 to 23 of 23
  1. #1
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Improving Login Form security

    Hello,

    I've recently built around 6 or so web sites which have all used the same kind of Login form with PHP.

    However, I'm wondering how 'secure' it really is, and wondered if any kind SitePointers would be able to 'audit' the code.

    I understand there's a lot of code to look over but I would really appreciate any help at all.

    The scripts allow a user to register, log in and then log out.

    SignUp.php
    PHP Code:
    <?php

    include ('includes/functions.php');
    include (
    'includes/config.inc.php');
    include(
    'includes/header.php');

    $ip getenv("REMOTE_ADDR");
    $hr getenv("HTTP_REFERER");
    $hst gethostbyaddr$_SERVER['REMOTE_ADDR'] );
    $ua $_SERVER['HTTP_USER_AGENT'];


    // If Form Submitted
    if (isset($_POST['saveForm'])) {
        
        if (isset(
    $_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
            
    // valid token
        
    } else {
            
    header("Location: /signup/signout.php");
        }
        

        
    $token md5(uniqid(rand(), TRUE));
        
    $_SESSION['token'] = $token;
        
    $_SESSION['token_time'] = time();

         require_once(
    'connect.php');
         
         
    // Start to set & clean-up variables
         
           // Check for a name.
         
    if (eregi ('^[[:alpha:]\.\' \-]{2,30}$'stripslashes(trim($_POST['name'])))) {
            
    //$name = escape_data($_POST['name']);
            
    $name substr($name,0,30);
         } else {
            
    $name FALSE;
            
    $errmessage TRUE;
         }
        

         
    // validate age
         
    $age getIntValue($_POST['age']);
         

            
    // validate & confirm email address       
         
    $email $_POST['email'];
         if(
    eregi("^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$"$email)) {
          
    $email stripslashes(strip_tags(trim(strtolower($_POST['email']))));
          
    $email substr($email,0,120);
         } else {
          
    $email FALSE;
          
    $errmessage TRUE;
            }

         
    $email2 $_POST['email2'];
         if(
    eregi("^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$"$email2)) {
          
    $email2 stripslashes(strip_tags(trim(strtolower($_POST['email2']))));
          
    $email2 substr($email2,0,120);
         } else {
          
    $email2 FALSE;
          
    $errmessage TRUE;
         }     

         if (
    $email == $email2) {
            
    $e TRUE;
         } else {
              
    $e FALSE;
              
    $errmessage TRUE;
         }
         

         
    // Terms
         
    $terms = (isset($_POST['terms'])) ? TRUE FALSE;

        
    // time stamp - watch our for multiple submissions
        
    if (strlen($_POST['stamp']) == 32) {
            
    $s escape_data($_POST['stamp']);
        } else {
            
    $s FALSE;
            
    $errmessage TRUE
        }
        

        
    // Check if all fields have been completed

       
    if ($e) {

             if(!isset(
    $name,$age,$e,$terms,$s) || empty($name) || empty($email) || empty($email2) || empty($age) || empty($terms) || empty($s)) {

                 
    $feedbackstyle "error"
                 
    $feedback "Please complete ALL the form fields.";

                 } else {

                
    // Make sure the email address is available.
                
    $email mysql_real_escape_string($email);
                
    $query "SELECT id FROM pusers WHERE email='$email'";
                
    $result mysql_query($query);

                if (
    mysql_num_rows($result) == 0) { // Available.          

                
    $pass createRandomPassword();
                
    $salt randomString();
               
                
    // prepend the salt to the password
                
    $p $salt sha1($salt.$pass);

                
    $ipaddress $_SERVER['REMOTE_ADDR'];
                
    $name mysql_real_escape_string($name);
                
    $age mysql_real_escape_string($age);
                
    $s mysql_real_escape_string($s);

                  
    $result mysql_query("INSERT INTO pusers(name,age,email,password,date_added,ip,timestamp) VALUES('$name','$age','$email','$p',NOW(),'$ip','$s') LIMIT 1");
                
                if (
    mysql_affected_rows() == 1) { // If it ran OK.

                          // Send the email.
                          
    $body "Thank you for signing up for an account with Profiler.";
                          
    $body .= "\n\nYour temporary password is ".$pass;
                          
    mail($_POST['email'], 'Profiler - Account Confirmation'$body'From: no-reply@meownsite.co.uk');

                        
    $feedbackstyle "success";
                        
    $feedback "Great success! Grab your Password in your Inbox and Log In.";

                    }

                   }
                   
              }

          }

    }

    ?>

    <form id="form1" name="form1" class="wufoo leftLabel" autocomplete="off" enctype="multipart/form-data" method="post" action="<?php echo(''.htmlentities($_SERVER["PHP_SELF"]).''); ?>">
    <input type="hidden" name="token" value="<?php echo $token?>" />

    <div class="info">

        <h2>Sign up</h2>
        <div>Complete the form below to sign-up at our web site.</div>

    </div>

    <?php

    if (isset($feedback)) {
         echo 
    '<div class="' $feedbackstyle '">';
         echo 
    $feedback;
         echo 
    '</div>';
    }

    ?>

    <ul>

    <li id="foli1">

        <label class="desc" id="title1" for="name">Name<span id="req_1" class="req">*</span></label>

        <div>
            <input id="name" name="name" type="text" class="field text medium" value="<?php echo (isset($_POST['name'])) ? htmlspecialchars($_POST['name']) : ''?>" maxlength="255" tabindex="1" <?php if ((isset($_POST['saveForm'])) && ($name == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
            <label for="name">Must be between <var id="rangeMinMsg1">3</var> and <var id="rangeMaxMsg1">50</var> characters.&nbsp;&nbsp;&nbsp; <em class="currently">Currently Used: <var id="rangeUsedMsg1">0</var> characters.</em></label>
    </div>

        </li>

    <li id="foli4">

        <label class="desc" id="title4" for="age">Age<span id="req_4" class="req">*</span></label>

        <div>
            <input id="age" name="age" type="text" class="field text small"  value="<?php echo (isset($_POST['age'])) ? htmlspecialchars($_POST['age']) : ''?>" maxlength="255" tabindex="3" <?php if ((isset($_POST['saveForm'])) && ($age == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
            <label for="age">Enter a number between <var id="rangeMinMsg4">15</var> and <var id="rangeMaxMsg4">130</var>.</label>
        </div>

    </li>

    <li id="foli7">

        <label class="desc" id="title7" for="email">Email<span id="req_7" class="req">*</span></label>

        <div>
            <input id="email" name="email" type="text" class="field text medium" value="<?php echo (isset($_POST['email'])) ? htmlspecialchars($_POST['email']) : ''?>" maxlength="255" tabindex="4" <?php if ((isset($_POST['saveForm'])) && ($email == FALSE)) { echo 'style="border:2px solid red"'; } ?> /> 
        </div>

    </li>


    <li id="foli8">

        <label class="desc" id="title8" for="email2">Confirm Email<span id="req_8" class="req">*</span></label>

        <div>
            <input id="email2" name="email2" type="text" class="field text medium" value="<?php echo (isset($_POST['email2'])) ? htmlspecialchars($_POST['email2']) : ''?>" maxlength="255" tabindex="5" <?php if ((isset($_POST['saveForm'])) && (($email2 == FALSE) OR ($e == FALSE))) { echo 'style="border:2px solid red"'; } ?> /> 
        </div>

    </li>


    <li id="foli314">

        <label class="desc" id="title314" for="terms">Terms & Conditions</label>

        <div class="col">
            <span>
                <input id="terms" name="terms" type="checkbox" class="field checkbox" value="yes" tabindex="8" <?php if(isset($_POST['terms']) && $_POST['terms'] == 'yes') { echo 'checked="checked"'; } ?>     />
                <label class="choice" for="terms">I agree  <?php if ((isset($_POST['saveForm'])) && ($terms == FALSE)) { echo '<span style="error">Please tick box.</span>'; } ?></label>
            </span>
        </div>

    </li>
        

    <li class="buttons">

        <input id="saveForm" name="saveForm" class="btTxt submit" type="submit" value="Submit" />

    </li>


    </ul>


    <input type="hidden" name="stamp" id="stamp" value="<?php echo md5(uniqid(rand(),true)); ?>" />


    </form>


    </div><!--container-->

    <img id="bottom" src="images/bottom.png" alt="" />

    <?php

    include('includes/footer.php');

    ?>
    SignIn.php
    PHP Code:
    <?php

    include ('includes/functions.php');
    include (
    'includes/config.inc.php');
    include(
    'includes/header.php');

    $ip getenv("REMOTE_ADDR");
    $hr getenv("HTTP_REFERER");
    $hst gethostbyaddr$_SERVER['REMOTE_ADDR'] );
    $ua $_SERVER['HTTP_USER_AGENT'];

    if (isset(
    $_POST['saveForm'])) { // Check if the form has been submitted.

        
    $token md5(uniqid(rand(), TRUE));
        
    $_SESSION['token'] = $token;
        
    $_SESSION['token_time'] = time();

        require_once (
    'connect.php'); // Connect to the Database.


        
    if (checkNotEmpty($_POST['email'])) {
          if ((
    strlen($_POST['email']) > '80') || (strlen($_POST['email']) < '7')) {
                  
    $email FALSE;
              } else {
                  
    $email strtolower(strip_tags($_POST['email']));
          }
        } else {
              
    $email FALSE;
        }    

        if (
    checkNotEmpty($_POST['password'])) {
          if ((
    strlen($_POST['password']) > '30') || (strlen($_POST['password']) < '8')) {
                  
    $p FALSE;
              } else {
                  
    $p strtolower(strip_tags($_POST['password']));
              }
        } else {
                  
    $p FALSE;
        }    

          if (
    strlen($_POST['stamp']) == 32) {
           
    $s TRUE;
          }

        if (
    $email && $p && $s) { // If everything's OK.

          // clean up for database
          
    $email mysql_real_escape_string($email);
          
    $ip mysql_real_escape_string($ip);

          
    // basic brute force protection
            /*$ip = $_SERVER['REMOTE_ADDR'];
            $sql = "SELECT id, email FROM login_attempts WHERE email = '$email' AND ip = '$ip'";
            $res = mysql_query($sql);

            if (mysql_num_rows($res) > '8') {
            $feedbackstyle = "error";
            $feedback = "You have made 8 failed login attempts.<br />Your Account has now been blocked.";
            include ('includes/footer.php');
            exit;
          }
          mysql_free_result($res);*/

            // Query the Database.
            
    $query "SELECT id, email, name FROM pusers WHERE (email='$email' AND password = CONCAT(SUBSTRING(password, 1, 32), SHA1(CONCAT(SUBSTRING(password, 1, 32), '$p')))) AND disabled = 'N' LIMIT 1";
            
    $result mysql_query ($query);

            
    // A match was made
            
    if (mysql_num_rows($result) == 1) {

                
    // Register the values and redirect.
                
    $row mysql_fetch_array($resultMYSQL_NUM);
                
    mysql_free_result($result);
                
    mysql_close(); // Close the db connection.            

                
    $_SESSION['name'] = escape_data(htmlspecialchars($row[2]));
                
    $_SESSION['email'] = $row[1];
                
    $_SESSION['u_id'] = $row[0];
                
    $user_id $row[0];
                
    session_regenerate_id();
                
    $_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
                
    $_SESSION['timestamp'] = time();
                
    $_SESSION['theagent'] = md5($_SERVER['HTTP_USER_AGENT']);
                
    $_SESSION['HTTP_USER_AGENT'] = $_SERVER['HTTP_USER_AGENT'];

                
    // Start defining the URL
                
    $url 'http://' $_SERVER['HTTP_HOST'] . dirname($_SERVER['PHP_SELF']);            

                
    // Check for trailing slash.
                
    if ((substr($url, -1) == '/') OR (substr($url, -1) == '\\') ) {
                    
    $url substr ($url0, -1); // Chop off the slash.
                
    }            

                
    // Add the Page
                
    $url .= '/index.php';

                
    ob_end_clean(); // Delete the buffer.
                
    header("Location: $url");
                exit(); 
    // Quit the script.

            
    } else { // No match was made.

                  // Alert Administrator of Failed Login Attempt
                  
    $to      "michael@meownsite.co.uk";
                  
    $subject "Profiler | Failed Login Attempt";
                  
    $message "Hello\n\nEmail: $email\n\nIP: $ip";
                  
    $headers 'From: no-reply@meownsite.co.uk' "\r\n" .
                      
    'Reply-To: no-reply@meownsite.co.uk' "\r\n" .
                      
    'X-Mailer: PHP/' phpversion();
                  
    mail($to$subject$message$headers);

                    
    // log failed attempt, in case brute force
                    
    $ip $_SERVER['REMOTE_ADDR'];
                    
    $sql "INSERT INTO login_attempts VALUES ('','$email','$ip',now()) LIMIT 1";
                    
    mysql_query($sql);            

                    
    $feedbackstyle "error"
                    
    $feedback "Either the email address and password entered do not match those on file, or you have not yet activated your account.";

                      
    $sqler "SELECT * FROM login_attempts WHERE ip = '$ip' AND date_added  >= (NOW()-1)";
                      
    $reser mysql_query($sqler);                       

                      if (
    mysql_num_rows($reser) > 3) {

                              
    $sqlupdate "UPDATE pusers SET disabled = 'Y' WHERE email = $email";
                              
    mysql_query($sqlupdate);                     

                            
    // uh-oh notify
                            
    $to      "michael@michaelmcg.co.uk";
                            
    $subject "Profiler | 3 Failed Login Attempts...";
                            
    $message "Hello\n\nEmail: $email\n\nIP: $ip";
                            
    $headers 'From: no-reply@meownsite.co.uk' "\r\n" .
                          
    'Reply-To: no-reply@meownsite.co.uk' "\r\n" .
                          
    'X-Mailer: PHP/' phpversion();
                              
    mail($to$subject$message$headers);
                    }
                    
    mysql_free_result($reser);
                }

            } else { 
    // If everything wasn't OK.
            
                    
    $feedbackstyle "error";
                    
    $feedback "Please try again.";

            }

    // End of Submit conditional.

    ?>

    <form id="form1" name="form1" class="wufoo leftLabel" autocomplete="off" enctype="multipart/form-data" method="post" action="<?php echo(''.htmlentities($_SERVER["PHP_SELF"]).''); ?>">

    <div class="info">

        <h2>Sign in</h2>

        <div>Complete the form below to sign-in to our web site.</div>

    </div>

    <?php

    if (isset($feedback)) { 

         echo 
    '<div class="' $feedbackstyle '">';
         echo 
    $feedback;
         echo 
    '</div>';
         
    }

    ?>

    <ul>

    <li id="foli7">

        <label class="desc" id="title7" for="email">Email<span id="req_7" class="req">*</span></label>

        <div>
            <input id="email" name="email" type="text" class="field text medium" value="<?php echo (isset($_POST['email'])) ? htmlspecialchars($_POST['email']) : ''?>" maxlength="80" tabindex="4" <?php if ((isset($_POST['saveForm'])) && ($email == FALSE)) { echo 'style="border:2px solid red"'; } ?> /> 
        </div>

    </li>

    <li id="foli414">

        <label class="desc" id="title414" for="password">Password<span id="req_414" class="req">*</span></label>

        <div>
            <input id="password" name="password" type="password" class="field text medium" value="<?php echo (isset($_POST['password'])) ? htmlspecialchars($_POST['password']) : ''?>" maxlength="30" tabindex="9" <?php if ((isset($_POST['saveForm'])) && ($p == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
            <label for="password">Must be between <var id="rangeMinMsg414">8</var> and <var id="rangeMaxMsg414">30</var> characters.&nbsp;&nbsp;&nbsp; <em class="currently">Currently Used: <var id="rangeUsedMsg414">0</var> characters.</em></label>
        </div>

    </li>

        <li class="buttons">
        <input id="saveForm" name="saveForm" class="btTxt submit" type="submit" value="Submit" />
        </li>
        
    </ul>

    <input type="hidden" name="stamp" id="stamp" value="<?php echo md5(uniqid(rand(),true)); ?>" />

    </form>

    </div><!--container-->

    <img id="bottom" src="images/bottom.png" alt="" />

    <?php

    include('includes/footer.php');

    ?>
    Index.php
    PHP Code:
    <?php # index.php

    include ('includes/functions.php');
    include (
    'includes/config.inc.php');
    include(
    'includes/header.php');

    if (isset(
    $_SESSION['HTTP_USER_AGENT'])) {
        if (
    $_SESSION['HTTP_USER_AGENT'] != md5($_SERVER['HTTP_USER_AGENT'])) {
            
    header("Location: /signup/signout.php");
        }
    } else {
        
    $_SESSION['HTTP_USER_AGENT'] = md5($_SERVER['HTTP_USER_AGENT']);
    }

    require_once (
    'connect.php'); // Connect to the Database.

    $clean = array();
    $html = array();

    $clean['name'] = $_SESSION['name'];
    $html['name'] = htmlentities($clean['name'], ENT_QUOTES'UTF-8');

    echo 
    "<h2>Welcome, {$html['name']}</h2>";
    echo 
    "<p><strong>Not {$html['name']}? <a href='signout.php'>Click here</a>.</strong></p>";

    include (
    'includes/footer.php');

    ?>
    Header.php
    PHP Code:
    <?php

    ob_start
    ();
    session_start();

    //error_reporting(E_ALL & ~E_NOTICE);

    $currentFile $_SERVER["SCRIPT_NAME"];
    $foo $_SERVER['PHP_SELF'];

    if (isset(
    $_SESSION['u_id']) && ($currentFile == '/signup/signin.php')) {
      
    header("Location: index.php");
    }

    if ((!isset(
    $_SESSION['u_id'])) && ($currentFile == '/signup/index.php')) {
      
    header("Location: signin.php");
    }

    $ip $_SERVER['REMOTE_ADDR'];

    if (isset(
    $_SESSION['ip'])) {
        if (
    $_SESSION['IP'] != $_SERVER['REMOTE_ADDR']) {
            
    header("Location: /signup/signout.php");    
        }
    }

    if (isset(
    $_SESSION['timestamp'])) {
        if ((
    time() - $_SESSION['timestamp']) > 1800) {
            
    header("Location: /signup/signout.php");
        } else {
            
    $_SESSION['timestamp'] = time();    
        }
    }


    require_once (
    'connect.php'); // Connect to the Database.

    ?>

    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

    <html xmlns="http://www.w3.org/1999/xhtml">
    <head>

    <title>Profiler</title>

    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />

      <!-- CSS -->
      <link rel="stylesheet" href="css/structure.css" type="text/css" />
      <link rel="stylesheet" href="css/form.css" type="text/css" />            

    </head>

    <body id="public">    

      <img id="top" src="images/top.png" alt="" />

      <div id="container">
    Footer.php
    PHP Code:
    </body>

    </html>

    <?php

    if (isset($_POST['saveForm'])) {

      
    // close my db connection
      
    mysql_close($dbc);

    }

    ?>
    Snippet from Functions.php
    PHP Code:
      function checkNotEmpty($s) {
          return (
    trim($s) !== '');
      }  

      function 
    getIntValue($s) {
        if (!
    is_numeric($s)) {
          return 
    false;
        } else {
          return (int)
    $s;
        }
      } 
    Like I say I understand there is a large amount of code on display here, but I would really value some of the professionals taking a look over it and maybe making same obvious notes of things I haven't considered.

    I've tried to consider things like XSS and SQL Injection.

    I'm going to use htaccess/htpasswd on my administrator directory as well as try enforce a strict password scheme for users.


    Many thanks for any assistance

  2. #2
    SitePoint Evangelist hexburner's Avatar
    Join Date
    Jan 2007
    Location
    Belgium
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    A couple of remarks:
    • eregi is deprecated in recent PHP-versions, use preg_match with the i modifier, see Deprecated features in PHP 5.3.x
    • users can disable the referer, so using the HTTP_REFERER is unreliable
    • with some providers you get a new ip for every request made, so the users' ip might not be static, which also means using the ip address is unreliable
    • the validation for the e-mail address could be simplified and improved:
      PHP Code:
      <?php
      $email 
      trim(stripslashes($email));
      if (!
      preg_match('/^[a-z0-9_\-]+(\.[_a-z0-9\-]+)*@(([_a-z0-9\-]+\.)+([a-z]{2}|aero|arpa|biz|com|coop|edu|gov|info|int|jobs|mil|museum|name|nato|net|org|pro|travel))$/i'$email))
      {
          
      // E-mail address is invalid
      }
      ?>
    • Use mysql_real_escape_string to escape the users' input to prevent SQL-injections
    • To prevent cross-site scripting, you can use htmlspecialchars when outputting the users' input.
      You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users' input comes from your login page and not from another host.
    FOR SALE: 1 set of morals, never used, will sell cheap

  3. #3
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    That's good advice there.

    I'll add though, that if you're using using at least PHP 5.2 then you also have filter_var and filter_input at your disposal for sanitizing and validating.

    Code php:
    $email = filter_input(INPUT_POST, 'email', FILTER_VALIDATE_EMAIL);
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  4. #4
    SitePoint Evangelist hexburner's Avatar
    Join Date
    Jan 2007
    Location
    Belgium
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thank you.
    But, remember that those filters are not as reliable as one might think.
    For example:
    PHP Code:
    <?php
        
    // Should all return false
        
    filter_var('http://.'FILTER_VALIDATE_URL); // returns http://.
        
    filter_var('a@b-.c'FILTER_SANITIZE_EMAIL); // returns a@b-.c
        
    filter_var('http://.'FILTER_SANITIZE_URL); // returns http://.
    ?>
    FOR SALE: 1 set of morals, never used, will sell cheap

  5. #5
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    Quote Originally Posted by hexburner View Post
    But, remember that those filters are not as reliable as one might think.
    For example:
    PHP Code:
    <?php
        
    // Should all return false
        
    filter_var('http://.'FILTER_VALIDATE_URL); // returns http://.
        
    filter_var('a@b-.c'FILTER_SANITIZE_EMAIL); // returns a@b-.c
        
    filter_var('http://.'FILTER_SANITIZE_URL); // returns http://.
    ?>
    Sanitize only removes characters, it doesn't check it it's valid. That's why there are separate sanitize and validate filters.

    That first example with http://. is an interesting variation though.
    I do see though on the validate filters page that there is an optional FILTER_FLAG_PATH_REQUIRED flag that can be applied. Would that help to deal with it?
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #6
    SitePoint Evangelist hexburner's Avatar
    Join Date
    Jan 2007
    Location
    Belgium
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    When FILTER_FLAG_PATH_REQUIRED is ommitted, filter_var returns false.

    Another interesting issue with the e-mail validation filter:
    PHP Code:
    <?php
        filter_var
    ('a.@b.c'FILTER_VALIDATE_EMAIL); // returns a.@b.c
    ?>
    Also, the domain names' TLD should be validated too, which filter_var does not.
    I don't think filter_var is the most reliable function for validation, for now.
    FOR SALE: 1 set of morals, never used, will sell cheap

  7. #7
    SitePoint Addict
    Join Date
    Apr 2009
    Posts
    248
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by hexburner View Post
    When FILTER_FLAG_PATH_REQUIRED is ommitted, filter_var returns false.

    Another interesting issue with the e-mail validation filter:
    PHP Code:
    <?php
        filter_var
    ('a.@b.c'FILTER_VALIDATE_EMAIL); // returns a.@b.c
    ?>
    Also, the domain names' TLD should be validated too, which filter_var does not.
    I don't think filter_var is the most reliable function for validation, for now.
    Your top level validation doesn't include any country codes, and with ICANN considering opening more and more domains, you're excluding people who might be using those.

    '.c' is a valid TLD according to the spec, it's just not distributed by ICANN. Excluding the filter_var function based on your example is silly. filter_var is significantly easier to use than rolling your own function, and as such is usually a much better idea.

  8. #8
    SitePoint Evangelist hexburner's Avatar
    Join Date
    Jan 2007
    Location
    Belgium
    Posts
    591
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by SituationSoap View Post
    Your top level validation doesn't include any country codes, and with ICANN considering opening more and more domains, you're excluding people who might be using those.

    '.c' is a valid TLD according to the spec, it's just not distributed by ICANN. Excluding the filter_var function based on your example is silly. filter_var is significantly easier to use than rolling your own function, and as such is usually a much better idea.
    Your entirely right.
    The syntax is correct, according to the specifications in RFC 822.
    And surely, one should not disregard filter_var based on my assumptions above.
    But, .c is not a registered TLD, and therefor unexisting and, more importantly, invalid.

    In this case the validity of the e-mail address is not important, since it will be matched to an e-mail address stored in the database.
    As such, the syntax of the e-mail address does not matter.

    The best way to validate an e-mail address, is to send an e-mail to that address with a verification link when an e-mail address is stored.
    But when e-mail validition is important, where the situation does not allow you to send a verification link to a given e-mail address, filter_var does not suffy, in my opinion.

    In that last situation, not only the syntax matters, but also the validity of the TLD, the existence of the domain itself and the existence of MX-records for that domain.
    I have to admit, it is reeeee...aly far fetched, but some government organisations require such intensive validation, like FOV (Federation of Organisations active in the popular (non-formal) adult education scene).

    For really thorough e-mail validation, you would need:
    • A list of valid TLDs, which can be found at http://www.icann.org/en/registries/t...el-domains.htm. Storing and updating that data on a regular basis for future reference should be no problem. To validate the TLD of the domain, you could create your own function using parse_url and the list of valid TLDs.
    • Syntax validation, filter_var suffies.
    • To check the MX-records of the domain, dns_get_mx would be sufficient, since when there are no MX-records present, the domain is unexisting or there are simply no MX-records set for that domain and therefor that domain cannot accept any e-mails.


    Input validation is a pain, but if such things are high priority requirements by the client, then I will meet the clients' demands and Thorough will be my middle name.
    ...
    And my last name will be Nuts
    FOR SALE: 1 set of morals, never used, will sell cheap

  9. #9
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Many thanks for the replies guys. Really appreciate your input. It's always valid

    Hexburner, many thanks for your email validation reg exp. I'm going to use it from now on, works great!

    -------------------

    Hexburner, in #2 you mention:

    # Use mysql_real_escape_string to escape the users' input to prevent SQL-injections
    # To prevent cross-site scripting, you can use htmlspecialchars when outputting the users' input.
    You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users' input comes from your login page and not from another host.
    In my signin.php and signup.php I make use of mysql_real_escape_string. Although, I'm thinking of using prepared statements for future work.

    XSS
    In my index.php I used:
    PHP Code:

    $clean 
    = array();
    $html = array();

    $clean['name'] = $_SESSION['name'];
    $html['name'] = htmlentities($clean['name'], ENT_QUOTES'UTF-8');

    echo 
    "<h2>Welcome, {$html['name']}</h2>";

    echo 
    "<p><strong>Not {$html['name']}? <a href='signout.php'>Click here</a>.</strong></p>"
    would this suffice as escaping output?

    -------------------

    users can disable the referer, so using the HTTP_REFERER is unreliable
    Is there any way around this, or is it not really too big of an issue to be concerned over?

    -------------------

    You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users' input comes from your login page and not from another host.
    Could you perhaps show me a brief example of how I could do this?
    I tried to use a token verfication method in my signup.php script above. Is this the best way to do it?




    In terms of Security on the signup.php and signin.php, do you think I have sufficiently covered everything I need to?


    Also, many thanks also to pmw57. I think I should look to upgrade more of my sites to PHP 5.2. I'll speak to my host on this so I can use the FILTER functions.


    I really value the feedback everyone has given on this thread so far. It means a lot.

    I think I'm going to try stick with these standards on future apps. Or just use CodeIgiter/ZF

  10. #10
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    Quote Originally Posted by invision2 View Post
    In my index.php I used:
    PHP Code:
    $clean = array();
    $html = array();

    $clean['name'] = $_SESSION['name'];
    $html['name'] = htmlentities($clean['name'], ENT_QUOTES'UTF-8');

    echo 
    "<h2>Welcome, {$html['name']}</h2>";

    echo 
    "<p><strong>Not {$html['name']}? <a href='signout.php'>Click here</a>.</strong></p>"
    would this suffice as escaping output?
    Yes, that does the job well.

    Quote Originally Posted by invision2 View Post
    I tried to use a token verfication method in my signup.php script above. Is this the best way to do it?
    Yes, that's a good way of doing it that you're using there.

    Quote Originally Posted by invision2 View Post
    In terms of Security on the signup.php and signin.php, do you think I have sufficiently covered everything I need to?
    Nearly, but not quite. Your code appears to rely on the server aplying magic quotes to things like the $_POST variables. Magic quotes are not good or reliable, so if they are enabled, you need to remove them, and then at the database side of things you should use mysql_real_escape_string instead. A good attempt to do that has been made, but the $p variable appears to have been missed.

    A code smell to watch out for is where get_magic_quotes_gpc() is not used when retrieving input. The signup code strips the slashes, but it shouldn't if the server already has magic_quotes disabled.

    PHP Code:
    $email $_POST['email'];
    if (
    get_magic_quotes_gpc()) {
        
    $email stripslashes($email);
    }
    $email strip_tags(trim(strtolower($email))); 
    Then where the variable is used in the database, another useful code smell is if mysql_real_escape_string is not used with the SQL statement.

    On the signin form for example, there's a chunk of commented code between the escaped variables and where they're used in the SQL statement. This means that it's easy to miss whether the variables have ben escaped or not, which is an existing problem in the code with the $p variable.

    That problem can be prevented by moving the escaping to where the variables are placed in the SQL string.

    The query page shows a good way to do this, where sprintf is used to place sanitised variables within the SQL statement.

    PHP Code:
    $sql sprintf('SELECT id, email, name FROM pusers WHERE (email="%s" AND password = CONCAT(SUBSTRING(password, 1, 32), SHA1(CONCAT(SUBSTRING(password, 1, 32), "%s")))) AND disabled = "N" LIMIT 1',
        
    mysql_real_escape_string($email),
        
    mysql_real_escape_string($p)
    ); 
    $result mysql_query ($sql); 
    Where numbers are used, you would use %d instead of "%s" and of source as they're numbers you wouldn't use mysql_real_escape_string, but something like intval($id) instead.

    So in the end, it's basically code smells that you're looking for.
    • Are the inputs being appropriately stripped, sanitised and validated.
    • Are the variables for SQL statements being appropriately escaped to protect the database.
    • Are the outputs being appropriately escaped to protect your page.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  11. #11
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Many many thanks for the detailed response Paul.

    The 6 sites these appear on are fairly old and it's definitely not the way I build sites now. I like to split it up(as you have done) by filter, db escape, output escaping.

    My current Data Escaping function is like this:

    PHP Code:
    // Create a function for escaping the data.

    function escape_data ($data) {

    // Use Magic Quotes.
    if(ini_get('magic_quotes_gpc')) {
        
    $data stripslashes($data);
    }

    // Check for mysql_real_escape_string() support.
    if (function_exists('mysql_real_escape_string')) {
            global 
    $dbc// Need the connection.
            
    $data mysql_real_escape_string (trim($data), $dbc);
    } else {
            
    $data mysql_escape_string (trim($data));
    }    

    // Return the escaped value.
    return $data;        

    // End of Function 
    Does this fix the magic quotes issue? Does this mean, I should just use my escape_data function for db input rather than stripslashes?

    Concur with your thoughts on keeping mysql_real_escape_string in the actual query. Keeps things nice and simple.


    Again, really appreciate you taking the time to look over my work.

    I'm going to try roll this out across my existing 6 apps.

    I think I'll then take a look at all my web site scripts for SQL Injection and XSS issues as I think this is where I may have holes.



    Thanks again Paul!

  12. #12
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    Quote Originally Posted by invision2 View Post
    Does this fix the magic quotes issue? Does this mean, I should just use my escape_data function for db input rather than stripslashes?
    There are some issues with that quoted code.

    • The ini_get function may return "off" which in your if condition would evaluate to true. Please use get_magic_quotes_gpc() instead.


    • Checking if mysql_real_escape_string exists looks to be really old code, from when PHP older than 4.3.0 is being used. The earlier mysql_escape_string had some serious security issues, so if you're going to use that then you also need to protect against those other security problems too.


    • The escape function has a significant problem, in that it cannot correctly escape form inputs that are not destined for a database, and it cannot correctly escape PHP values that have not had magic_quotes applied to them.


    • Using that escape function means that you have to escape the values either when you receive the input, or when you apply the values to the SQL string.

      When you escape the values only you receive the input, later on through the code when the values are applied to the SQL statement you need to remember to not apply the mysql_real_escape_string function to those specific values, and to apply it to other values that aren't directly from the input. The SQL statement would then have some values escaped that did not directly originate from user input, and other values not escaped because that has already been done to them. That can only lead to bad things happening.

      When the values have magic quotes and mysql_real_escape_string applied only at the SQL statement, the part of the code where the inputs are retrieved then need to remain unescaped where those values are later on escaped at the sql code. That can only lead to bad things happening too.


    So please don't use that code. You're only setting yourself up for failure should you do so.

    Instead, set up a demarcation point where the inputs have magic quotes stripped from them, are sanitised, and validated.
    Then later on, where you handle the database work, escape all of the values going in to the database, whether that means using mysql_real_escape_string or intval or other techniques.

    You can use separate functions to perform these tasks, but just ensure that the next person coming along (who may also be yourself in 6 months time) can clearly understand that the values are being properly treated.

    That has been found to be the only appropriate way to sufficiently protect yourself from problems that may occur.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  13. #13
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Eeek. Sounds like I should be ditching that escape_data function altogether.

    Would you suggest sticking with mysql_real_escape_string for all database input and not using stripslashes at all?

  14. #14
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    Stripslashes is important and necessary to perform for input, but only when magic_quotes are enabled. Why? Some inputs are destined for the database, and some are not.

    If you don't strip slashes, they will still be there in the code when you later on escape the data for the database.

    When working with a new server you can entirely disable magic quotes, and PHP 6.0 will not allow magic quotes so that stripping slashes is not needed, but where there is pre-existing code that hasn't been protected, or where your code will be used in unknown environments, you need to strip slashes on all inputs.

    escaping strings is a completely separate task that applies only to data being sent to the database. That data can come from user inputs, and that data can come from other processes within your code. That's why escaping the data at the point where the data gets added to the database, is the only way to consistently ensure that the correct behaviour occurs.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  15. #15
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hi Paul,

    Aaah, thanks for clearing this up.

    From now on I will escape input data solely at the SQL query.

    I'm going to work on a very basic Guestbook app shortly and try include everything I've learned from this. Building both a procedural and OOP version Stay tuned

    Again, can't thank you enough for your help. I've a feeling I'll be returning to this thread often.

  16. #16
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    By the way, as a good example of why to escape the output, the first 5 minutes of The End of All Things provides a very good run-down of the dangers that cross-site-scripting exposes your web site to.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  17. #17
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    If header.php is what protects your content from non logged in users, you have no protection.

    header() does not stop script execution. Sending a location header is merely a recomendation to the client that they should load another url. They might not listen.

    use exit() to stop script execution.

    Try it
    PHP Code:
    header('content-type: text/plain');
    $opts = array('http' => array('max_redirects'  => 0'ignore_errors' => true));
    readfile('http://example.com/members_only.php'falsestream_context_create($opts)); 

  18. #18
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Manythanks again Paul. I hope to work on a very small Guestbook ap and perhaps post the results here too.

    crmalibu, many thanks for your thoughts.
    So I would include the header, and then follow it with an exit() statement?

    Could you show me an example, taking my header.php above in consideration?
    Sorry, I'm just a bit unsure how to use your example.


    Many thanks for your help.

  19. #19
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    The email regex hexburner provides is equally flawed. It rejects legitimate Gmail addresses.

    A more inclusive regex for email addresses would be:
    PHP Code:
    preg_match('/^[a-z0-9!#$%&\'*+\\/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&\'*+\\/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/i'$email)) 
    Though beware, it does not properly handle quoted addresses such as "John Doe"@example.com (not that anyone uses those).

    You can combine sprintf() and mysql_query() in a way that makes it easy for you to escape strings and yet make it very hard to do something dumb (like not escaping). Here's an example:
    http://php.net/manual/en/function.mysql-query.php#81188

  20. #20
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Many thanks sk89q. I use your PHP Security checklist a lot. Brilliant resource.

    Thank you. Will update the RegEx shortly and take a look at the SQL query example.

  21. #21
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Off Topic:

    Ah! I totally forgot about that, and that thing was riddled with grammar errors. (Now fixed.) I also thought that everyone forgot about that, since the domain change was fairly abrupt.

    I also came back to edit my post, since it came off a little harsh. Sorry, hexburner. Can't fix that now.

  22. #22
    SitePoint Wizard
    Join Date
    Mar 2008
    Location
    United Kingdom
    Posts
    1,285
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Heh, yep it's a great resource.

    The plan now is to survey all my existing sites, try plug some holes and then monitor my access/error logs on a daily basis. Good times!

  23. #23
    SitePoint Addict
    Join Date
    Apr 2009
    Posts
    248
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    If you're going to be rewriting significant tracts of code anyway, could I suggest moving away from the mysql extension, and toward MySQLi or PDO, which allow prepared statements, and thus require zero escaping of code?


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •