PHP and the dollar sign

Banuiesc ca “whitelist” este o denumire pretentioasa la acel “if(array_key_exists())”, care verifica sa nu cumva sa instantiez o clasa “crafted” de client :slight_smile: Dar era la mintea cocosului ca trebuie validat input-ul.

La fel cum unii programatori inca folosesc MD5 for password hashing la fel si asta nu este o problema evidenta pt. toata lumea - hence the bad practice.

Faptul ca pentru tine, acest lucru este fals nu implica automat ca este fals pentru toata lumea, tot vad ca nu intelegi asta.

Tot nu prea inteleg ce vrei sa spui. Nu tin minte sa fi zis ca un mecanism nu poate fi folosit gresit. Cand mergi cu 100km/h cu masina nu te opreste nimic sa bagi in marsarier si sa-ti praf motorul, transmisia si tot ce mai e pe acolo. Tu pari sa sugerezi ca e “bad practice” sa dotezi masinile cu marsarier, doar pentru ca e posibil sa faci ceva catastrofal.

2 Likes

Sunt deja pe pagina #2 de pe google results si nu gasesc nimic legat de asta in PHP world ceea ce probabil explica de ce nu este vazuta ca bad practice.

Gandeste-te la un controller action unde primesti un type - string-ul il tranformi intr-o constanta (clasa) si apoi instantiezi un obiect via new.

Fara whitelisting - poti instantia User in loc de Tag si ii trimiti mesajul #destroy => Auch. Sunt n-variante care se pot intampla: data destruction/leak, wrong behavious/data etc.

In Ruby world am vazut multe cazuri similare care nu sunt ok, chiar daca in final genereaza doar o exceptie insa au un potential de security risk quite high. Pentru asta s-a si adaugat #safe_constantize (constantize este echivalentul $class) in API-ul de la ActiveSupport.

De atlfel, mi se pare ciudat ca primele doua pagini cu exemple in PHP - nu specifica nimic de asta - doar “wow this is awesome, thanks” si da am cautat si varianta inversa - how to make it safe - no results, poate nu am cautat eu bine.


Daca ar fi sa trag o serie de concluzii:

  • pt. juniors nu este bad practice ca aparent nimeni nu le zice ca este
  • pt. juniors++ nu o vad ca bad practice pt. ca folosesc whitelisting instictiv (desi debatable)
2 Likes
  1. La şcoala de şoferi nu mi-a zis nimeni ca e o idee proasta sa bag in marşarier cand merg cu 100km/h. Ma intreb de ce n-am incercat s-o fac niciodata. Poate din cauza ca este atat de evident ce se poate intampla?

  2. Nu te poti numi programator daca nu validezi input-ul. Oricum un astfel de amator n-ar avea de unde sa stie ca poti lua numele clasei dintr-un string. Si nici macar nu e nevoie de asa ceva, daca vrei s-o dai cu bata in balta folosesti chestii mult mai simple.

Sa zicem ca esti cel mai naiv “programator” din lume si vrei sa permiti download cu contorizare, deci trebuie s-o faci prin php. Si in infinita ta naivitate scrii asa:

In fisierul html bagi link asa:

<a href="download.php?file=fisierulmeu.zip">download</a>

Iar in download.php scrii asa;

$fname = $_GET["file"];
$path = "/var/www/downloads/$fname";
readfile($path);

Si foarte multumit de mine zic, phuai, ce inteligent sunt. Am pus readfile sa citeasca cu path absolut, n-are cum sa citeasca /etc/passwd.

2 Likes

Sunt sigur ca orice junior stie de ce trebuie sa valideze, filtreze si escapeze inputul. Nu se poate sa nu fi auzit de la cei mai batrani povesti de groaza cu balauri, legate de sql injection si xss attacks.

Dar asta nu inseamna neaparat ca si stiu ce anume sa sanitizeze. Deasemenea, din ce in ce mai multi programatori invata de pe internet tot ce stiu. Iar nu toti au norocul sa lucreze cu persoane care sa spuna astfel de povesti de groaza.

