[Sicurezza PHP] Dati passati in $_GET

[wartpro]

Buongiorno a tutti,

vorrei ricevere un consiglio e magari delle dritte riguardo il modo in cui gestisco, in questo caso, i dati provenienti dall'array $_GET.

Ho un "box" dove all'interno ci sono delle immagini. I link che portano alla pagina che visualizza la foto gallery sarà così composto:

www.sito.it/photo.php?album=nature&num=1

Dove "album" va a prendere l'album e "nature" l'id (dal database) della foto visualizzata.

Cliccando sull'immagine (sul link), si va a pagina "photo.php" dove all'interno c'è:

if ($_GET) {

if (isset($_GET['album'])) {

    $album_from_get = mysql_escape_string(htmlentities(strip_tags(trim($_GET['album'])), ENT_QUOTES, 'UTF-8'));
    
    $allowed_albums = array('animals','people','details','places','nature');

    if (!in_array($album_from_get, $allowed_albums)) {

        $album_from_get = str_replace($album_from_get, "places", $album_from_get);

    }

    $album_selected = $album_from_get;

    switch ($album_selected) {
        case 'animals': $cat = 'x1'; break;
        case 'people':  $cat = 'x2'; break;
        case 'details':  $cat = 'x3'; break;
        case 'places':  $cat = 'x4'; break;
        case 'nature':  $cat = 'x5'; break;
        default:          $cat = 'x5'; break;
    }

} else { $cat = 'x1'; }

} else {

    echo "get non settato";
    exit();

}#    $_GET 

Il codice è semplice da capire:

- Controllo innanzitutto se l'array $_GET è presente..altrimenti..implementerò una contromossa;
- Controllo se l'array $_GET['album'] è settato..se no, imposto una categoria di album di default da estrarre dal db;
- Se $_GET['album'] è settato, trimmo->strip_taggo,htmlenTITLO, ecc. il dato passato in $_GET['album'];
- Imposto in un array i valori possibili che il $_GET['album'] deve contenere e..se non contiene uno tra questi, sostituisco il suo contenuto con un valore di default.
- Infine passo la palla allo switch che verificherà il contenuto di $_GET['album'] ed imposterà la giusta categoria da estrarre dal db per stampare a video la gallery corretta;



Ora..

..è un approccio corretto?
..ci sono controlli ulteriori che fareste e che mi sono dimenticato?
..ci sono passaggi obsoleti?


Grazie in anticipo

[goikiu]

potresti controllare che effetivamente $_GET[num]sia un numero.

Ne esistono varie versioni.... a seconda del dato che ti aspetti.

http://it.php.net/manual/en/function.is-numeric.php

Sulla sinistra della pagina ufficiale noteria is_int, etcetera, in base al tipo di dato che ti aspetti, se è un numero normale probabilmente stai cercando is_num allora.

[wartpro]

Ah già..la variabile [num]..mi ero dimenticato. Questo il codice che gestisce la variabile [num] (perdonami)

#    --------------------------------------------
#    IF PHOTO REQUEST COMES FROM AN OTHER PAGE..
#    --------------------------------------------
if (isset($_GET['num'])) {

 #    ------------------
 #    CLEAN $_GET['num']
 #    ------------------
 $num_get = mysql_escape_string(htmlentities(strip_tags(trim($_GET['num'])), ENT_QUOTES, 'UTF-8'));
  #    ----------------------
  #    GET PHOTO IDs FROM DB
  #    ----------------------
  $test_input = mysql_query("SELECT id FROM --tabella-foto--");

  while ($test_res = mysql_fetch_assoc($test_input)) {

    $test_res_ar[] = $test_res['id'];
  }
    #    ---------------------
    #    CHECK $_GET MATCHING
    #    ---------------------
    if ((!preg_match('/^[0-9]+$/',$num_get)) || (!in_array($num_get, $test_res_ar))) {

       $test_res_ar = array_flip($test_res_ar);    # SET RANDOM VALUE IF NO MATCHINGS
       $num_rand = array_rand($test_res_ar,1);
       $num_get = $num_rand;
    }
      #    -------------------
      #    GET QUERY & RESULT
      #    -------------------
       $actual = mysql_query("SELECT dati,dati FROM --tabella-foto-- WHERE id='$num_get'");
       $present = mysql_result($actual,0,'dato');
       $newlabel = mysql_result($actual,0,'dato');

} else {

    $present = mysql_result($album_select,0,'dato');
    $newlabel = mysql_result($album_select,0,'dato');
} 

Dunque:

1) Verifico che $_GET['num'] sia settato..altrimenti mi prendo il primo risultato utile dal pezzo di codice visto prima ($album_select);
2) Se $_GET['num'] è settato, via con l'escape, trimmamento, strippamento, ecc. (Questo punto è quello che più di tutti mi interessa..che ne pensi di questo?);
3) Invio la query e metto i risultati in un array;
4) Controllo sia che il valore passato in $_GET['num'] sia un numero, e sia che sia presente all'interno dell'array pocansi creato..l'array con gli id delle foto.
4.1) Se il tutto NON corrisponde allora prendo un numero a caso tra gli id estratti e lo faccio girare ad ogni "refresh" di pagina con $_GET['num'] non settato.
4.2) Se il tutto E' SI, settato allora invio la query finale per il risultato;


Che ne pensi? (ate?)

[goikiu]

Mi sembra possa funzionare... io gestisco le cose leggermente differentemente... ovvero, non faccio striplashes, trasformazioni, etcetera... se la variabile che mi aspetto è un numero innanzitutto controllo la lunghezza e controllo con preg_match che abbia solo numeri, altrimenti l'utente l'ha inserito a manina...

Per quello che potrebbe esser testo.... se è statico allora faccio uno switch come te, solo che nel caso default metto l'errore.

Se è dinamico uso l'array e controllo con in_array

Diciamo che tolgo parecchi controlli ma l'importante è che poi il risultato sia sicuro no?

[wartpro]

Si si pienamente d'accordo. Infatti puntavo proprio a quello, ad una considerazione sul "grado di sicurezza" del codice qui utilizzato.

Penso che potrebbe andare.

Ho fatto semplici "test xss" e quando si inseriscono tag script o php i caratteri nella barra dell'URL vengono trasformati per mano (di sicuro) della funzione htmlentities.

Senti tu come gestisci gli errori delle query?

..nel senso..

Sono io che sono supermega paranoico a mettere sotto ogni query inviata al db, un if (!$query) { ... } oppure è pratica "comune" tra noi?

[xproj]

io credo che una volta fatto l'escape dei dati in input, a livello di sicurezza ci siamo... andare a vedere ad esempio se un dato è numerico o meno perché è un id, mi sembra superfluo. Se non è numerico la query non darà nessun risultato e basta...

[goikiu]

@wartpro:

Io controllo più che altro se vi sono righe, di solito per i controlli su lingua esistente, per il resto di solito mi aspetto almeno una riga, non può esser vuota, se è vuota qualcuno ha bucato qualcosa... ma in generale avendo sempre un while $riga = mysql_etcetera al limite non passerà mai dentro il ciclo.

@xproj:

Non capisco il tuo approccio... esistono diversi tipi di approcci... io non faccio l'escape delle stringhe ad esempio, o lo faccio molto raremente, semplicemente dopo un trim della variabile testuale controllo i caratteri inseriti, nel caso di login e password mi aspetto solo valori alfanumeri standard a-z (maiuscolo e minuscolo) e 0-9, niente accenti o altri caratteri, se ci sono perchè fare l'escape? Sarebbe un lavoro inutile no?

Per gli altri tipi di campi... dipende, se mi aspetto un testo che va a database devo fare un str_replace di caratteri accentati con la loro entità html ed un add_slashes ma per il resto non mi aspetto che lo staff del sito Y decida di auto-craccarsi il sito ti pare?

Inoltre considera che tutto va in base al target del programma, nel mio caso parlo di un pannello di gestione applicazione, ad accesso riservato, quindi non puoi "iscriverti", dunque non mi aspetto che qualcuno che non sappia cosa deve fare entra nel programma... se ci entra tanto mi può comunque mandare a palline il lavoro svolto, quindi a che prò invece mettere tutti i controlli sui miei nmila form (e relativi campi) ?

[xproj]

un escape (io uso mysql_real_escape_string) ha come fine quello di evitare un attacco di tipo sql injection... soprattutto sui login... io facevo un discorso generale sulla sicurezza dei form...

E perché non permetti ad esempio caratteri tipo underscore? Così spingi a scegliere password deboli

[goikiu]

Una password "complessa" normalmente viene intesa con caratteri alfanumerici da a-z più i numeri 0-9 dove le lettere sono maiuscole e minuscole.
Non ho mai visto un sito che mi dicesse di usare caratteri "speciali" per la complessità di una password.

[wartpro]

Ok..ho capito: lascio il controllo su TUTTE le query che invio

Spero solo di non venire tempestato di email