Sicurezza sull'inclusione file dipendente da GET

[daniele_dll]

Hola,

volevo un consiglio, magari è una cosa stupida, ma sono abbastanza attento a questo tipo di problematica.

Premetto che questo check lo sto leggermente modificando rispetto a quello che uso nel mio framework ove il tutto dipende dal database mentre qui il file da includere deriva da un parametro in GET.

Si, lo so, è pericoloso, ma sto scribacchiando il mio pannello di controllo per l'hosting sul mio dedicato e non mi va di buttarci su tutto il mio framework con tutto quanto, richiede abbastanza tempo per lo sviluppo.

Come si può vedere, qui l'azione, ovvero il file da includere, deriva da $_GET['action']. Il check sull'esistenza dei file e sulla posizione del file lo eseguo tramite realpath.

Secondo voi è abbastanza sicuro?

Qui c'è il codice:

if (isset($_GET['action']) === false)
{
    $action = 'Dashboard';
}
else
{
    $action = $_GET['action'];
}

$pathCheck = realpath('./Actions/' . $action . '.action.php');
if ($pathCheck === false || dirname($pathCheck) !== realpath('./Actions'))
{
    echo "errore!";
}

[filippo.toso]

Una versione più compatta, e per certi versi più sicura:

<?php 
$action = isset($_GET['action']) ? $_GET['action'] : 'Dashboard'; 
$path = dirname(__FILE__) . "/Actions/$action.action.php";
if ((dirname($path) != dirname(__FILE__) . "/Actions") || !is_readable($path)) {
  die('Error');
}
?>

Ancor meglio:

<?php 
$valid_actions = array('Dashboard', 'Forum', 'Login', 'Store');
$default_action = 'Dashboard';

$action = isset($_GET['action']) && in_array($_GET['action'], $valid_actions) ? $_GET['action'] : $default_action; 
$path = dirname(__FILE__) . "/Actions/$action.action.php";
if (!is_readable($path)) {
  die('Error');
}
?>

[gianiaz]

Originariamente inviato da daniele_dll
Hola,

volevo un consiglio, magari è una cosa stupida, ma sono abbastanza attento a questo tipo di problematica.

Premetto che questo check lo sto leggermente modificando rispetto a quello che uso nel mio framework ove il tutto dipende dal database mentre qui il file da includere deriva da un parametro in GET.

Si, lo so, è pericoloso, ma sto scribacchiando il mio pannello di controllo per l'hosting sul mio dedicato e non mi va di buttarci su tutto il mio framework con tutto quanto, richiede abbastanza tempo per lo sviluppo.

Come si può vedere, qui l'azione, ovvero il file da includere, deriva da $_GET['action']. Il check sull'esistenza dei file e sulla posizione del file lo eseguo tramite realpath.

Secondo voi è abbastanza sicuro?

Qui c'è il codice:

if (isset($_GET['action']) === false)
{
    $action = 'Dashboard';
}
else
{
    $action = $_GET['action'];
}

$pathCheck = realpath('./Actions/' . $action . '.action.php');
if ($pathCheck === false || dirname($pathCheck) !== realpath('./Actions'))
{
    echo "errore!";
}

io seguo più o meno questo metodo, in realtà non faccio particolari verifiche, ma solo:

require(DIRECTORY.$action.".inc.php");