Assumtions are the cause of most problems.

Daca nu stiu, sa invete, ca de aia au creier si e nevoie de ei. Altfel, daca softurile erau asa destepte, programau chiar ele, nu mai era nevoie de programatori umani.

Limbajul te poate ajuta pana la un punct, daca trece de punctul ala incepe sa te incurce. Pana la urma limbajele astea sunt create ca unelte pentru profesionisti. Daca nu esti in stare sa intelegi cum functioneaza si e prea complicat pentru tine, poti sa ramai la VisualBasic. Din 4 click-uri e gata “programul” :slight_smile:

Nu asta e ideea, deoarece nu au cum sa invete daca nu stiu de respectivele probleme. Iar, de obicei, in documentatie si tutorialele de pe net nu gasesti explicatii detaliate. Plus ca, in general, documentatia si tutorialele necesita contact interuman minim, ceea ce face ca multe persoane sa caute sa interactioneze cu alti programatori fie offline, fie online (exemplu fiind forumul acesta).

Dar deja am dat in offtopic.

Ideea este sa oferim exemplele detaliate, sa punem intrebari la alor caror raspuns sa se gandeasca, si sa ii invatam, direct sau indirect, sa isi puna intrebari de genul si pentru alte situatii. Iar asa ajungem la overengineering. (Si asta este o problema.) Practic, cum ai da-o, probleme tot sunt.

Ne invartim in cerc, eu dau un exemplu tu aduci altul + argumentum ad ignorantiam (ca poti face aceasta greseala doar daca iti lipseste half a brain or something) facand complet abstractie ca am intalnit destul de des aceasta problema (anectodal evidence, sure you can ignore this) dar chiar exista o functie implementata intr-o librarie pt. ea (actual evidence based on empirical data).

si in alte limbaje (Java, Python) similar thing:

But I strongly recommend to use a whitelist of classnames.

via http://stackoverflow.com/questions/6094575/creating-an-instance-using-the-class-name-and-calling-constructor

again cineva mentioneza macar the whitelist part in loc de “wtf!!! mind blown”

via http://stackoverflow.com/questions/4578335/creating-php-class-instance-with-a-string

Nu ne invartim in niciun cerc. Ai dat mai sus un exemplu. Te-am intrebat de cateva ori care e vulnerabilitatea acolo. Guess what, n-am primit niciun raspuns. Hai sa dezbatem concret, care e problema cu $instance = new $class_name. In afara de poveştile cu baubau, cu intrarile nevalidate. Ca daca nu validezi intrarea, ruby iti crapa cu fix aceeasi eleganţă.

That is why it’s a bad practice! AAAAA :)))) Daca uiti sa aplici o validare sau sa faci whitelist este un vector de atac sau at best weird behaviours.

Am dat peste n de astfel de oameni iar intr-o librarie dintr-un framework exista o functie pt. asta what more evidence do you need?

Stai putin sa inteleg. Deci daca in ruby uiti sa faci o validare nu se intampla nimic rau? O sa curga in continuare lapte si miere? Sau cum? A nu valida inputul nu e “bad practice”?

A folosi combinatia de let's turn a string into a constant + whitelisting (oricum i-ai zice) pentru cateva cazuri in loc de un banal switch sau case este bad practice.

Problema este ca faci o validare pt. whitelisting al unui pattern care poate fi usor inlocuit cu un switch sau case - diferenta e ca in cazul unui switch sau case nu ai cum sa uiti sa faci validarea.

Daca ai cateva variante, foloseste un construct simplu al limbajului nu metaprogramming.

Daca iti faci propriul DSL sau framework then by all means, go nuts.

2 Likes

Tot repeti ca e bad practice, si tot nu-mi spui de ce. Ruby n-o ia razna daca nu validezi?

1 Like

