Abszolút nyomi PHP biztonságosabbá tétele.

( Fisher | 2012. 09. 28., p - 16:26 )
Internet: PHP, CGI, stb.

Van egy - sajnos megvásárolt - wordpress pluginem, amiben egy ilyen remek dolgot találtam: 


if($_GET['userid']) {
        $config  = $wpdb->get_results("SELECT * FROM ".$wpdb->prefix."data_table WHERE id=".$_GET['userid']);

Az még nekem is marha feltűnő, hogy ez így nagyon nem pompás. Nyilván van az ilyen esetekre már kicsiszolt preg_match vagy valami más minta, amit használni illik, és biztos jobb lesz mintha én találom ki a langyos vizet.

Légyszi help, mert a PHP-hoz (is) hülye vagyok :D

( Frantique v | 2012. 09. 28., p - 16:30 )


if($_GET['userid']) {
$config = $wpdb->get_results("SELECT * FROM ".$wpdb->prefix."data_table WHERE id=".mysql_real_escape_string($_GET['userid']));

--
Coding for fun. ;)

A mysql_real_escape_string jó, de igazából a $wpdb ojjektum (azaz a szülő osztály) megfelelő függvényét vagy a WP beépített változó kezelését kéne használni. Ráadásul a mysql_real_escape stringnek kell egy link resource is.

Ami a hab a tortán: "Use of this extension is discouraged. Instead, the MySQLi or PDO_MySQL extension should be used. See also MySQL: choosing an API guide and related FAQ for more information."
Link: http://hu.php.net/manual/en/function.mysql-real-escape-string.php

Az extra bónusz, pedig hogy eleve üres változóra és legalább egy is_numeric-re lehetne szűrni mielőtt betolja bármilyen függvénynek és utána escape-el. :)

Ha az az ID valami numerikus dolog szeretne lenni (a kod alapjan ez a helyzet), akkor legcelszerubb arrol meggyozodni, hogy tenyleg numerikus-e (is_numeric).
Viszont ahol egy ilyen van, ott tobb is akad, altalanos megoldas viszont jelenleg nem nagyon van, max. eletehetsz valami wafot, es remenykedhetsz, hogy mukodokepes marad.

Az is_numeric()-kel figyelni kell, 0xabcd hexaban ertelmes, emiatt true. 1e10 szinten ertelmes (exponens), pedig mindkettoben vannak betuk.

Egyebkent Bobby Tables szereti feltorni az ilyen oldalakat..

--
My gold plated butt-plug business is being sued by Apple.
Apparently they have a patent for overpriced crap for arseholes.

( sj | 2012. 09. 28., p - 18:14 )

mondjuk kezdesnek valami ilyesmi:


if(isset($_GET['userid']) && $_GET['userid'] && is_numeric($_GET['userid']) && preg_match("/^([0-9]+)$/", $_GET['userid']) ) {
$config = $wpdb->get_results("SELECT * FROM ".$wpdb->prefix."data_table WHERE id=" . (int)$_GET['userid']);
...

de en inkabb egy ilyesmi megoldasra allnek at:


if(isset($_GET['userid']) && $_GET['userid']) {
$config = $wpdb->get_results2("SELECT * FROM ".$wpdb->prefix."data_table WHERE id=?", array((int)$_GET['userid']));
...

Miert kell nekem sajnalnom a Klubradiot?

Ne halmozd...

A if ($_GET['userid']) vizsgálata önmagában korbácsolással jutalmazandó, ráadásul az isset($_GET['userid']) mellett felesleges is. Arról nem is beszélve, hogy az is_numeric() bármely értéket elfogad, amely számként értelmezhető de erre meg úgy is ott a preg_match... 

if (!isset($_GET['userid'])) 
  throw new Exception('Nem jött userid.');

if (!preg_match('/^([0-9]+)$/', $_GET['userid'])) 
  throw new Exception('Érvénytelen userid');

$res = $wpdb->get_results($wpdb->prepare('SELECT ...', (int)$_GET['userid']);

Exception helyett opcionálisan lehet dobni InvalidArgumentException() -t

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™

Ez az egyik baj a PHP-vel: egyes nyelvi elemek úgy néznek ki, mint egy függvény (eval, list, stb. ráadásul sokan az echo-t meg a require/include[_once] -kat is függvényként ábrázolják kódban, ami AFAIK plusz egy felesleges kiértékelést okoz a zárjójel miatt), amit jobb IDE-k ugyan kiemelnek, de többnyire inkább nem.

Aztán ha valaki nem nézi meg, hogy mégis mi ez a nyelv (ami a PHPistukák 100%-a, a maradék kóderek 95%-a és a
programozók 50%), amivel dolgozik, akkor érik ilyen kellemetlen meglepetések az embert.

Arról nem is beszélve, hogy kétlem, hogy a scriptnyelvekben programozók nagy része egyáltalán tisztában van ilyen fogalmakkal, mint érték/referencia szerinti átadás, függvény, metódus, osztály, interface, absztrakció, PHP esetén, típusok közötti különbség, tömb, hashmap, list, stb.

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™

( Elbandi v | 2012. 09. 29., szo - 00:38 )

meg ha mar wp, akkor olvass ide vonatkozo doksit

--
A vegtelen ciklus is vegeter egyszer, csak kelloen eros hardver kell hozza!

Igen, ezt várnám a nagyszerű fejlesztőktől, hogy ne trágyahalmot toljanak ki a kezük közül. Alighanem ideje elkönyvelnem a veszteséget, és keresni egy másik plugint, lehetőleg olyat, amihez nem kell kézzel átmatatnom az összes érintett bejegyzést, de ez már egy másik történet.

( Jason v | 2012. 09. 29., szo - 09:42 )

http://php.net/manual/en/function.filter-input.php

esetleg ezt a $_GET -ek helyett. 


$userid = filter_input(
			INPUT_GET, 
			'userid', 
			FILTER_SANITIZE_NUMBER_INT, 
			array('options' => 
				array(
					'default' => 1, /* value to return if the filter fails */ 
					'min_range' => 0 /* will fail if number is below this */
				), 
				'flags' => FILTER_FLAG_ALLOW_HEX /* parses hexadecimal numbers also */
			)
		);

( Fisher v | 2012. 10. 03., sze - 20:41 )

Úgy látszik a mysql_real_escape_string nem jött be. Nyomtak egy ilyen get-et: 


"GET /wp-content/plugins/plugin/playlist.php?videoid=9%20UNION%20SELECT%201,2,3,group_concat(user_login,0x3a,user_email,0x3b),5,6,7,8,9,10,11%20FROM%20wp_users-- HTTP/1.1"

Ezzel megszerezték a szükséges e-mailt.

Majd noszogatták kicsit a wordpress-t, hogy küldjön jelszócserét: 


"GET /wp-login.php?action=lostpassword HTTP/1.1"

A mysql logban szépen látszik is: 


UPDATE `wp_users` SET `user_activation_key` = 'V86U3lRBY4TWbW23vHJF' WHERE `user_login` = 'fisher'

És szaporán ki is olvasták a szükséges kulcsot: 


"GET /wp-content/plugins/plugin/playlist.php?videoid=9%20UNION%20SELECT%201,2,3,group_concat(user_login,0x3a,user_activation_key,0x3b),5,6,7,8,9,10,11%20FROM%20wp_users-- HTTP/1.1"

Majd jelszót váltottak, akkor már a wordpressen keresztül.

Leellenőrizem, a mysql_real_escape_stringet a javasoltnak megfelelően adtam meg, de úgy látszik nem használt. Most az indiai fejlesztő megoldását tettem be, ami egy preg_replace, de azt már le akarom cserélni egy $wpdb->prepare megoldásra, de mivel a php-hoz se értek, ezt inkább majd a hétvégén, amikor majd jobban ráérek utánaolvasni.

Tipikus hozzászólás egy olvasni/értelmezni nemtudótól... Az (int) igenis alkalmas lehet arra, hogy kiküszöbölje a problémát (feltéve, hogy csak int lehet az ID-ben). Ellenkező esetben preg->bukta, mysql_real_escape_string->bukta. Az is_numeric-re még nem láttam működő megoldást, ennek ellenére már nem használom. Prepared statement mindenhol alapból, + is_numeric/(int) (mysqli, de mivel ahhoz van még mit fejleszteni főleg hibakezelésben, át fogok térni a PDO-ra). A preg-et megkerülő megoldások előtt kalapot emeltem, ebben a ferdeszeműek az ászok :)

A PHP biztonságról lehetne hónapokig tartó szemináriumokat tartani. A kedvencem a hogyan futtassunk PHP-t egy képfájlból, de még rengeteg ilyen vicces (legtöbbször Apacs alapbeállításra/funkcióra) visszavezethető módszer van hackelni. Logokból, és orosz klienscímekből kiindulva finom dolgokat találni...

Jeleztem, hogy jelen esetben nem ér semmit a real_escape_string. Hogy miért? Mert az ugyan kiescapeeli (vagy hogy írják) a string határolókat, de a query-ben nem volt string.

A javított escapeelős kód ilyesmi lett volna:
$wpdb->get_results("SELECT * FROM ".$wpdb->prefix."data_table WHERE id='".mysql_real_escape_string($_GET['userid'])."';");

A WP-s megoldás (vaktában):
$wpdb->query( $wpdb->prepare('SELECT * FROM '.$wpdb->prefix.'data_table WHERE id=%d;', $_GET['userid']));
Legalább láthattunk egy valódi példát ennek kihasználására :)

Az a baj, hogy nekem nem világos, hogy hogyan fordul át a GET tartalma. A %20 nyilván szóköz lesz, így első körben lehetne vágni az első szóköztől a string végéig mindent. De nem tudom, hogy lehet-e szóközt csempészni egy sql statementbe, hogy nem a szóköz karakter van ott, illetve fogalmam sincs, hogy szóköz helyett szerepelhet-e pl. logikai operátor, olyasmi mint a &, esetleg parancselválasztó mint a bashban: ls;date Ehhez viszont sql könyvet kéne olvasnom.

Igazság szerint azt is meg kéne néznem, hogy az id-re van-e bárhol olan megkötés, validálás, hogy nem lehet benne szóköz, mert jelenleg már arra hajlok, hogy még a numerikus típus se tuti, lehet hogy string, amiben elvileg bármi lehet.

Mondjuk a tábladefiníciót megnézhetném, de most rohannom kell.

1. már rég hagytam
2. elolvastam
3. honnét tudjam hogy melyik megoldást válasszam az ajánlott 3-4 közül? Pláne hogy elég sokszor jönnek cáfolatok a korábban tutinak véltre.

A megoldás alighanem a sajátom lesz, a stringet vágom az ötödik karaktertől, így az ID elmehet 99999-ig, és csak hogy legyen benne, teszek egy (int)-et.

A saxusét választottam, mert úgy döntöttem, hogy csak olyat használok fel, amit értek is. Ám abban helyből van egy tipikus hiba, helyesen így lenne szerintem: 


if (!preg_match('/^([0-9]+)$/D', $_GET['userid'])) 
  throw new Exception('Érvénytelen userid');

A magyarázatát lásd itt: http://blog.php-security.org/archives/76-Holes-in-most-preg_match-filte…

Egyelőre még keresem a problémákat vele, nekem az szúrt szemet elsőnek, hogy semmiféle hosszvalidálás nincs benne, ezért keresgéltem, hogy buffer overflow-ra használható lenne-e. De mindenhol esküdöztek, hogy a php nem olyan, illetve hazafelé a metrón gondolkoztam, és gyanítom, hogy a proxy vagy a http szerver előbb kiakadna egy lehetetlenül hosszú url-en, így ez aligha lesz probléma.

Egyelőre amúgy a fejlesztő által adott preg_replace van bent, ami leginkább ennek az inverze, kitakarít kb. mindent, ami nem szám. Ez se annyira szimpatikus megoldás, ezért akarom lecserélni.

Gyorsan csináltam pár tesztet, és bugosnak tűnik a filter: 


<?php

$userid = (int)filter_input(
                        INPUT_GET,
                        'userid',
                        FILTER_SANITIZE_NUMBER_INT,
                        array('options' =>
                                array(
                                        'default' => 1, /* value to return if the filter fails */
                                        'min_range' => 0 /* will fail if number is below this */
                                ),
                                'flags' => FILTER_FLAG_ALLOW_HEX /* parses hexadecimal numbers also */
                        )
                );

echo $userid;
?>

Itt tesztelem: http://blog.fisher.hu/b.php?userid=aa-------------------123a4\n5344590
Ez ezt adja vissza: 


-------------------12345344590

Ami nekem nem tűnik valid int-nek. Legalábbis az (int) megvetően néz rá.

php-ben a sok minuszjel legális :) ugyanis az int opcionális mínuszjelekből majd utána minimum egy számjegyből áll :-D

