这段代码安全吗？

Question

<?php
session_start();

include("connect.php");

$timeout = 60 * 30;
$fingerprint = md5($_SERVER['REMOTE_ADDR'] . $_SERVER['HTTP_USER_AGENT']);

if(isset($_POST['userName']))
{
    $user = mysql_real_escape_string($_POST['userName']);
    $password = mysql_real_escape_string($_POST['password']);
    $matchingUser = mysql_query("SELECT * FROM `users` WHERE username='$user' AND password=MD5('$password') LIMIT 1");
    if (mysql_num_rows($matchingUser))
    {
        if($matchingUser['inactive'] == 1)//Checks if the inactive field of the user is set to one
        {
            $error = "Your e-mail Id has not been verified. Check your mail to verify your e-mail Id. However you'll be logged in to site with less privileges.";
            $_SESSION['inactive'] = true;
        }
        $_SESSION['user'] = $user;
        $_SESSION['lastActive'] = time();
        $_SESSION['fingerprint'] = $fingerprint;
    }
    else
    {
        $error = "Invalid user id";
    }
}
if ((isset($_SESSION['lastActive']) && $_SESSION['lastActive']<(time()-$timeout)) || (isset($_SESSION['fingerprint']) && $_SESSION['fingerprint']!=$fingerprint)
     || isset($_GET['logout'])
    )
{
    setcookie(session_name(), '', time()-3600, '/');
    session_destroy();
}
else
{
    session_regenerate_id(); 
    $_SESSION['lastActive'] = time();
    $_SESSION['fingerprint'] = $fingerprint;
}
?>

这只是 http://en.wikibooks.org/wiki/PHP_Programming/User_login_systems 的修改版本。 setcookie(session_name(), '', time()-3600, '/');在这里的作用是什么？

这里有一个错误： 我使用这个登录表单：

<?php 
   if(!isset($_SESSION['user']))
    {
        if(isset($error)) echo $error;
           echo '<form action="' . $_SERVER["PHP_SELF"] . '" method="post">
        <label>Username: </label>
        <input type="text" name="userName" value="';if(isset($_POST['userName'])) echo $_POST["userName"]; echo '" /><br />
        <label>Password: </label>
        <input type="password" name="password" />
        <input type="submit" value="Login" class="button" />
        <ul class="sidemenu">
        <li><a href="register.php">Register</a></li>
        <li><a href="forgotPassword.php">Forgot Password</a></li>
    </ul>
    </form>';
    }
    else
    {
        echo '<ul class="sidemenu">
        <li>' . $_SESSION['user'] . '</li>
        <li><a href="' . $_SERVER["PHP_SELF"] . '?logout=true">Logout</a></li>
        </ul>';
    }
?>

问题在于当我退出登录时，页面仍然保持原样，即登录表单没有显示出来，而是显示了相同的注销和用户。当我刷新页面时，它才恢复正常。

- iSattar

---

+1：没有理由对一个合法的问题进行负评。 - Juliet

== 1)//检查用户的不活动字段是否设置为1 <-- 这是每个人都讨厌看到的类型注释。通过阅读代码，您就知道了。 - Pim Jager

抱歉，Pim。我只是想告诉读者，我有一个名为“inactive”的字段，如果用户处于非活动状态，则存储值1。如果你感到受伤，我很抱歉。 - Abdulsattar Mohammed

Answer 1

当您注销时，首先会排队销毁cookie（它将在响应发送后发生），然后立即呈现您的页面。浏览器在呈现之前没有机会删除cookie，因此您的$_SESSION变量仍然存在。

PHP文档中关于session_destroy的说明如下：

session_destroy()销毁与当前会话相关联的所有数据。它不取消设置与会话相关联的任何全局变量，也不取消会话cookie。

解决方案是，不要销毁会话和cookie，而是简单地取消设置会导致身份验证的变量：

unset($_SESSION['user']);
unset($_SESSION['lastActive']);
unset($_SESSION['fingerprint']);

注：我建议将你的代码分成函数。这会使它更有组织和可读性（如果你做得对，还可以重复使用）。

- strager

---

啊，太棒了，这为我解决了一些问题。我一直在想为什么session_destroy()对我没有任何作用，所以最终像你说的那样取消了所有的会话变量。 - cletus

Answer 2

$matchingUser['inactive']永远不会被设置，因为使用mysql_fetch_assoc()时无法从数据库获取实际数据。

修改后的版本：

