¿Este script de logueo es suficiente?

[anacona16]

Tengo este script para validar los usuario registrados en un sistema:

Código PHP:

<?php 
    session_start();
    include'../include/conexion.php';
    include'../include/funciones.php';  
 
    if (!isset($_POST['username'])){
        setcookie("msglog","&iexcl;Usuario no Autenticado!",time()+2,"/");
        redirect("../index.php");
    }
 
    $urlAcceso  = $_GET["urlAcceso"];
 
    $consultaUsuario = $db->prepare("SELECT * FROM tb_acceso WHERE username = ? ");
    $consultaUsuario->bindParam(1, $_POST['username'], PDO::PARAM_STR);
    $consultaUsuario->execute();        
 
    if ($consultaUsuario->rowCount() == 0){
        setcookie("msglog","&iexcl;El usuario <b>".$_POST['username']."</b> no tiene acceso!",time()+2,"/");
        redirect("../index.php");
        break;
    }else{
        $usuario        = $consultaUsuario->fetch();
        $passUsuario    = $usuario['password'];             
 
        if (md5($_POST['password']) == $passUsuario){
            $_SESSION['token']      = rand(0,10);
            $_SESSION['username']   = $usuario['username'];
            $_SESSION['descusua']   = $usuario['descusua'];
            $_SESSION['rollusua']   = $usuario['rollusua']; 
            
            redirect('../vistas/panel.php');        
 
        }else{
            setcookie("msglog","&iexcl;Contrase&ntilde;a Incorrecta!",time()+2,"/");
            redirect ("../index.php");
        }
    }
 

El archivo funciones.php contienes una funcion JavaScript para redireccionar (tal vez no sea la mejor opcion pero por ahora asi lo hago)

Quiero que por favor me den su opinion de este script. ¿Es suficiente? ¿Esta mal? ¿Que tan mal?

Gracias.

[anacona16]

Este es el script para cerrar session

Código PHP:

<?php 
    session_start();
 
    include("../include/funciones.php");    
    unset($_SESSION['token']);
    unset($_SESSION["user"]);
    unset($_SESSION["roll"]);
    unset($_SESSION['username']);
 
    session_destroy();
    $_SESSION = array();
 
    setcookie("msglog","Sesi&oacute;n Cerrada!!!",time()+2,"/");
 
    redirect("../index.php");
 

[anacona16]

Y este es el script para validar en cada pagina que el usuario este logueado

Código PHP:

if(!isset($_SESSION['username']) || !isset($_SESSION['token'])){
            setcookie("msglog","Usuario no autenticado",time()+2,"/");
            redirect('../index.php');
 
        }
 

[perryjr]

1. Supongo, que la función redirect(..) cuando la usas, hace un exit o algo así para interrumpir que la página se mande. No sirve de nada redireccionar al usuario, si tb le envias la página que no debería ver (un hacker si podría verla MUY facilmente)

2. Otra posible debilidad, es que aquí compruebas:

Código PHP:

if (!isset($_POST['username']))
 

pero aquí no:

Código PHP:

$urlAcceso  = $_GET["urlAcceso"];
 

Nada te puede asegurar que el cliente (algo que no controlas casi en absoluto, no sabes quien hay usando tu página) haya enviado /pagina?urlAccesso=... Y tu código fallaría.

Código PHP:

$urlAcceso  = isset($_GET['urlAcceso']) ? $_GET["urlAcceso"] : '/pagina-principal-del-sitio';
 

3. Procura usar cadenas simples '...' en vez de "...", porque cada vez que usas "..." PHP tiene que escanear la cadena entera (aunque sean cortas, pero mira es un tiempo y recursos gastados) en busca de cosas que reemplazar, tipo "antes $reemplazar despues". Es un consejo para incrementar el rendimiento, aunque no es absolutamente necesario.

Finalmente solo decirte que:
1. Excelente que guardes las contraseñas con md5() y lo compruebes con eso.
2. Excelente código, fácil de leer y muy organizado.

[anacona16]

perryjr gracias por tu comentario.

Código PHP:

if (!isset($_POST['username']))
 

Esto lo hago para verificar si se envio el formulario. ¿Por que habria debilidad?

Código PHP:

$urlAcceso  = $_GET["urlAcceso"];
 

Esto era algo que iva a implementar pero no lo hice, se quedo el codigo ahi.

Gracias por el consejo de las comillas sencillas.

Espero sigas sugiriendo cambios.

Gracias.

[perryjr]

Debilidad me refiero a que dependiendo de las configuracion de PHP que tengas en el servidor, si tu intentas acceder a $_GET['urlAcceso'], y no existe el elemento 'urlAcceso' en $_GET porque no se ha enviado tal elemento al servidor, tu script fallará, dejando tras de sí un error. De nuevo, dependiendo de las configuraciones de tu servidor, es posible que ese error llegue al usuario (llamemoslo hacker mientras no lo conozcamos =) ).

Esto me pasó a mi con la web de una inmobiliaria. Enviando /contacto.php, asi tal cual, sin datos; provocaba un error, que por configuraciones del servidor se enviaba al cliente (PHP_warning... todo ese rollo). La gracia es que ese rollo contenia nombres de archivo, líneas en esos archivos, etc. gracias a las cuales un atacante podría beneficiarse. Y de hecho les demostré que eso, unido a un pequeño error de SQL injection, me daba acceso a su base de datos.

No es que eso por si solo sea un problema, sino que es una debilidad más que se irá sumando hasta que tengan poder para hackear la página.

De todas maneras, es verdad que no somos Facebook, no somos Tuenti, esperemos y rezemos que Anonymous no tenga interés en nuestros datos... =) Pero aun asi nunca viene de más ponerle seguridad a las cosas

Es como otra cosa que no he mencionado ahora que me acuerdo, porque no me pareció importante. Y si alguien hace esto:
http://example.com/login.php?urlAcce...-infectada.php

Google por ejemplo si implementa mecanismos para no redireccionar fuera del dominio google.com; porque lo necesita. Seguramente ni tu ni yo lo necesitemos nunca, pero no está de más conocer que el problema esta ahí, y coger buenas costumbres en la programación para evitarlos =)

[anacona16]

Ok gracias, eso en cuanto al $_GET['urlAcceso'], y en cuanto a lo de asignacion de las variables de session, busqueda en la base de datos y los demas?

[perryjr]

1. Evitas SQL Injection, con lo de bindParam().
2. No guardas la contraseña en la $_SESSION (sí, he visto programadores que lo hace -.-')

Me parecen bastante bien. Quizás podrías mejorar en que la llamada a setcookie(...); pa los mensajes es muy repetitiva. Puedes crear una funcion setMessage(); o algo así para abstraerte de todo eso (y si cambias el sistema de mensajes, no tienes que cambiar tanto código).

Por lo demás esta muy bien tu código en esos sentidos.

[anacona16]

Eso de setMessage() ya lo habia pensado, hacer una clase que contenga todos esos mensajes en un script aparte.