Se nella get mi mettono qualsiasi porcheria tipo "www.altrosito.com" non ho problemi (o fino ad ora almeno non ne ho avuti..

L'ideale magari sarebbe creare un'array di action valide e testarla con in_array, che dici?

ciao

[filippo.toso]

io seguo più o meno questo metodo, in realtà non faccio particolari verifiche, ma solo:

require(DIRECTORY.$action.".inc.php");

Se nella get mi mettono qualsiasi porcheria tipo "www.altrosito.com" non ho problemi (o fino ad ora almeno non ne ho avuti..

Ammettiamo che il tuo sito permetta l'upload di file da parte dell'utente. Con un codice come il tuo, sarebbe possibile hackerare il sito facilmente, è sufficiente caricare uno script PHP e poi passare un path tipo ../../../upload_folder/mio_file_php

[daniele_dll]

Originariamente inviato da filippo.toso
Una versione più compatta, e per certi versi più sicura:

<?php 
$action = isset($_GET['action']) ? $_GET['action'] : 'Dashboard'; 
$path = dirname(__FILE__) . "/Actions/$action.action.php";
if (!is_readable($path)) {
  die('Error');
}
?>

Ancor meglio:

<?php 
$valid_actions = array('Dashboard', 'Forum', 'Login', 'Store');
$default_action = 'Dashboard';

$action = isset($_GET['action']) && in_array($_GET['action'], $valid_actions) ? $_GET['action'] : $default_action; 
$path = dirname(__FILE__) . "/Actions/$action.action.php";
if (!is_readable($path)) {
  die('Error');
}
?>

mmm, non voglio mettere dentro un array le azioni supportate, quando devo fare modifiche mi tocca andare ad aggiornare anche la index, a me piaccono le cose automatizzate ^^

Inoltre considera che

$path = dirname(__FILE__) . "/Actions/$action.action.php";
if (!is_readable($path)) {
  die('Error');
}

non è proprio per niente sicuro :lol:

guarda che fatto cosi, se non ci fosse il check dell'array, come non c'è sopra, passando un po di ../ e la path e cosi via si potrebbe tranquillamente includere un file che non stia dentro Actions!

Il check che faccio con realpath questo lo include: realpath nel caso il percorso non esista e/o non sia accessibile da php restituisce false altrimenti restituisce il percorso liscio, ovvero se ci stanno ad esempio ../../../../etc/passwd (nel caso che tutti i vari chup portino alla root del disco) mi restituirebbe /etc/passwd (leggibile da php perché è leggibile da tutti gli utenti) e verifico con il secondo pezzo dell'if se ci posso accedere o meno, ovvero

dirname($pathCheck) !== realpath('./Actions')

dirname($pathCheck) mi restituirà nel caso di spora /etc che è diverso da /path/alla/root/del/sito/web/Actions

Originariamente inviato da gianiaz
io seguo più o meno questo metodo, in realtà non faccio particolari verifiche, ma solo:

require(DIRECTORY.$action.".inc.php");

Se nella get mi mettono qualsiasi porcheria tipo "www.altrosito.com" non ho problemi (o fino ad ora almeno non ne ho avuti..

L'ideale magari sarebbe creare un'array di action valide e testarla con in_array, che dici?

ciao

si ma in quel modo sei soggetto a possibili attacci cross site perché nel caso che non ci stia settato l'open base dir, e molti non lo settano altrimenti php non può accedere a /tmp e quindi andrebberò cambiati altri svariati parametri, si potrebbe accedere a file presenti su alri account del server e quindi anche a file con un nome ad hoc

[gianiaz]

Originariamente inviato da filippo.toso
Ammettiamo che il tuo sito permetta l'upload di file da parte dell'utente. Con un codice come il tuo, sarebbe possibile hackerare il sito facilmente, è sufficiente caricare uno script PHP e poi passare un path tipo ../../../upload_folder/mio_file_php

Ci ho pensato, ma innanzitutto funzionerebbe anche con il tuo metodo senza il controllo sull'azione, e poi in genere nell'upload dei file controllo l'estensione.

Per funzionare una cosa del genere dovrei permettere di inserire file senza estensione.

Comunque è una buona obiezione, e vedrò di correggere il mio codice (non si è mai abbastanza sicuri).



edit

Comunque pensandoci, basta mettere

require(DIR.basename($_GET['action']).'.inc.php');

ciao

[daniele_dll]

Originariamente inviato da gianiaz
Ci ho pensato, ma innanzitutto funzionerebbe anche con il tuo metodo senza il controllo sull'azione, e poi in genere nell'upload dei file controllo l'estensione.

Per funzionare una cosa del genere dovrei permettere di inserire file senza estensione.

Comunque è una buona obiezione, e vedrò di correggere il mio codice (non si è mai abbastanza sicuri).



edit

Comunque pensandoci, basta mettere

require(DIR.basename($_GET['action']).'.inc.php');

ciao

Beh, si, questo sistema è facile facile

[Giuliotresauri]

Non avevo la piu' pallida idea di un simile rischio con la get,ossia,il far includere dei file a seconda di come viene impostata la get...


Percio',la soluzione,se ho ben capito, (se uno vuole far fare le include a seconda delle get) e' utilizzare la basename,giusto? - come in DIR.basename($_GET['action']).'.inc.php'); -

[abidibo]

Posso consigliarvi di dare un'occhiata a come la pensa il Open Web Application Security Project (OWASP) al riguardo:
http://www.owasp.org/index.php/Top_10_2007-A3

[daniele_dll]

che è per altro quello proposto qui sul forum sia in questo thread sia in altri thread (facendo una ricerchina)