Zabezpieczenie skryptu PHP Opcje

[Baku12345]

Witam
Mam skrypt co do którego mam zastrzeżenia jeśli chodzi o bezpieczeństwo. Nie wrzucę go ze względu na to, że jest zbyt długi. Wrzucę natomiast jego uproszczoną wersję by odnieść się do problemu. A wygląda on tak

[PHP] pobierz, plaintext 
<?php
    session_start();
 
    $now = time();
    $session_data = "";
 
    $pdo = new PDO('mysql:host=localhost; port=3306; dbname=test', 'root', '');
 
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $pdo->exec("SET NAMES 'utf8'");
 
    if (isset($_POST['login']) && isset($_POST['password']))
        {
            if (!empty($_POST['login']) && !empty($_POST['password']))
                {
                    $login = addslashes(strip_tags($_POST['login']));
                    $password = hash_hmac('sha256', $_POST['password'], '2NYbH5qS8J');
 
                    $stmt = $pdo->prepare("SELECT * FROM users WHERE login = :login && password = :password");
                    $stmt -> execute(array(':login' => $login, ':password' => $password));
 
                    if ($row = $stmt->fetch(PDO::FETCH_ASSOC))
                        {
                            $_SESSION[$session_data] = array('id' => $row['id'], 'time' => $now, 'user' => $row['name']);
                        }
                    else
                        {
                            echo "Podałeś błędny login lub hasło";
                            exit;
                        }
                }
            else
                {
                    echo "Nie wprowadziłeś wszystkich danych";
                    exit;
                }
        }
 
    $max_idle_time = 900;
    if (isset($_SESSION[$session_data]) && !empty($_SESSION[$session_data]))
        {           
            if ($now - $_SESSION[$session_data]['time'] < $max_idle_time)
                {                    
                    $_SESSION[$session_data]['time'] = $now;
                    echo "Jesteś zalogowany";
// tu następuje pobranie z bazy danych danych przeznaczonych dla użytkownika o danym id a następnie ich wyświetlenie
                }
            else
                {
                    echo "Przekroczono czas bezczynności";
                    session_destroy();
                }
        }
    else
        {
            echo '<h1>Panel Użytkownika</h1>
                  <form method="post" action="">
                    <div>
                        <input type="text" name="login" placeholder="Twój login" value="">
                    </div>
                    <div>
                        <input type="password" name="password" placeholder="Twoje hasło" value="">
                    </div>
                        <button type="submit">Zaloguj się</button>
                   </form>';
        }
?>
[PHP] pobierz, plaintext 

Skrypt działa dobrze. Przy pierwszym wejściu na stronę przechodzimy na koniec kodu do formularza, ponieważ zmienna login, password i session nie istnieją.
Jeżeli wyślemy login, hasło i skrypt sprawdzi, że nie są puste to jest porównywany hash z tym znajdującym się w bazie. Dalej jeśli się zgadzają jest tworzona zmienna sesyjna, w której jest id, czas i nazwa użytkownika. W drugiej części skrypt sprawdza, że istnieje sesja między innymi z id użytkownika i wyświetla dane przeznaczone tylko dla niego.

Problem w tym, że jeżeli nie znam loginu ani hasła użytkownika to też mogę się dostać do danych przeznaczonych tylko dla danego użytkownika. Wystarczy, że w sposób "sztuczny" utworzę zmienną sesyjną z id i zmieniając tylko id mogę sobie skakać z konta na konto.

Pytanie jak zabezpieczyć taki skrypt?

PS: Trafiłem przypadkiem na ten błąd robiąc kopię tego skryptu, ale z podpiętą inną bazą innym loginem i hasłem. Odpaliłem jeden skrypt podałem login i hasło, a potem nie zamykając przeglądarki odpaliłem drugi skrypt z całkiem inną bazą loginem i hasłem w niej (tylko id użytkownika się zgadzało) i byłem zalogowany na obu skryptach. Mogłem sobie zmieniać id użytkownika w jednym skrypcie, a w drugim byłem zalogowany na tym użytkowniku o danym id. Wnioskuję więc, że każdy mógłby wysłać zmienną sesyjną z id i by został wpuszczony i by mu się wyświetliły dane tego użytkownika.