Pentru ca folosesti wrong tool for the job in caz ca nu era clar ce am zis mai sus:

  • folosesti un construct care tine de metapogramming si are propriile nuante care incepatorii sau cum vrei sa le zici uita de ele (adica uita sa si valideze input-ul)
  • motivul pt. care este folosit este: wow factor si writing clever code for the sake of that

S-au incalzit spiritele pe thread-ul asta, si e pacat, ca a inceput foarte frumos.

Am sa incerc eu sa raspund.

In general, generarea de cod dinamic, e un anti-pattern. Ma refer aici la chestii precum eval, constructie de sql de mana, invocare de functii pe baza unei valori dintr-o variabila[1], invocarea unei metode dintr-o variabila, generare de cod dinamic, etc.

Motivul principal este ca este extrem de usor sa te impusti in picior. Si asta ca expert nu ca novice.

O prima problema este securitatea. Daca invocarea dinamica este bazata pe input de la utilizator, trebuie sa ai extrem de mare grija ce faci. Exemplele date pe thread sunt simple, si oricine vede daca ceva e OK sau nu. Intr-un codebase de 1 milion de linii la care lucreaza 20 de oameni treaba nu mai e asa. Chiar si ceva simplu ca “toate controller-ele” din proiect s-ar putea sa fie greu de obtinut. Cineva o sa uite sa adauge ceva in white-list, altcineva o sa uite sa stearga ceva din whitelist, productivitatea scade si attack surface-ul creste. Plus, cand set-ul de validare o sa aiba 1000 sau mai multe elemente, cuiva o sa-i vina idea buna sa puna si expresii regulate acolo. Alt canal de atac. Cand ai o alternativa simpla ca un map de la rute la handlere (cum ai in node/express, ASP.NET MVC), sau anotari (ASP.NET MVC) zic ca nu merita complexitatea.

O alta problema este performanta. Mai toate limbajele folosite pe partea de web sunt interpretate cu un compilator de bytecode JIT mai mult sau mai putin bun. Dar chiar si cele mai slabe apreciaza cand ai un apel la o functie cunoscuta la compile-time (constructorul controllerului), in loc de un eval/lookup in scope urmata de apelul unei functii date de rezultat. Cand treaba asta se intampla in hot-path - pentru fiecare request pe care-l procesezi, chiar conteaza.

De asemenea, in cazul unui switch sau unui map, multe editoare o sa-ti arate ca un controller / functie etc. este folosit, de cate ori este etc. Cand treaba e dinamica (sau cu anotari, din nefericire), pierzi informatia asta. Si ai un set de functii/clase care sunt folosite, dar pe care uneltele de analiza statica nu le pot identifica ca atare.

Sigur, nu inseamna ca sunt lucruri de evitat complet. Au si ele rostul lor, altfel n-ar fi in limbaj. Cateodata chiar trebuie sa evaluezi un program dat de utilizator (pe HackerRank sau jsfiddle de exemplu). Cateodata trebuie sa generezi cod automat, pentru ca profilul de performanta nu poate fi atins altfel. Dar de cele mai multe ori, exista o alternativa mai simpla, mai performanta si mai buna.


[1] ceva de genul

x = ‘sqrt’
$x(100)
Asemanator cu invocarea de constructor din variabila.

5 Likes

De acord cu tot ce ai zis. Eu doar incerc sa spun ca feature-ul nu este rău per se. Toată şmecheria este sa-l folosesti cu cap. Ca asa o gramada de tehnologii sau mecanisme ar putea fi considerate evil. Multithreading-ul e evil, metaprogramming-ul e evil, sql-ul e evil, polimorfismul e evil, overload-ul operatorilor e evil, functiile lambda sunt evil… Ce ne mai ramane? :slight_smile:

pai mare parte din ele (indeosebi ultimele 3) chiar nu se recomanda a fi folosite in mod uzual de programatorii care dezvolta o amarata aplicatie web

1 Like

sunt utile in middle steps of refactoring a huge class/method.sau cand nu ai destule date inca unde ar trebui sa existe o metoda.

1 Like