A 3. paramétert úgy variálod, ahogy akarod, a sanitize pl megtisztítást jelent. A validate filterekkel (pl. FILTER_VALIDATE_INT) tudsz hamist kérni, ha nem szám pl.

Itt vannak tételesen, típusonként: http://www.php.net/manual/en/filter.filters.php
Speciel így utólag, nekem sem tűnik értelmesnek a sanitize_int, ami gyakorlatilag kiszűri a nem-számjegyeket a stringből. Neked a validate fog kelleni. Ekkor:

$userid = filter_input(INPUT_GET, 'userid', FILTER_VALIDATE_INT, array('options' => array('min_range' => 0)));
echo $userid;

ez FALSE lesz ha nem szám, s a szám, ha okés. Try.

"php-ben a sok minuszjel legális :)"
Oké, de erre: 


<?php
$num= 2 + ---4
?>

Én ezt kapom: 


PHP Parse error:  syntax error, unexpected '-' in /tmp/dd.php on line 3

Try, try... ezt akartam elkerülni. De alighanem az lesz, hogy akkor FILTER_VALIDATE_INT lesz, és ha nem számként ismeri fel, akkor csinálok valamit.

Ha csak 0..9 karaktereket engedsz, helyből nem lehet benne minusz jel.

Egyébként: a két módszert lehet kombinálni is. :)