PS2: Po linii 39 następuje sprawdzenie czy została utworzona zmienna sesyjna między innymi z id użytkownika, a w linii 46 jest select który pobiera dane przeznaczone dla użytkownika o tym id. I właśnie tu jest luka, którą nie wiem jak załatać, jedyne co mi przychodzi do głowy to ponowne sprawdzenie czy nazwa użytkownika i hasło się zgadzają, ale wtedy jaki sens ma kod przed linią 39? Mam nadzieję, że jasno to opisałem W skrócie macie 2 kopie tego skryptu logujecie się na jednej to w drugiej jesteście zalogowani mimo, że hasła użytkownika w bazie podpiętej do drugiego skrypcie nie znacie. Prościej nie umiem

Ten post edytował Baku12345 4.03.2017, 21:40:30

[Damonsson]

Przypisuj cookie do konkretnej domeny, tyle. A jak masz separację pod tą samą domeną, to dodaj do cookie adres i sprawdzaj czy się zgadza. Zapewni to separacje pomiędzy projektami. Zwiększy jakoś tam bezpieczeństwo, że pomiędzy skryptami nie będzie można edytować sesji.

Ale oczywiście nikt nie może zmienić sobie ID, bo jest pobierane z bazy danych. Pytanie w stylu: "Mam hasło do mojej witryny i mogę się na nią włamać dzięki temu. Jak zabezpieczyć skrypt?"

Ten post edytował Damonsson 4.03.2017, 22:34:14

[JakubBab]

Co masz na myśli pisząc sztucznie? Ktoś musiałby się dobrać do Twojego kompa żeby ukraść to id (albo do konta użytkownika który korzysta z Twojej ulsugi i podrkaść wartość sesyjną). Może to zrobić np. korzystając z public network a jednym z użytkowników jest niegrzeczny Janusz który nasłuchuje na Twój traffic i kradnie info! evil bitch!

Jak się zabezpieczyć? Ciężko znaleść złoty środek ale możesz spróować poprzez sprwadzanie IP naprzykład.


Takie masz sessions data?

[PHP] pobierz, plaintext 
$_SESSION[$session_data] = array('id' => $row['id'], 'time' => $now, 'user' => $row['name']);
[PHP] pobierz, plaintext 

Wez sobie to md5 sformatuj lub crypt(), a później porównuj np:

http://php.net/hash_equals

Po drugie:

Nie wszystko w jednym pliku. Trzymasz połączenie z bazą, logikę i prezentacje danych w jednym miejscu. Właśnie uśmierciłeś małęgo pingwinka.

Tak naprawde wystarczy użyć empty() , bez isset()

[Baku12345]

W sumie faktycznie dodanie adresu ip lub domeny mogłoby trochę pomóc. Co do hashowania tej linii

[PHP] pobierz, plaintext 
$_SESSION[$session_data] = array('id' => $row['id'], 'time' => $now, 'user' => $row['name']);
[PHP] pobierz, plaintext 

to byłby to dobry pomysł, gdybym w linii 46 nie pobierał danych po id użytkownika

[PHP] pobierz, plaintext 
$stmt = $pdo->prepare("SELECT * FROM tabela WHERE user_id = :user_id ORDER BY ... ASC");
$stmt->execute(array(':user_id' => $_SESSION[$session_data]['id']));
[PHP] pobierz, plaintext 

Tak więc muszę mieć tu czyste id bez hashowania.

Co do pytania co mam na myśli pisząc sztucznie to mam na myśli stworzenie sesji przed wejściem na stronę. Wystarczy, że stworzę sobie plik atak.php i umieszczę w nim to

