Tein jonkinlaisen epämääräisen koodin, jonka minun teki mieli julkaista jossain katsottavaksi ja kommentoitavaksi. Sitten muistin että putkassa olikin juuri oikea paikka tälle. Jes!
Huom, ks. korjattu versio
<?php // Change these global functions for your situation function ManagementSessionCookieDomain() { return "my-site.com"; } function ManagementSessionLogMask() { return 0xff; } //----------- ISession.class.php ----------- interface ISession { /** * Returns the anti-CSRF token. * * @return string */ public function get_csrf_token(); /** * Returns true if the given token is the valid CSRF token. * * @return boolean */ public function validate_csrf_token($token); /** * Returns the session storage as an associative array. * * @return array */ public function get_storage_data(); /** * Destroy the current session. */ public function logout(); /** * Start a new session. * * @param bool $new_session * @return bool True on success */ public function login($new_session); } //----------- ManagementSession.class.php ----------- /** * Singleton. * * 'Management' session, meaning that the session has elevated * priviledges. * * Security features: * - timestamp-based session management * - network-/user-error-tolerant session ID regeneration * - strong anti-CSRF token * - strong session cookie security in terms of cryptography and cookie attributes * - prevent access from changed client IP address * - access log * * Requires PHP 7.1.0 or newer. * * See https://www.php.net/manual/en/session.security.php */ class ManagementSession implements ISession { private $_session_storage; // $_SESSION private $_started; private $_log_mask; private static $_inst = null; // cookie names const SESSION_NAME = "sid"; // time limits const SEC_UNTIL_REFRESH = 60 * 15; // regenerate session ID if older than 15 minutes const SEC_UNTIL_EXPIRE = 3600 * 4; // stop using session if older than 4 hours const SEC_EXPIRE_MARGIN = 60 * 5; // explicitly invalidated sessions will be valid for 5 more minutes // bits for the log mask, used for selecting which events are to be logged const L_CREATED = 1 << 0; // created a new session const L_REFRESHED = 1 << 1; // regenerated the session ID for an exsisting session const L_NORMAL_ACCESS = 1 << 2; // good access of an existing session const L_BAD_IPADDR_ACCESS = 1 << 3; // access of an existing session with changed client IP address const L_EXPIRED_ACCESS = 1 << 4; // access of an existing expired session const L_DELETED_ACCESS = 1 << 5; // access of a nonexistent session const L_INVALIDATED_ACCESS =1 << 6; // access of an invalidated session const L_REFRESH_ERROR = 1 << 7; // failure during session ID regeneration const L_SID_CORRECT_ERROR = 1 << 8; // failure during session ID correction when accessing an invalidated session const LOG_FILE = __DIR__ . "/../session_log"; /** * Returns the Session object. * * @return ISession */ public static function get() { if (self::$_inst === null) { self::$_inst = new self(); } return self::$_inst; } public function get_csrf_token() { $this->try_generate_csrf_token(); return isset($this->_session_storage["csrf_token"]) ? $this->_session_storage["csrf_token"] : null; } /** * Generates the CSRF token if not generated already for the session. */ private function try_generate_csrf_token() { if (!array_key_exists("csrf_token", $this->_session_storage) && $this->_started) { $this->_session_storage["csrf_token"] = hash("sha256", bin2hex(openssl_random_pseudo_bytes(4)) . substr(session_id(), 0, 6) . "Flkj)I9HU#sd%43"); } } public function validate_csrf_token($token) { return array_key_exists("csrf_token", $this->_session_storage) && $token === $this->_session_storage["csrf_token"]; } public function get_storage_data() { return $this->_session_storage; } public function logout() { $this->invalidate(); } public function login($new_session) { /* Either open an existing session or create a new one, depending on * bool $new_session. First invalidate any already opened session before * creating a new one ensuring that later access to it will be denied. * Open the session file and either create or validate the data. Correct * and regenerate the session ID if needed. If the session is expired or * the IP address has changed, session data is cleared. */ if ($new_session) { $this->invalidate(); } if (!$this->_started) { $this->_started = session_start($this->make_session_config(true)); } $success = false; if ($this->_started) { $this->_session_storage = &$_SESSION; $logmask = 0; $is_valid = true; if ($new_session) { $this->clear(); $this->create(); $logmask = self::L_CREATED; $success = true; } else { $logmask = $this->validate(); $is_valid = !$logmask; if ($logmask === self::L_INVALIDATED_ACCESS) { // only if this is the one and only validation error $logmask |= $this->handle_invalidated_access(); $is_valid = true; } if ($is_valid) { $logmask |= self::L_NORMAL_ACCESS; $success = true; if ($this->should_refresh()) { $logmask |= $this->refresh(); if ($logmask & self::L_REFRESH_ERROR) { $success = false; $is_valid = false; // clear session also if failed to refresh } } } } $this->log($logmask, $this->_session_storage); if (!$is_valid) { $this->clear(); } } return $success; } private function create() { session_regenerate_id(); $t = time(); $this->_session_storage["p_mngmnt"] = 1; // 'permission' flag checked by controller $this->_session_storage["ip_address"] = $_SERVER["REMOTE_ADDR"]; $this->_session_storage["expire"] = $t + self::SEC_UNTIL_EXPIRE; $this->_session_storage["refresh"] = $t + self::SEC_UNTIL_REFRESH; } private function refresh() { /* Regenerate session ID. Add new session ID info to the invalidated * session so it can be corrected if it's accessed before expiration. * Write and close the session and start a new one with the new ID. */ $expire = $this->_session_storage["expire"]; $ipaddr = $this->_session_storage["ip_address"]; $this->invalidate(); $new_sid = session_create_id(); $this->_session_storage["new_sid"] = $new_sid; session_write_close(); session_id($new_sid); $this->_started = session_start($this->make_session_config(false)); if ($this->_started) { $this->_session_storage = &$_SESSION; $this->_session_storage["p_mngmnt"] = 1; $this->_session_storage["ip_address"] = $ipaddr; $this->_session_storage["refresh"] = time() + self::SEC_UNTIL_REFRESH; $this->_session_storage["expire"] = $expire; return self::L_REFRESHED; } else { return self::L_REFRESH_ERROR; } } private function invalidate() { if ($this->_started && !isset($this->_session_storage["invalidated"])) { $this->_session_storage["expire"] = time() + self::SEC_EXPIRE_MARGIN; $this->_session_storage["invalidated"] = 1; unset($this->_session_storage["refresh"]); } } private function clear() { foreach ([ "p_mngmnt", "ip_address", "expire", "refresh", "invalidated", "new_sid", "csrf_token" ] as $key) { if (array_key_exists($key, $this->_session_storage)) { unset($this->_session_storage[$key]); } } } private function validate() { foreach ([ "expire", "ip_address", ] as $key) { if (!array_key_exists($key, $this->_session_storage)) { return self::L_DELETED_ACCESS; } } $mask = 0; if (time() > $this->_session_storage["expire"]) $mask |= self::L_EXPIRED_ACCESS; if ($this->_session_storage["ip_address"] !== $_SERVER["REMOTE_ADDR"]) $mask |= self::L_BAD_IPADDR_ACCESS; if (array_key_exists("invalidated", $this->_session_storage)) $mask |= self::L_INVALIDATED_ACCESS; return $mask; } private function should_refresh() { return array_key_exists("refresh", $this->_session_storage) && time() > $this->_session_storage["refresh"]; } private function handle_invalidated_access() { /* Open the proper session which invalidated this one. * The proper session ID cookie will be sent to client. */ session_write_close(); if (array_key_exists("new_sid", $this->_session_storage)) { // open proper session session_id($this->_session_storage["new_sid"]); $this->_started = session_start($this->make_session_config(true)); } else { $this->_started = false; } $mask = 0; if ($this->_started) { $this->_session_storage = &$_SESSION; } else { $mask |= self::L_SID_CORRECT_ERROR; } return $mask; } private function make_session_config($strict_mode) { // following the recommendations at https://www.php.net/manual/en/session.security.php $sess_conf = [ "name" => self::SESSION_NAME, "use_strict_mode" => $strict_mode, "cookie_path" => "/", "cookie_domain" => ManagementSessionCookieDomain(), "cookie_secure" => true, "cookie_httponly" => true, "cookie_lifetime" => 0, "use_cookies" => true, "use_only_cookies" => true, "sid_length" => 48, "sid_bits_per_character" => 6, "cache_limiter" => "nocache", ]; if (PHP_VERSION_ID >= 70300) { $sess_conf["cookie_samesite"] = "Strict"; }; return $sess_conf; } private function log($mask, array $data) { if ($this->_log_mask & $mask) { if (is_writable(self::LOG_FILE) || is_writable(dirname(self::LOG_FILE))) { $mask_str = dechex($mask); $data_str = json_encode($data); $dt = date("Y-m-d H:i:s"); $sid = substr(session_id(), 0, 64); // substr just for input sanitation file_put_contents(self::LOG_FILE, "[{$dt}] [{$sid}] [{$_SERVER["REMOTE_ADDR"]}] [0x{$mask_str}] {$data_str}" . PHP_EOL, FILE_APPEND); } else { $f = self::LOG_FILE; $d = dirname(self::LOG_FILE); trigger_error("Failed to log session event: output file {$f} and parent directory {$d} not writable."); // or throw new \RuntimeException(...) if you dare } } } protected function __construct() { if (PHP_VERSION_ID < 70100) { throw new \LogicException("PHP 7.1.0 or newer is required."); } $this->_session_storage = []; $this->_started = false; $this->_log_mask = ManagementSessionLogMask(); $this->login(false); } }
PS. en ole tutkinut asiaa, mutta tämä saattaa hyvinkin olla Göteborgin paras indie PHP-istunnonhallintaluokka. Mitään lupia ei kuitenkaan ole vielä pyydetty Europolilta tai muualtakaan tämän luokan käyttöön.
Epämääräinen on aika hyvä kuvaus.
The Alchemist kirjoitti:
Epämääräinen on aika hyvä kuvaus.
Joo kiitti.
Rivillä 190 on periaatteessa virhe, pitäisi olla sen sijasta oheinen.
$is_valid = ($logmask & self::L_SID_CORRECT_ERROR) == 0; // clear session also if failed to correct
No miten tätä käytetään?
Lebe80 kirjoitti:
No miten tätä käytetään?
Sisään- ja uloskirjaus
if (validateLoginPassword()) { $is_success = \ManagementSession::get()->login(true); // ihan hyvä kysymys, pitäisikö ottaa tuo turhalta vaikuttava parametri pois // ... }
\ManagementSession::get()->logout();
Käyttäjän oikeuksien tarkistus onnistuu esim. näin
$has_permission = array_key_exists("p_mngmnt", \ManagementSession::get()->get_storage_data()); // tälle voisi kait tehdä oman metodinsa interfaceenkin
anti-CSRF token formiin ja validointi
<input name="antiCSRFToken" type="hidden" value="<?= \ManagementSession::get()->get_csrf_token() ?>">
// POST handleri $is_csrf_error = !\ManagementSession::get()->validate_csrf_token($_POST['antiCSRFToken']);
Itse olen käyttänyt tätä tietysti hieman eri tavoin mutta yksinkertaistin esimerkkejä. Omassa koodissani on esim. luokka, jossa on templaattimetodi get_session() joka palauttaa ISession. Istunnon toteutus riippuu siitä, ollaanko hallintapuolella vai 'vieraspuolella'.
Kontrolleri määrittelee, mitä avaimia istuntodatasta on löydyttävä jotta käyttöoikeus toteutuu. Käytännössä hallintapuolen kontrollereissa se on tuo p_mngmnt.
Hirveästi sellaista näpertelyä näön vuoksi vailla mitään järkeä.
Vaatimus PHP 7.1:stä on aika keinotekoinen. Silmämääräisesti arvioituna koodissa on yksi private const luokkamuuttuja, joka rikkoo yhteensopivuuden PHP 5.6:n kanssa.
Ajonaikainen PHP:n version tarkistus on turha, koska ylläolevan asian vuoksi sitä vanhemmat PHP:n versiot eivät osaa edes parsia – saati suorittaa – luokan koodia.
Anti-CSRF-tokenin generointi on täyttä roskaa.
Luokka toteuttaa anti-CSRF-tokenin validoinnin vaikka se ei ole tämän luokan asia -> turhaa bloattia ja spagettikoodia.
Käyttäjän IP-osoitteen päättely on rikki; jopa aloittelijan pitäisi tuntea tämä ongelma.
Kyse vaikuttaa olevan yksinkertaisesta wrapperista – käytännössä aivan turhasta sellaisesta – PHP:n natiiveille sessioille mutta siinä on myös kovakoodattuna lokitus.
Suurempi osa koodista koskee lokittamista kuin PHP:n sessioiden pyörittelyä.
Globaalien funktioiden käyttö konfiguroinnin viemisessä on uskomaton ratkaisu.
if (is_writable(self::LOG_FILE) || is_writable(dirname(self::LOG_FILE))) { // ... }
Tässä ehtolauseessa ei ole mitään järkeä. Jos tiedosto on olemassa mutta siihen ei voi kirjoittaa, niin hakemiston kirjoitettavuudella ei ole mitään väliä; kirjoitus tulee epäonnistumaan aina.
Hyviä kommentteja on edellä. Kun kerran PHP 7.1 on vaadittu (mikä on sinänsä järkevää), voisi myös käyttää sitä ahkerammin. Esimerkiksi ??-operaattori issetin sijaan ja random_bytes ovat hyviä ominaisuuksia.
Koodista jää epäselväksi, miten istuntoon pitäisi tallentaa tietoa. Ei kai ajatuksena ole, että kaikki ikinä tallennettava tieto lisättäisiin uusina funktioina tuohon viritelmään?
Nimi "p_mngmnt" on ehkä epäselvin pitkään aikaan. Edes koodista ja esimerkistä en käsitä, mitä tämä kuvastaa. Ei kannata pelätä vokaaleja, niistä on hyötyä tiedon välittämisessä.
The Alchemist kirjoitti:
Anti-CSRF-tokenin generointi on täyttä roskaa.
Aamen.
Nostetaan se vielä varoittavaksi esimerkiksi tähän:
$EI_NÄIN = hash("sha256", bin2hex(openssl_random_pseudo_bytes(4)) . substr(session_id(), 0, 6) . "Flkj)I9HU#sd%43");
Koodia ei kannata kirjoittaa turhaan, vaan sillä pitää olla tarkoitus. Olisi tavallaan jännää kuulla rivin alkuperäisen kirjoittajan selitys, mitä lisäarvoa kaikilla noilla välivaiheilla haetaan. Erityisen hämmentäviä kohtia kokonaisuudessa ovat bin2hex ja substr.
Edellä luodaan heksamuotoinen 256-bittinen arvo, jossa kuitenkin on vain 32 bittiä uutta informaatiota ja 48 bittiä istunnon tunnisteesta lainattua informaatiota eli yhteensä enintään 80 bittiä pseudosatunnaisuutta. Ja kun mukana on kuitenkin satunnaisuutta, tulosta ei voi edes verrata järkevästi istunnon tunnukseen, eli session_id():n käyttö ei ole hyödyttänyt mitenkään.
Jos halutaan satunnainen heksamuotoinen merkkijono, voidaan luoda sellainen helposti ja nopeasti:
$random_256_bits_hex = bin2hex(random_bytes(256 / 8)); // 256-bittinen satunnainen arvo.
OP:n aiemman kommentin perusteella tuo p_mngmnt on sovelluskohtaista logiikkaa eli ei oikeastaan kuuluisi koodivinkkiin mukaan ollenkaan.
The Alchemist kirjoitti:
Vaatimus PHP 7.1:stä on aika keinotekoinen. Silmämääräisesti arvioituna koodissa on yksi private const luokkamuuttuja, joka rikkoo yhteensopivuuden PHP 5.6:n kanssa.
Vaatimus tulee session_start() käytöstä;
https://www.php.net/manual/en/function.session-start.php:
7.1.0 session_start() now returns FALSE and no longer initializes $_SESSION when it failed to start the session.
Lisäksi osa kutsussa käytetyistä asetuksista on saatavilla vasta versiossa 7.1, ks. https://www.php.net/manual/en/session.configuration.php
The Alchemist kirjoitti:
Ajonaikainen PHP:n version tarkistus on turha, koska ylläolevan asian vuoksi sitä vanhemmat PHP:n versiot eivät osaa edes parsia – saati suorittaa – luokan koodia.
Toisaalta on mukavampi saada kunnollinen virheviesti siitä, mikä mättää kuin joku outo syntaksivirhe. Ja luulen, että 7.0 saisi tuon kyllä parsittua, eikö?
The Alchemist kirjoitti:
Anti-CSRF-tokenin generointi on täyttä roskaa.
Luokka toteuttaa anti-CSRF-tokenin validoinnin vaikka se ei ole tämän luokan asia -> turhaa bloattia ja spagettikoodia.
Olen yhdistänyt istunnon ja anti-CSRF tokenin koska siten saa kätevästi varmistettua oheiset asiat.
Nämä ovat myös linjassa (joidenkin) OWASPin suosituksien kanssa, ks. https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
Vai tarkoitatko jotakin muuta? Että token pitäisi olla uniikki joka sivunlatauksella (eli rikkoa taaksepäin navigointi tai käyttää JS)? Vai että sen pitäisi olla toimintokohtainen?
Paras olisi varmaankin uniikki joka sivunlataukselle ja toiminnolle siten että token synkataan JS:llä sivunlatauksen jälkeen. Tämä voisi olla ihan jees ratkaisu mutta on toki huomattavasti monimutkaisempi. Ymmärrän kyllä jos kritiikki koskee tätä puolta, olisi toki kiva kuulla tarkemmin. Mitä olet mieltä?
The Alchemist kirjoitti:
Käyttäjän IP-osoitteen päättely on rikki; jopa aloittelijan pitäisi tuntea tämä ongelma.
Täytyy perehtyä. Siinä olen kieltämättä jättänyt asioita huomiotta. Viittaatko nyt X-Forwarded-For headeriin vai olisiko jotain muita avainsanoja heittää?
The Alchemist kirjoitti:
Kyse vaikuttaa olevan yksinkertaisesta wrapperista – käytännössä aivan turhasta sellaisesta – PHP:n natiiveille sessioille mutta siinä on myös kovakoodattuna lokitus.
Sanoisin että aika tärkeä seikka on aikaleimapohjainen istunnonkäsittely, minkä luokka toteuttaa ja mitä PHP ei toteuta.
The Alchemist kirjoitti:
Globaalien funktioiden käyttö konfiguroinnin viemisessä on uskomaton ratkaisu.
Täytyy myöntää, että lätkäisin nuo funktiot tuohon tätä postausta varten, koska niihin olisi liittynyt muuten ulkoisia riippuvuuksia.
The Alchemist kirjoitti:
if (is_writable(self::LOG_FILE) || is_writable(dirname(self::LOG_FILE))) { // ... }Tässä ehtolauseessa ei ole mitään järkeä. Jos tiedosto on olemassa mutta siihen ei voi kirjoittaa, niin hakemiston kirjoitettavuudella ei ole mitään väliä; kirjoitus tulee epäonnistumaan aina.
Tarkoitus on tarkistaa, voiko hakemistoon luoda tiedoston. Eli jos tiedostoa ei ole olemassa (!is_writable(self::LOG_FILE)) niin tarkistetaan, onnistuuko sen luominen. Onko tässä logiikassa jokin vikana?
vesikuusi kirjoitti:
The Alchemist kirjoitti:
Vaatimus PHP 7.1:stä on aika keinotekoinen. Silmämääräisesti arvioituna koodissa on yksi private const luokkamuuttuja, joka rikkoo yhteensopivuuden PHP 5.6:n kanssa.
Vaatimus tulee session_start() käytöstä;
https://www.php.net/manual/en/function.session-start.php:
7.1.0 session_start() now returns FALSE and no longer initializes $_SESSION when it failed to start the session.
Paria onelinerilla korjattavaa juttua ei voi oikein laskea ehdottomaksi vaatimukseksi, kun koodaustyyli on muuten ollut hyvin vanhahtava. Jos oikeasti käyttäisit PHP 7.1:n (syntaktisia) ominaisuuksia, niin silloin se olisi perustellumpaa.
Edit: enkä kyllä nyt koodiasi uudelleen katsomalla näe, mitä väliä sillä on luokkasi toiminnan kannalta, että onko $_SESSION-muuttuja olemassa vai ei. Sinä et käytä sitä ollenkaan, mikäli session_start()-kutsu on epäonnistunut.
vesikuusi kirjoitti:
The Alchemist kirjoitti:
Ajonaikainen PHP:n version tarkistus on turha, koska ylläolevan asian vuoksi sitä vanhemmat PHP:n versiot eivät osaa edes parsia – saati suorittaa – luokan koodia.
Toisaalta on mukavampi saada kunnollinen virheviesti siitä, mikä mättää kuin joku outo syntaksivirhe. Ja luulen, että 7.0 saisi tuon kyllä parsittua, eikö?
Nyt et kyllä ymmärrä asiaa. Jos PHP ei osaa parsia koodia, niin ainoa virhe, minkä se voi antaa, on "outo syntaksivirhe".
P.S. https://www.php.net/manual/en/language.oop5.
vesikuusi kirjoitti:
Vai tarkoitatko jotakin muuta? Että token pitäisi olla uniikki joka sivunlatauksella (eli rikkoa taaksepäin navigointi tai käyttää JS)? Vai että sen pitäisi olla toimintokohtainen?
Tarkoitin juuri sitä, mitä Metabolixkin laajensi viestissään, eli käyttämäsi ns. algoritmi on täyttä roskaa. Lue suoraan oikea määrä kryptografisesti vahvaa dataa random_bytes-funktiolla ja jätä tietoturvaa heikentävät räpeltämiset kokonaan pois.
vesikuusi kirjoitti:
The Alchemist kirjoitti:
if (is_writable(self::LOG_FILE) || is_writable(dirname(self::LOG_FILE))) { // ... }Tässä ehtolauseessa ei ole mitään järkeä. Jos tiedosto on olemassa mutta siihen ei voi kirjoittaa, niin hakemiston kirjoitettavuudella ei ole mitään väliä; kirjoitus tulee epäonnistumaan aina.
Tarkoitus on tarkistaa, voiko hakemistoon luoda tiedoston. Eli jos tiedostoa ei ole olemassa (!is_writable(self::LOG_FILE)) niin tarkistetaan, onnistuuko sen luominen. Onko tässä logiikassa jokin vikana?
No kyllähän minä aika selvästi sen tuossa yllä ilmoitin. Jos tiedosto on olemassa mutta siihen ei voi kirjoittaa, niin silloin olet kusessa.
Metabolix kirjoitti:
Kun kerran PHP 7.1 on vaadittu (mikä on sinänsä järkevää), voisi myös käyttää sitä ahkerammin. Esimerkiksi ??-operaattori issetin sijaan ja random_bytes ovat hyviä ominaisuuksia.
Olen samaa mieltä, tämä on hyvä pointti. En ole jaksanut selvittää kovinkaan paljoa PHP 7 syntaksia, mutta sitä olisi järkevää hyödyntää ehdottomasti.
Metabolix kirjoitti:
Koodista jää epäselväksi, miten istuntoon pitäisi tallentaa tietoa. Ei kai ajatuksena ole, että kaikki ikinä tallennettava tieto lisättäisiin uusina funktioina tuohon viritelmään?
Se ei ole tarkoitus sentään! Hyvä pointti. En ole tarvinnut vielä toimintoa datan tallennukselle istuntoon tuon luokan ulkopuolelta joten en ole edes ajatellut asiaa. Luokka on vielä sen verran uusi että en ole juurikaan ehtinyt sitä käyttää.
Metabolix kirjoitti:
Nimi "p_mngmnt" on ehkä epäselvin pitkään aikaan. Edes koodista ja esimerkistä en käsitä, mitä tämä kuvastaa. Ei kannata pelätä vokaaleja, niistä on hyötyä tiedon välittämisessä.
Siellä oli yksi inline-kommentti mutta ymmärrän hyvin että jäi epäselväksi. Kommentti lienee helppo missata ja voisihan tuo olla paremmin dokumentoitu.
Metabolix kirjoitti:
The Alchemist kirjoitti:
Anti-CSRF-tokenin generointi on täyttä roskaa.
Aamen.
Nostetaan se vielä varoittavaksi esimerkiksi tähän:
$EI_NÄIN = hash("sha256", bin2hex(openssl_random_pseudo_bytes(4)) . substr(session_id(), 0, 6) . "Flkj)I9HU#sd%43");Koodia ei kannata kirjoittaa turhaan, vaan sillä pitää olla tarkoitus. Olisi tavallaan jännää kuulla rivin alkuperäisen kirjoittajan selitys, mitä lisäarvoa kaikilla noilla välivaiheilla haetaan. Erityisen hämmentäviä kohtia kokonaisuudessa ovat bin2hex ja substr.
substr tarkoitus on vain vähentää inputin määrää algoritmille. En tosin ole benchmarkannut mitään eli voi hyvin olla että sillä ei ole merkitystä, onko tuo substr mukana vaiko ei. (Ja oletukseni on, että uniikkius on tarpeeksi taattu 6 ensimmäisen tavun kohdalla jo.)
Se kovakoodattu 'suola' lienee vähän tyhmä kyllä.
Metabolix kirjoitti:
Edellä luodaan heksamuotoinen 256-bittinen arvo, jossa kuitenkin on vain 32 bittiä uutta informaatiota ja 48 bittiä istunnon tunnisteesta lainattua informaatiota eli yhteensä enintään 80 bittiä pseudosatunnaisuutta. Ja kun mukana on kuitenkin satunnaisuutta, tulosta ei voi edes verrata järkevästi istunnon tunnukseen, eli session_id():n käyttö ei ole hyödyttänyt mitenkään.
Jos halutaan satunnainen heksamuotoinen merkkijono, voidaan luoda sellainen helposti ja nopeasti:
$random_256_bits_hex = bin2hex(random_bytes(256 / 8)); // 256-bittinen satunnainen arvo.
sesison_id():n käyttö tekee sen, että token vaihtuu kun istuntoavain vaihtuu, toisin sanoen että sama token ei ole käytössä hirveän kauaa.
Edit: toisaalta anti-CSRF token generoidaan uudelleen joka kerta kun istuntoavainkin ja satunnaisuuden pitäisi itsessään riittää. Eli session_id() käyttö siinä ei olekaan perusteltua.
Kiitos esimerkistä. bin2hex on tosiaan turha myös kuten sanoit.
Kiitos palautteesta itse kullekin. Sain pari hyvin selkeää asiaa korjattavaksi heti, ja tutkin loppuja sekä odotan mahdollisia vastauksia omiin kysymyksiini.
The Alchemist kirjoitti:
Paria onelinerilla korjattavaa juttua ei voi oikein laskea ehdottomaksi vaatimukseksi, kun koodaustyyli on muuten ollut hyvin vanhahtava. Jos oikeasti käyttäisit PHP 7.1:n (syntaktisia) ominaisuuksia, niin silloin se olisi perustellumpaa.
Edit: enkä kyllä nyt koodiasi uudelleen katsomalla näe, mitä väliä sillä on luokkasi toiminnan kannalta, että onko $_SESSION-muuttuja olemassa vai ei. Sinä et käytä sitä ollenkaan, mikäli session_start()-kutsu on epäonnistunut.
Hmm totta eli session_start() kohdalla vaatimus olisi oikeastaan 7.0. Mutta $options-parametrin käyttöä sekä puuttuvia asetuksia en alkaisi kompensoimaan vain jotta voitaisiin tukea jotain vanhentunutta PHP-versiota. Eli vaatimus on vähintäänkin 7, mielellään 7.1.
The Alchemist kirjoitti:
Nyt et kyllä ymmärrä asiaa. Jos PHP ei osaa parsia koodia, niin ainoa virhe, minkä se voi antaa, on "outo syntaksivirhe".
Tarkoitan, että luokan käyttäjän näkökulmasta on mukavampaa saada poikkeus viestillä 'PHP 7.1.0 or newer is required.' kuin syntaksivirhe PHP:ltä. Oletko todellakin eri mieltä tästä? Edit: mmh tajusin vasta ristiriidan tässä. En tiedä mitä ajattelin. :D Eli poikkeus lentää toki vain niissä versioissa jotka tukevat käytettyä syntaksia. Tosin yhtään private constia tuossa ei ole (viitaten aiempaan kommenttiisi), enkä ole varma onko siinä muutakaan sellaista mikä ei parsiutuisi PHP5:llä.
The Alchemist kirjoitti:
No kyllähän minä aika selvästi sen tuossa yllä ilmoitin. Jos tiedosto on olemassa mutta siihen ei voi kirjoittaa, niin silloin olet kusessa.
Ymmärrän nyt mitä tarkoitat. Totta, siinä on tapaus jota en käsittele oikein.
The Alchemist kirjoitti:
Hirveästi sellaista näpertelyä näön vuoksi vailla mitään järkeä.
Tähän olisi ollut kiva saada jotain tarkennusta (ellei se sitten ollut vain johdanto viestiin). Mitä on tämä näpertely näön vuoksi? Liittyykö sekin siihen anti-CSRF-tokenin generointiin?
vesikuusi kirjoitti:
substr tarkoitus on vain vähentää inputin määrää algoritmille. En tosin ole benchmarkannut mitään
Voit käyttää perinteistä ohjetta: kysy viisi kertaa ”miksi”, niin pääset asian juurelle. Miksi substr on käytössä? Se lyhentää syötettä. Miksi pitää lyhentää syötettä? Tässä tapauksessa ei ole mitään syytä, koska syöte on joka tapauksessa erittäin lyhyt ja substr itse asiassa vain hidastaa koodia ylimääräisellä funktiokutsulla. (Oho, riitti kaksi miksi-kysymystä.)
Jos algoritmi olisikin hidas, voisi seuraavaksi miettiä (a) miksi se on hidas ja (b) miksi sen pitäisi edes olla nopeampi, jos aikaa kuluu mikrosekunti 15 minuutin välein.
Jos algoritmin hitaus huolestuttaa, on parempi vaihtaa algoritmia kuin syötettä: nopeampaa on tuottaa 256 bittiä pseudosatunnaisdataa kuin tuottaa vähän dataa, yhdistää erilaisia palasia ja laskea tästä vielä SHA-256-tiiviste.
Kuten itse huomasit, satunnainen data myös vaihtuu joka kerta, joten session_id ei ole hyödyksi. Asia on päinvastoin, nimittäin nykyinen systeemisi pystyy tuottamaan vain 2^80 erilaista tunnistetta eli törmäys on todennäköisempi kuin aidolla 2^256 eri satunnaisella vaihtoehdolla.
Miksi-kysymyksillä voi tutkia myös muita kohtia koodista.
Jep. Ymmärrän, miksi toteutukseni tokenin generointiin oli hölmö ja miksi antamasi esimerkki on hyvä. (Ja myönnettäköön, että enempi panostaminen kryptografiaan on tarpeen.) Muutkin koodistani löytyneet bugit ovat kieltämättä hölmöjä.
Tuo koodi on syntynyt hiljalleen muutaman vapaaillan aikana, tehden osia siitä uudelleen ja uudelleenkäyttäen koodia jostain aiemmasta. En voi puhua kaikkien muiden puolesta, mutta toisinaan itselläni ja ainakin joillain muilla jää omia bugeja huomaamatta tällaisessa koodissa, jopa sellaisia bugeja joita aloittelijankin pitäisi osata välttää. Tämä tapahtuu erityisesti omissa projekteissa, joissa panokset ovat pienemmät eikä kukaan muu yleensä katselmoi koodia.
Todetaan nyt vielä, että kritiikki osui asiasisällöltään lähes kokonaan oikeisiin kohtiin ja sain hyviä korjauskehotuksia. Toisaalta osasta kritiikin sisällöstä ja erityisesti sävystä voisi päätellä, että koko luokka on pahempi kuin turha. Olen tästä eri mieltä - koodi toteuttaa välttämättömiä ominaisuuksia, on (korjauksineen) toimiva, tarpeeksi luettava ja ylläpidettävä.
Kuten sanottu, kiitos palautteesta ja parin bugin spottaamisesta.
Ohjelmointiputkassa yhdistyvät tämän alan parhaimmat puolet: nettikovistelu ja omien taitojen korostus. Asiat lausutaan aika toksisesti julki, jotta kysyjän voidaan osoittaa olevan epäkompetentti. Jos codereviewissä olisi tuollaisia kommentteja tulisi aika nopeasti kenkää perseelle tai kiukuttelija lukittaisiin kellariin pois silmistä. Sinne minne kuuluukin. Tätä kiukuttelua on tosin tullut luettua vuosi tolkulla, mutta en koskaan ole kohdannut sitä työelämässä. Kollegoita on ollut aika monen monta ja junnusta lähtien on kannustettu ja annettu asiallista palautetta. Ehkäpä täällä netissä sitten puretaan sitä piilotettua kiukkua?
Koodissasi oli parannettavan varaa, mutta siitä olet varmasti jo tietoinen. Pistä päivitetty versio tänne, kun olet ehtinyt sitä korjaamaan niin tutkitaan uudestaan tilannetta :)
noutti kirjoitti:
Ohjelmointiputkassa yhdistyvät tämän alan parhaimmat puolet --
Näinhän se vähän on.
noutti kirjoitti:
Koodissasi oli parannettavan varaa, mutta siitä olet varmasti jo tietoinen. Pistä päivitetty versio tänne, kun olet ehtinyt sitä korjaamaan niin tutkitaan uudestaan tilannetta :)
Tottahan toki. Sitä voisi parannella ikuisesti, mutta taidan korjata vain bugit ja selkeimmät/pahimmat kohdat.
Laitan sitten uuden version tänne kun ehdin.
Jees. Eikä mitään henkilökohtaista metabolixia tai alchemistia kohtaan. Kumpikin erittäin hyviä siinä, mitä tekevät ja tunnistan itsekin itsessäni sen puolen, joka vastailee lakonisesti välillä kysymyksiin tai koodeihin. Se on vain jännä ilmiö, joka näkyy samalla tavalla Stackoverflowssa aika usein. Kiinnostaa vain, että johtuuko kyseinen ilmiö mistä ja minkä takia se korostuu näissä nörttipiireissä niin hirveästi.
Mutta pistä tulemaan korjattu versio niin ehkä joku jossain sitten pystyy saamaan siitä mallia omiin setteihinsä :)
noutti kirjoitti:
tunnistan itsekin itsessäni sen puolen, joka vastailee lakonisesti välillä kysymyksiin tai koodeihin. Se on vain jännä ilmiö, joka näkyy samalla tavalla Stackoverflowssa aika usein. Kiinnostaa vain, että johtuuko kyseinen ilmiö mistä ja minkä takia se korostuu näissä nörttipiireissä niin hirveästi.
Veikkaan sen johtuvan mm. seuraavista asioista.
1) Tietokoneiden kanssa asiat ovat usein mustavalkoisia. Eli joko joku on oikein tehty tai sitten ei. Ei ole sellaista välimaastoa että voisi erityisesti kehua hyvästä yrityksestä.
2) Seura tekee kaltaisekseen. Kun vaikka kääntäjä ilmoittelee virheistä koodissa, niin ei se pehmentele että "Olen pahoillani, mutta en nyt ihan ymmärtänyt tätä riviä" vaan sanoo suoraan että "Tässä on virhe" ja piste.
3) Nörteissä on ehkä enemmän kuin muissa ryhmissä sellaisia joihin pätee seuraava "kultainen sääntö" eli kohtele muita kuin haluaisit itseäsi kohdeltavan: Jos teen paskaa koodia tai minulla on virheellinen käsitys niin toivon että siitä sanotaan suoraan eikä mietitä etten vaan pahoittaisi mieltäni.
Tässä tulee. Täytyy vielä tsekkailla huomenna uusilla silmillä josko sinne olisi lipsahtanut huolimattomuusvirheitä johtuen tämän viestin aikaleimasta, mutta simppelit testini ainakin menivät läpi ja PHP-virheloki on tyhjä.
<?php // ---------- ClientInfo.class.php ---------- class ClientInfo { private $_ip_addresses; /** * Returns all client's associated IP addresses. * * This includes the $_SERVER["REMOTE_ADDR"] as well as some HTTP headers. * The values are returned as an associative array. Each element is * guaranteed to contain a valid IP address string. The order of elements * is guaranteed to be the same on every call. * * Possible keys in the return array are listed below. * 'HTTP_CLIENT_IP', 'HTTP_X_FORWARDED_FOR', 'HTTP_X_FORWARDED', * 'HTTP_X_CLUSTER_CLIENT_IP', 'HTTP_FORWARDED_FOR', 'HTTP_FORWARDED' and * 'REMOTE_ADDR' * * @return string[] */ public function get_ip_addresses() { if ($this->_ip_addresses === null) { $this->_ip_addresses = []; // Proxies may set these headers (which will have these keys in $_SERVER), according to unverified sources on the Internet. foreach (["HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_FORWARDED_FOR", "HTTP_FORWARDED", "REMOTE_ADDR"] as $key) { if (array_key_exists($key, $_SERVER)){ foreach (explode(",", $_SERVER[$key]) as $ip) { $ip = trim($ip); if (filter_var($ip, FILTER_VALIDATE_IP) !== false) { $this->_ip_addresses[$key] = $ip; } } } } ksort($this->_ip_addresses); } return $this->_ip_addresses; } } // ---------- ISession.class.php ---------- interface ISession { /** * Returns the anti-CSRF token. * * @return string */ public function get_csrf_token(); /** * Set data to session. * * @param string $key * @param mixed $value */ public function set_data($key, $value); /** * Returns true if the data exists. * * The data must have been previously set with set_data(). * * @param string $key * @return bool True if the data exists. */ public function has_data($key); /** * Get data from session. * * The data must have been previously set with set_data(). * * @param string $key * @return mixed */ public function get_data($key); /** * Destroy the current session. */ public function logout(); /** * Start a new session. * * @return bool True on success */ public function login(); } // ---------- ManagementSession.class.php ---------- /** * Singleton. * * 'Management' session, meaning that the session has elevated * priviledges. * * Security features: * - timestamp-based session management * - network-/user-error-tolerant session ID regeneration * - strong anti-CSRF token * - strong session cookie security in terms of cryptography and cookie attributes * - prevent access from changed client IP address * - access log * * Requires PHP 7.1.0 or newer. * * See https://www.php.net/manual/en/session.security.php */ class ManagementSession implements ISession { private $_session_storage; // $_SESSION private $_started; private $_client_info; private static $_inst = null; private static $_log_mask = 0x0; private static $_log_file = null; private static $_cookie_domain = ""; private static $_cookie_name = "sid"; // time limits private const SEC_UNTIL_REFRESH = 60 * 15; // regenerate session ID if older than 15 minutes private const SEC_UNTIL_EXPIRE = 3600 * 4; // stop using session if older than 4 hours private const SEC_EXPIRE_MARGIN = 60 * 5; // explicitly invalidated sessions will be valid for 5 more minutes // bits for the log mask, used for selecting which events are to be logged const L_CREATED = 1 << 0; // created a new session const L_REFRESHED = 1 << 1; // regenerated the session ID for an exsisting session const L_NORMAL_ACCESS = 1 << 2; // good access of an existing session const L_BAD_IPADDR_ACCESS = 1 << 3; // access of an existing session with changed client IP address const L_EXPIRED_ACCESS = 1 << 4; // access of an existing expired session const L_DELETED_ACCESS = 1 << 5; // access of a nonexistent session const L_INVALIDATED_ACCESS =1 << 6; // access of an invalidated session const L_REFRESH_ERROR = 1 << 7; // failure during session ID regeneration const L_SID_CORRECT_ERROR = 1 << 8; // failure during session ID correction when accessing an invalidated session /** * Returns the Session object. * * @return ISession */ public static function get() { if (self::$_inst === null) { self::$_inst = new self(); } return self::$_inst; } /** * Configure the session log. * * Mask selects events to be logged. * * @param string $file The log file, e.g. "/media/wwwuser/logs/session_log" * @param int $mask Events to be logged, e.g. ManagementSession::L_BAD_IPADDR_ACCESS | ManagementSession::L_DELETED_ACCESS */ public static function configure_log($file, $mask) { self::$_log_file = $file; self::$_log_mask = $mask; } /** * Configure the session cookie. * * @param string $name The cookie name, e.g. "sid" * @param string $domain The cookie domain, e.g. "my-site.com" */ public static function configure_cookie($name, $domain) { self::$_cookie_name = $name; self::$_cookie_domain = $domain; } public function get_csrf_token() { $this->try_generate_csrf_token(); return $this->_session_storage["csrf_token"] ?? null; } /** * Generates the CSRF token if not generated already for the session. */ private function try_generate_csrf_token() { if (!array_key_exists("csrf_token", $this->_session_storage) && $this->_started) { $this->_session_storage["csrf_token"] = bin2hex(random_bytes(256 / 8)); // 256-bit } } public function set_data($key, $value) { $this->_session_storage["user_{$key}"] = $value; } public function get_data($key) { return $this->_session_storage["user_{$key}"] ?? null; } public function has_data($key) { return array_key_exists("user_{$key}", $this->_session_storage); } public function logout() { $this->invalidate(); } public function login() { return $this->access(true); } private function get_user_data() { $ret = []; foreach ($this->_session_storage as $key => $value) { if (substr($key, 0, 5) === "user_") { $ret[$key] = $value; } } return $ret; } private function access($new_session) { /* Either open an existing session or create a new one, depending on * bool $new_session. First invalidate any already opened session before * creating a new one ensuring that later access to it will be denied. * Open the session file and either create or validate the data. Correct * and regenerate the session ID if needed. If the session is expired or * the IP address has changed, session data is cleared. */ if ($new_session) { $this->invalidate(); } if (!$this->_started) { $this->_started = session_start($this->make_session_config(true)); } $success = false; if ($this->_started) { $this->_session_storage = &$_SESSION; $logmask = 0; $is_valid = true; if ($new_session) { $this->clear(); $this->create(); $logmask = self::L_CREATED; $success = true; } else { $logmask = $this->validate(); $is_valid = !$logmask; if ($logmask === self::L_INVALIDATED_ACCESS) { // only if this is the one and only validation error $logmask |= $this->handle_invalidated_access(); $is_valid = ($logmask & self::L_SID_CORRECT_ERROR) == 0; // clear session also if failed to correct } if ($is_valid) { $logmask |= self::L_NORMAL_ACCESS; $success = true; if ($this->should_refresh()) { $logmask |= $this->refresh(); if ($logmask & self::L_REFRESH_ERROR) { $success = false; $is_valid = false; // clear session also if failed to refresh } } } } $this->log($logmask, $this->_session_storage); if (!$is_valid) { $this->clear(); } } return $success; } private function create() { session_regenerate_id(); $t = time(); $this->_session_storage["ip_address"] = $this->make_client_address_string(); $this->_session_storage["expire"] = $t + self::SEC_UNTIL_EXPIRE; $this->_session_storage["refresh"] = $t + self::SEC_UNTIL_REFRESH; } private function refresh() { /* Regenerate session ID. Add new session ID info to the invalidated * session so it can be corrected if it's accessed before expiration. * Write and close the session and start a new one with the new ID. */ $expire = $this->_session_storage["expire"]; $ipaddr = $this->_session_storage["ip_address"]; $user_data = $this->get_user_data(); $this->invalidate(); $new_sid = session_create_id(); $this->_session_storage["new_sid"] = $new_sid; session_write_close(); session_id($new_sid); $this->_started = session_start($this->make_session_config(false)); if ($this->_started) { $this->_session_storage = &$_SESSION; foreach($user_data as $usr_key => $usr_value) { $this->_session_storage[$usr_key] = $usr_value; } $this->_session_storage["ip_address"] = $ipaddr; $this->_session_storage["refresh"] = time() + self::SEC_UNTIL_REFRESH; $this->_session_storage["expire"] = $expire; return self::L_REFRESHED; } else { return self::L_REFRESH_ERROR; } } private function invalidate() { if ($this->_started && !isset($this->_session_storage["invalidated"])) { $this->_session_storage["expire"] = time() + self::SEC_EXPIRE_MARGIN; $this->_session_storage["invalidated"] = 1; unset($this->_session_storage["refresh"]); } } private function clear() { foreach ([ "ip_address", "expire", "refresh", "invalidated", "new_sid", "csrf_token" ] as $key) { if (array_key_exists($key, $this->_session_storage)) { unset($this->_session_storage[$key]); } } foreach ($this->_session_storage as $key => $value) { if (substr($key, 0, 5) === "user_") { unset($this->_session_storage[$key]); } } } private function validate() { foreach ([ "expire", "ip_address", ] as $key) { if (!array_key_exists($key, $this->_session_storage)) { return self::L_DELETED_ACCESS; } } $mask = 0; if (time() > $this->_session_storage["expire"]) { $mask |= self::L_EXPIRED_ACCESS; } if ($this->_session_storage["ip_address"] !== $this->make_client_address_string()) { $mask |= self::L_BAD_IPADDR_ACCESS; } if (array_key_exists("invalidated", $this->_session_storage)) { $mask |= self::L_INVALIDATED_ACCESS; } return $mask; } private function should_refresh() { return array_key_exists("refresh", $this->_session_storage) && time() > $this->_session_storage["refresh"]; } private function handle_invalidated_access() { /* Open the proper session which invalidated this one. * The proper session ID cookie will be sent to client. */ session_write_close(); if (array_key_exists("new_sid", $this->_session_storage)) { // open proper session session_id($this->_session_storage["new_sid"]); $this->_started = session_start($this->make_session_config(true)); } else { $this->_started = false; } $mask = 0; if ($this->_started) { $this->_session_storage = &$_SESSION; } else { $mask |= self::L_SID_CORRECT_ERROR; } return $mask; } private function make_session_config($strict_mode) { // following the recommendations at https://www.php.net/manual/en/session.security.php $sess_conf = [ "name" => self::$_cookie_name, "use_strict_mode" => $strict_mode, "cookie_path" => "/", "cookie_domain" => self::$_cookie_domain, "cookie_secure" => true, "cookie_httponly" => true, "cookie_lifetime" => 0, "use_cookies" => true, "use_only_cookies" => true, "sid_length" => 48, "sid_bits_per_character" => 6, "cache_limiter" => "nocache", ]; if (PHP_VERSION_ID >= 70300) { $sess_conf["cookie_samesite"] = "Strict"; }; return $sess_conf; } private function make_client_address_string() { $addrs = $this->_client_info->get_ip_addresses(); return implode("_", array_map(function($ipaddr, $key) { return "{$key}:{$ipaddr}"; }, $addrs, array_keys($addrs))); } private function log($mask, array $data) { if (self::$_log_mask & $mask) { if (self::$_log_file && (is_writable(self::$_log_file) || (!file_exists(self::$_log_file) && is_writable(dirname(self::$_log_file))))) { $mask_str = dechex($mask); $data_str = json_encode($data); $dt = date("Y-m-d H:i:s"); $sid = substr(session_id(), 0, 64); // substr just for input sanitation file_put_contents(self::$_log_file, "[{$dt}] [{$sid}] [{$this->make_client_address_string()}] [0x{$mask_str}] {$data_str}" . PHP_EOL, FILE_APPEND); } else { $strfile = self::$_log_file ?? "(null)"; trigger_error("Failed to log session event: output file {$strfile} not writable"); // or throw new \RuntimeException(...) if you dare } } } protected function __construct() { if (PHP_VERSION_ID < 70100) { throw new \LogicException("PHP 7.1.0 or newer is required."); } $this->_session_storage = []; $this->_started = false; $this->_client_info = new \ClientInfo(); $this->access(false); } }
Konffaus
\ManagementSession::configure_log(__DIR__ . "/session_log", \ManagementSession::L_BAD_IPADDR_ACCESS | \ManagementSession::L_DELETED_ACCESS); \ManagementSession::configure_cookie("mySession", "my-site.com");
Sisään- ja uloskirjaus
if (validateLoginPassword()) { $ms = \ManagementSession::get(); if ($ms->login()) { $ms->set_data("management_permission", 1); } }
\ManagementSession::get()->logout();
Käyttäjän oikeuksien tarkistus onnistuu esim. näin
$has_permission = \ManagementSession::get()->has_data("management_permission");
anti-CSRF token formiin
<input name="antiCSRFToken" type="hidden" value="<?= \ManagementSession::get()->get_csrf_token() ?>">
Aihe on jo aika vanha, joten et voi enää vastata siihen.