$matchingUser = mysql_query("SELECT * FROM `users` WHERE username='$user' AND password=MD5('$password') LIMIT 1");
if (mysql_num_rows($matchingUser))
{
    $matchingUserData = mysql_fetch_assoc($matchingUser);
    if($matchingUserData['inactive'] == 1) //Checks if the inactive field of the user is set to one
    {
        $error = "Your e-mail Id has not been verified. Check your mail to verify your e-mail Id. However you'll be logged in to site with less privileges.";
        $_SESSION['inactive'] = true;
    }

- Karsten

Answer 3

一些安全注意事项：

if($matchingUser['inactive'] == 1)

更好的写法是：

if(!$matchingUser['inactive'])

因为如果数据库架构发生变化（例如，现在是整数来表示某种类型的活动（在我看来这是糟糕的设计，最好使用枚举）），你的代码会出现问题。

当然，这是一个双重否定，可能不太易读。更好的做法是:

if($matchingUser['isactive'])

甚至可以这样：

if($matchingUser->isActive())

假设您创建了一个User类等等。

以防万一，在必要的地方使用require或require_once（如果connect.php包含函数声明，则最好使用后者）。

将用户的ID存储在会话变量中，而不是用户名。有可能您会允许用户稍后更改其名称，此时会话数据将无效（至少['user']将无效）。通过ID（主键，唯一）查找数据库记录比通过用户名（可能已索引，字符串）更快。

30分钟后踢我真的很烦人。您不是我访问的唯一网站，我可能在做一些工作后重新访问（例如，如果我被叫去做某些事情，或者去吃午饭休息）。

使用htmlspecialchars来帮助预防跨站脚本攻击（XSS）。

这里不需要使用$_SERVER['PHP_SELF']：

<a href="' . $_SERVER["PHP_SELF"] . '?logout=true">

只需在不使用它的情况下编写：

<a href="?logout=true">

当用户提交POST请求时，务必进行重定向操作（TODO：记录如何操作）。否则，用户的后退按钮可能会导致重新提交数据（这可能不是您想要的结果！）。

- strager

Answer 4

$_SERVER['REMOTE_ADDR'] 如果用户在代理后面，可能会发生变化。

- stesch

Answer 5

MD5是一种相当薄弱的加密密码方式，有很多方法可以绕过它。然而，将其设置为IP地址确实有所帮助，但IP地址肯定会经常更改。

此外，您没有任何措施来确保某人不会一遍又一遍地攻击系统以破解用户的密码。

G-Man

- GeoffreyF67

---

为什么要负面评价？这是系统中一个有效的安全漏洞，这也是OP所询问的。 - Nikhil

Answer 6

使用盐值与MD5或sha1一起。 这是我用来生成密码和在登录时检查密码的方法。

function generateHash($plainText, $salt = null)
{
    define('SALT_LENGTH', 9);
    if ($salt === null)
    {
        $salt = substr(md5(uniqid(rand(), true)), 0, SALT_LENGTH);
    return array($salt, sha1($salt . $plainText) );
    }
    else
    {
        $salt = substr($salt, 0, SALT_LENGTH);
    return sha1($salt . $plainText);
    }

}

对于 $plainText，将其发送到密码变量中。

当您想要生成新的哈希时，它将返回2个值。第一个值称为“盐”，第二个值是加密后的密码。将它们都存储到数据库中。

当有人尝试登录您的网站并且您想要检查时，请将他们的密码和盐变量发送到函数中，它将返回一个哈希值。然后，您可以将其与存储在数据库中的值进行比较，以检查用户输入的密码是否正确。

- user58670

---

每次调用函数时，这将定义常量。 - Gumbo

盐应该每次都是唯一的。通常它被称为“nonce”，意思就是：一个一次性的词语。 - Nikhil

Answer 7

看起来没问题。setcookie(session_name(), '', time()-3600, '/')本质上通过将时间设置为当前时间之前来删除Cookie。

- Evan Fosmark

---

当浏览器看到一个过期日期已经过去的cookie时，它会删除这个cookie。 - Tiago

好的。为什么要删除一个从未创建过的cookie？ - Abdulsattar Mohammed

iSattar，cookie已经被会话自动创建。当您执行session_Start()时，如果不存在cookie，则会创建一个cookie。 - Evan Fosmark

Answer 8

这段代码的作用是清除旧的会话cookie，它通过将一个空字符串设置为会话cookie并将过期时间设置为过去来实现。不过我不确定为什么作者要同时使用这两种方法。

- Tiago

Answer 9

在这种情况下，您必须确保您的服务器设置正确以使mysql_real_escape_string正常工作。在这种情况下，您的代码假定魔术引号已关闭，而我经常发现许多服务器已启用它。您应该检查get_magic_quotes_gpc()返回true还是false，以查看服务器设置是否自动为您转义字符串。

代码看起来很安全，但我建议您探索一种新的进行MySQL查询的方式：PDO。这允许参数化查询。

- d03boy

Answer 10

你可以使用 Nate Abele 描述的这种方法来提高用于构建指纹的信息的可靠性。

- Gumbo