[PHP] pobierz, plaintext 
<?php
    session_start();
 
    $now = time();
    $session_data = "";
 
    $_SESSION[$session_data] = array('id' => 1, 'time' => $now);
?>
[PHP] pobierz, plaintext 

Jeśli go otworzę w przeglądarce, to stworzy się sesja z id użytkownika 1 i aktualnym czasem. Teraz jak otworzę plik index.php (ten co go pokazałem w pierwszym poście, to nawet się nie będę musiał logować na użytkownika o id 1. Będzie zalogowany bez podawania loginu i hasła I tu jest problem.

Ten post edytował Baku12345 5.03.2017, 03:51:49

[viking]

A teraz się zastanów ile jest możliwych kombinacji przy tej ilości znaków nazwy sesji.
Oczywiście mógłby dojść do powtórzenia ale szanse na to nikłe.
Po co to addslashes? Raczej fitruj poprzez sprawdzanie dopuszczalnych znaków.

[daro0]

Postaram się tutaj przedstawić to jakoś w miarę klarownie, bo na podstawie wielu postów na różnych forach widać jak na dłoni, że jak się ktoś czegoś uczy to jeszcze w starym stylu i pewnie też z jakichś beznadziejnych i niepraktycznych tutoriali PHP które są jeszcze w internecie.

Po pierwsze, podział kodu i podział odpowiedzialności, dla uproszczenia zakładam że wszystkie pliki PHP są w jednym katalogu projektu, lecę więc po kolei, od helperów, przez szablony aż po inne pliki. Uwaga, zakładam że liczą się duże i małe litery w nazwach plików z klasami.

Klasa View.php odpowiedzialna za renderowanie szablonów:

[PHP] pobierz, plaintext 
<?php
 
defined('DOCROOT') OR die('No direct script access.');
 
class View
{
    private $_file;
    private $_data;
 
    public static function factory($file, array $data)
    {
        return new View($file, $data);
    }
 
    public function __construct($file, array $data)
    {
        $this->_file = DOCROOT . $file . '.php';
        $this->_data = $data;
    }
 
    public function render()
    {
        extract($this->_data);
	ob_start();
	include $this->_file;
	return ob_get_clean();
    }
 
    public function __toString()
    {
        return $this->render();
    }
 
}
[PHP] pobierz, plaintext 

Klasa Auth.php odpowiedzialna za logowanie i sprawdzanie czy użytkownik jest zalogowany:

[PHP] pobierz, plaintext 
<?php
 
defined('DOCROOT') OR die('No direct script access.');
 
class Auth
{
    private $_config;
    private $_db;
    private $_session_key;
 
    public function __construct($config)
    {
        $this->_config = $config;
        $dsn = 'mysql:host=' . $config['database']['host'] . '; port='.$config['database']['port'].'; dbname=' . $config['database']['database'];
        $this->_db = $pdo = new PDO($dsn, $config['database']['username'], $config['database']['password']);
        $this->_session_key = $config['auth']['session_key'];
        session_start();
    }
 
    public function get_user()
    {
        $now = time();
        $max_idle_time = $this->_config['auth']['max_idle_time'];
 
        if (array_key_exists($this->_session_key, $_SESSION))
        {
            if (($now - $_SESSION[$this->_session_key]['last_active'] < $max_idle_time))
            {
                return $_SESSION[$this->_session_key]['user'];
            }
            return NULL;
        }
 
        return NULL;
    }
 
    public function logged_in()
    {
        return ($this->get_user() !== NULL);
    }
 
    public function login($username, $password)
    {
        $password = hash_hmac('sha256', $password, $this->_config['auth']['hash_key']);
        $stmt = $this->_db->prepare('SELECT * FROM users WHERE username = :username AND password = :password');
        $stmt->execute(array(':username' => $username, ':password' => $password));
 
        if ($row = $stmt->fetch(PDO::FETCH_ASSOC))
        {
            $_SESSION[$this->_session_key] = array(
                'user' => $row,
                'last_active' => time(),
            );
 
            return true;
        }
        return false;
    }
}
[PHP] pobierz, plaintext 

plik config.php z danymi do połączenia z bazą i danymi do obsługi logowania:

[PHP] pobierz, plaintext 
<?php
 
return array(
    'database' => array(
        'host' => 'localhost',
        'port' => 3306,
        'database' => 'testdb',
        'username' => 'root',
        'password' => 'root',
    ),
    'auth' => array(
        'session_key' => 'auth_user',
        'hash_key' => '213dsklsdks32000',
        'max_idle_time' => 60,
    ),
);
[PHP] pobierz, plaintext 

główny layout, plik layout.php

[PHP] pobierz, plaintext 
<html lang="pl">
    <head>
        <meta charset="utf-8" />
    </head>
    <body>
        <?php if (isset($content)) echo $content; ?>
    </body>
</html>
[PHP] pobierz, plaintext 

szablon logowania login.php

[PHP] pobierz, plaintext 
<?php if ($user): ?>
<H1><?= 'Jesteś zalogowany(a) jako: ' . $user['username'] ?></H1>
<script>
function reloadAsGet()
{
    var loc = window.location;
    window.location = loc.protocol + '//' + loc.host + loc.pathname + loc.search;
}
 
setTimeout(function(){
    reloadAsGet();
}, <?= $reload_timeout * 1000 ?>);
</script>
<?php else: ?>
    <h1>Logowanie</h1>
    <form action="" method="POST">
        <div>
            <input type="text" name="username" value="">
        </div>
        <div>
            <input type="password" name="password" value="">
        </div>
        <button type="submit">Zaloguj</button>
    </form>
<?php endif; ?>
<h3 id="message"><?= $message ?></h3>
[PHP] pobierz, plaintext 

no i ostatecznie index.php

[PHP] pobierz, plaintext 
<?php
define('DOCROOT', realpath(dirname(__FILE__)).DIRECTORY_SEPARATOR);
 
spl_autoload_extensions('.php');
spl_autoload_register();
 
$config = include 'config.php';
$auth = new Auth($config);
$message = '';
$reload_timeout = $config['auth']['max_idle_time'];
 
if ($_SERVER['REQUEST_METHOD'] === 'POST')
{
    $result = $auth->login($_POST['username'], $_POST['password']);
    if (!$result)
    {
        $message = 'Błąd logowania';
    }
}
 
$user = $auth->get_user();
 
$content = View::factory('login', compact('user', 'message', 'reload_timeout'));
$layout    = View::factory('layout', compact('content'));
 
echo $layout;
[PHP] pobierz, plaintext 

Co też testowałem na jednej ze swoich baz MySQL na localhoście, nie wiem tylko czy ta funkcja reloadAsGet zadziała na wszystkich przeglądarkach. W tym przykładzie po ustawionym czasie bezczynności 1 minuty po zalogowaniu nastąpi po tym czasie przeładowanie strony nie jako POST ale GET ze sprawdzeniem czy użytkownik jest zalogowany, jeśli nie to się wyświetli ten formularz.

Po drugie. Jest chyba dość cienka granica między bezpieczeństwem a paranoją. Tu zakładam że w każdym innym projekcie są różne te hash_key i zakładam że jest SHA256 i 64 znaków w polach z hasłem. Ten automatyczny reload to w pewnych przypadkach może pewnie nawet przeszkadzać niż pomagać, w szczególności może denerwować.

I nie jakieś przypisywanie bezpośrednio do sesji a tutaj użyty jest klucz auth_user związany z tą właśnie obsługą logowania a ze względów praktycznych do tejże sesji zapisywany jest cały wiersz z bazy danych a nie tylko niektóre pola, nie muszę chyba tłumaczyć dlaczego.