A filter_input-tal nekem a személyes problémám az, ad "default" értéket, ha hibás érték jön. Miért? Ha hibás érték jön, miért pl. az 1-es userid-t adjuk meg? (Ami jó eséllyel valami admin user). Mindezt ahelyett, hogy hibás adat esetén a próbálkozót elküldenénk melegebb éghajlatra.

Ha már mindenképp filter_input, akkor szerintem így: 

$userid = filter_input(... 'default' => null, ...);

if ($userid === null) throw new Exception('Érvénytelen userid');

(Ilyen esetben semmiképp se castold át (int)-é, különben a null értéket 0-vá castolja!)

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™

Az ellenőrzés után azért a $wpdb->prepare() -t nem ártana propagálni, mert a WP doksi szerint is az a nyerő. Fullos exceptiont főleg nem célszerű dobni egy adott CMS-en belül, hanem a saját hibakezelő függvényét kellene bedrótozni, bár én nem értek a WP-hez sem, meg a PHP-hoz is csak kicsit. :)

Természetesen a prepare nem maradhat el.

Exception dobása meg teljesen jó lenne, mert általános. Egyetlen probléma, hogy valószínűleg a WP hírből sem ismeri (ismerve az átlag PHP-s "hiba"(nem)"kezelés", de alapvetően a probléma szempontjából lényegtelen.

Ha meg már hackelni akar valaki, akkor akár egy nyuszikát is ki lehet rajzolni a parasztnak, oly' mindegy.

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™

Szerintem az egesz mar ott bukik, hogy a userid siman get parameterkent kozlekedik oda-vissza.
Amikor par eve hackthissite-on peldakat oldottam meg, az ilyen ?userid=234 atirasa ?userid=1-re kb. a bemelegito feladatok kozt szerepelt.

--
My gold plated butt-plug business is being sued by Apple.
Apparently they have a patent for overpriced crap for arseholes.

Jobb helyeken van egy belepeskori auth. (user/pass keres), utana - ha ez megfelelo - letarolnak egy session cookie-t, es a username/userid (amit epp hasznalnak) server oldalon marad.

Ha ehelyett a userid ide-oda kozlekedik a bongeszo es a server kozott, akkor a user siman atirhatja, es hozzaferhet mas userek adataihoz. Esetleg admin jogot is kaphat, ha annyira hulye volt a rendszert keszito indiai biorobot.

--
My gold plated butt-plug business is being sued by Apple.
Apparently they have a patent for overpriced crap for arseholes.

Ki mondta, hogy az user a saját adataihoz fér hozzá? Ld. hupon a profil oldalt meg a track oldalt. Attól, hogy userid, még nem biztos, hogy a saját userid.

Az meg hogy GET vagy POST vagy COOKIE az totálisan irreleváns. Kliens oldaltól érkező adat => meg kell győződni arról, hogy valóban az-e, amire számítunk, minden egyéb esetben meg kell tagadni a kérés kiszolgálását.

És ez totálisan független a jogosultságkezeléstől, session hijackingtól és más egyéb, magasabb szintű funkcióktól.

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™

A hupos pelda jogos, valoban nem csak emiatt johet be userid. Az SQL injection hegyek utan - rosszindulatuan, de talan jogosan - felteteleztem, hogy a userid-t nem erre hasznalja az illeto. Csak finoman jeleztem, hogy ez a plugin valahol az alapoknal lehet nagyon elrontva. (ha jol ertem, eddig ketszer jottek be a plugin miatt)

A GET/POST/Cookie nyilvan nem szamit, bongeszobol jon, manipulalhato.

--
My gold plated butt-plug business is being sued by Apple.
Apparently they have a patent for overpriced crap for arseholes.

Ha ehelyett a userid ide-oda kozlekedik a bongeszo es a server kozott, akkor a user siman atirhatja, es hozzaferhet mas userek adataihoz.

jahogy te igy ertetted. Ez szamomra annyira trivialis, hogy bemondasra nem hissszuk el, hogy uid=0 vagy, hanem login utan session-t nyitunk user belanak, es onnantol kezdve nem kell a userid-nek sehogysem ide-oda mennie, sot a user szamara sokszor teljesen mindegy a userid-je.

Miert kell nekem sajnalnom a Klubradiot?

( wladek1 | 2012. 10. 04., cs - 22:13 )

talan:
Rekurzivan vegigmesz az osszes user inputon es castolsz/mysql_real_escape_string()/addslashes()/etc

( saxus | 2012. 10. 04., cs - 22:52 )

Összességében nézve, ez a topic nagyon szép ékes bizonyítéka annak, hogy a szakma úgy általában miért van olyan véleménnyel a (magukat programozónak hivő) PHP tákolókról, amilyenről.

Ez a topic kb. egy az egyben mehetne a TDWTF-re.

----------------
Lvl86 Troll, "hobbifejlesztő" - Think Wishfully™