Kryptografifejl fundet i PHP 5.3.7

5.3.8 Er sådan set tilgængelig fra deres mirrors. Den er bare ikke blevet annonceret endnu.

http://php.net/get/php-5.3.8.tar.gz/from/a/mirror

Jeg er ked af at sige det men generelt er PHP noget crap!

http://maurus.net/resources/programming-languages/...

Det er meget muligt at argumenterne er gamle - det ændrer dog ikke ved konceptet som jeg stadig har svært ved at tage seriøst.

EDIT: hvordan kan man anbefale ikke at installere en given version? hvorfor fjerner man den ikke hvis det alligevel er spørgsmål om få dage før en rettet version kommer?

(hej flamebait)

#2 Are you married to Microsoft?

php har sine meritter - dog bruger jeg det ikke så meget selv

Fantastisk fejl, prøv lige at overveje konsekvenserne.

Heldigvis er den fejl nem at opdage for dem der bruger et af de berørte formater. Det er først hvis man bruger et af de berørte formater, ikke tester den nye version før den installeres i produktion, og ikke har backupkopier af sine data, at det kan gå helt galt.

Hvis man har hashes i sin database med et berørt format men fra en version uden fejlen, så vil brugerne ikke længere kunne logge ind. Den vil simpelthen afvise det korrekte password som om det havde været ugyldigt.

Hvis derimod et password oprettes med den fejlbehæftede version, så vil den fejlbehæftede version acceptere alle passwords som gyldige til det login. Der er tre måder det kan ske på.
1. Nyt login oprettes med fejlbehæftet version.
2. Password ændres på en bruger hvis password var gemt i et andet format. (Det vil kun ske hvis man har valgt at skifte hvilket format password gemmes i siden brugeren sidst ændrede password.)
3. Password ændres for en bruger som allerede var logget ind, og man havde glemt at kræve indtastning af password igen for at ændre password.

I hvert af de tre tilfælde vil man stå med et login hvor et vilkårligt password vil blive accepteret. Desuden vil man absolut ingen anelse have om hvad passwordet er. Skifter man efterfølgende til en version uden fejlen vil det login være låst, alle passwords vil blive afvist. Både det korrekte password og det password der blev skiftet fra vil blive afvist.

Det bedste et site kan gøre i det tilfælde er at identificere alle berørte brugere og bruge den nyeste backup til at finde det foregående password.

Worst case scenariet kræver dog at mange ting sker på en uheldig måde. Det mest sandsynlige er at man installerer den nye version og opdager at ingen kan logge ind, og derefter nedraderer eller opgraderer til en version uden den fejl.

Problemet ligger i at man i nogle tilfælde, når crypt() benyttes med MD5, kun får en salt-værdi retur fra funktionen, og altså ikke den saltede hashværdi som forventet. Fejlen sker ikke hvis man bruger crypt() med DES eller BLOWFISH. skrev:

Slap da af, hvor er det sort snak for os der ikke koder... :o)

#6

Det er vel ikke så overraskende at en tråd om et programmerings sprog er programmerings orienteret.

7#
Overhovedet ikke. Men stadig sort snak for mit vedkommende. Men jeg ser ingen grund til at det skulle omformuleres, hvis det giver mening for folk det er relevant for.

Det giver fint mening, desværre.. for det er en rigtig træls fejl.

Fejlen sker ikke hvis man bruger crypt() med DES eller BLOWFISH.

I første omgang havde jeg ikke skænket det nogen tanke hvilke ciphers der var nævnt som ikke værende berørt. Men nu jeg tænker lidt mere over det er det faktisk ganske uheldigt at det er de ciphers som ikke er berørt.

DES var historisk set den der blev brugt først. Det kan godt være det betyder det er den bedst testede, hvilket måske er medvirkende til at det ikke var der der smuttede en fejl igennem. Men DES er også den svageste. Den tillader kun passwords på op til 8 tegn, og den bruger kun syv bit fra hvert af de 8 tegn.

Begrænsningen på syv bits per tegn er ikke grund til bekymring. En maksimal længde på 8 tegn er grund til bekymring.

Jeg ved ikke hvordan Blowfish bliver brugt til at gemme passwords. Men da både DES og Blowfish er ciphers med 64 bits blokstørrelser kan det være de bruges på samme måde. Det ville betyde at den kunne håndtere passwords på op til 56 tegn. Men hvis der kun sammenlignes krypteringen af en enkelt salt værdi vil der være flere passwords der kan resultere i samme kryptering og dermed er sikkerheden begrænset af de 64 bits blokstørrelse. Blowfish kunne anvende 7-8 forskellige salt værdier og gemme en kryptering af hver, så ville den være ret sikker.

De fleste systemer valgte derimod at skifte fra DES til MD5 da de havde brug for mere sikkerhed, og sidenhen SHA1 og SHA2. Systemer der gør mest ud af sikkerheden har altså en tendens til at bruge SHA2 til passwords.

Det er uheldigt hvis en bug i en implementation af crypt rammer de ellers bedst sikrede og ikke de mindre godt sikrede.

Den lektie man skal lære af historien er selvfølgelig ikke at man skal holde sig fra SHA2 og bruge DES i stedet. Lektien er derimod at lave unit tests på sin kode. Der skulle ikke have været nogen ret omfattende test til at fange fejlen.

1. Tag et password og generer saltet værdi til at gemme.
2. Test at korrekt password sammen med streng genereret i skridt 1 vil blive accepteret.
3. Test at et forkert password sammen med streng genereret i skridt 1 vil blive afvist.

Ovenstående er vel omtrent det mindste nogen kunne have fundet på at skrive i en unit test til en password validering. Og det ville have været nok til at fange denne fejl. At fejlen slap igennem betyder nok at der overhovedet ingen unit tests var.

cazaadotdk (3) skrev:

#2 Are you married to Microsoft?

Næææh. Men hvis jeg fortrækker æbler fremfor appelsiner behøves det heller ikke at betyde at jeg er gift med et æble.
Så meget for din flamebait. Og tak!

Jeg læste på slashdot at fejlen ville være blevet fanget hvis de havde kørt deres regressionstests *før* de releasede. [citation needed]

also wall of text @kasperd

Som #10.. effin hell... unit tests anyone?
Srsly..

En anden interessant ting er vel at finde ud af hvilken ting man forsøgte at løse, siden at crypt() blev ændret... :P

Det er jo ikke fordi det ikke virkede førhen :P

#13

Well rettelsen der fik crypt til at gå i stykker var så vidt jeg kan se disse to:
http://svn.php.net/viewvc?view=revision&revision=314434
http://svn.php.net/viewvc?view=revision&revision=314438

Og det her er så hvor den bliver rettet, så det hele virker igen.
http://svn.php.net/viewvc?view=revision&revisi...

stas skrev:

Unbreak crypt() (fix bug #55439)
# If you want to remove static analyser messages, be my guest,
# but please run unit tests after

Så det har været i et forsøg på at nedsætte advarsler omkring potentiel forkert kode af en person der ikke var inde i hvad der blev lavet.

Får mig til at tænke på Debian's ssh key eventyr...

D_V (14) skrev:

Well rettelsen der fik crypt til at gå i stykker var så vidt jeg kan se disse to

Den første ændring der skifter strcat ud med strncat ændrer så vidt jeg kan se ikke på funktionaliteten. Så vidt jeg kan forstå på dokumentationen af strncat vil de to gøre nøjagtigt det samme.

Til gengæld kan jeg ingen dokumentation finde af strlcat. Så ændringen fra strncat til strlcat er muligvis et problem.

At reverte begge ændringer er dog under alle omstændigheder et naturligt valg.

D_V (14) skrev:

Så det har været i et forsøg på at nedsætte advarsler omkring potentiel forkert kode af en person der ikke var inde i hvad der blev lavet.

Det lyder som om de faktisk har haft unit tests, men at de ikke blev kørt.

Karma-- på den person der syntes at compiler warnings var vigtigere end unit tests.

Man bør selvfølgelig også spørge sig selv hvordan en ændring slap hele vejen igennem til release uden at blive fanget af unit tests.

Der er grundlæggende tre muligheder for at rette dette:
- Kræv at unit tests giver success før kode kan checkes ind.
- Kør periodiske automatiske unit tests på nyeste version i source control og send advarsler ud når det fejler.
- Kræv at unit tests køres som en del af release processen.

De tre muligheder udelukker ikke hinanden.

At kræve unit tests kører før checkin kan nemt være upraktisk. Og hvis checket køres på udviklerens maskine kan fejl i opsætningen af udviklerens maskine betyde at det kan checkes ind alligevel. Jeg har set tilfælde hvor en korrekt ændring ikke kunne checkes ind fordi det tidligere var lykkedes for en anden person at checke en ændring ind som unit tests ikke ville acceptere.

Periodiske check af source control er ganske praktisk. Hvis det samme system der kører disse unit tests også bygger et sæt filer som efterfølgende kan releases kan det i nogle tilfælde simplificere release processen og garantere at unit tests ikke fejler i releasede versioner. Dermed kan sådan et system sikre begge de to sidste punkter.

Periodiske kørsler af unit tests i source control med efterfølgende emails om fejl har den ulempe at det kan holde op med at virke uden at nogen opdager det. Systemet skal jo kun sende emails hvis noget fejler, så hvis det system er ude af stand til at sende emails vil ingen indse at der er et problem. Men hvis fejlen samtidigt betyder at der ikke er noget build at release vil man opdage fejlen når man prøver på at release det nyeste build.

kasperd (15) skrev:

Karma-- på den person der syntes at compiler warnings var vigtigere end unit tests.

Det var så Rasmus Lerdorf... Som det kan ses af de 2 checkins..

#unit tests

http://epixa.com/2011/08/how-php-is-broken-and-how...

Der er tilsyneladende kun 70% dækning af unit tests.

Men denne funktion er dækket.

Men med 192 unit tests som altid fejler kan man jo misse at der denne gang var 193.

Citater fra linket:

Even at only 70% code coverage, this bug was identified by the unit tests. The tests that were in place to make sure that bugs like this couldn't happen did their jobs — they failed when they were suppose to fail.

For this to happen, one of two things had to occur: someone released a stable version without running tests on newly added code, or, more likely, someone didn't notice that there was a really important test failing because PHP 5.3 has 192 failing unit tests!

arne_v (17) skrev:

Men med 192 unit tests som altid fejler kan man jo misse at der denne gang var 193.

At mange fejlende unit tests betyder at det bliver sværere at opdage når en alvorlig fejl er den ene grund til at det er skidt at have 192 unit tests der fejler i første omgang.

Men der er en anden grund til at det er et problem at der er 192 unit tests der fejler. Det er at hvis ikke lige det var fordi denne ændring introducerede en kæmpe bug, så var der ingen der ville have bekymret sig om at antallet af fejlende unit tests gik fra 192 til 193.

Selv en stigning fra 10 fejl til 1000 fejl kan nemt blive ignoreret, hvis den sker i små skridt. Det ville f.eks. kun kræve 18 checkins der hver øgede antallet af fejlende unit tests med 30% for at gå fra 10 til 1000.

Hvis unit tests skal give mening er man nødt til at se på det som et kæmpe problem hvis antallet af fejlende unit tests går fra 0 til 1.

I head og release branches skal antallet af fejlende unit tests holdes på 0. Selvfølgelig kan der ske smuttere. Men så skal det have førsteprioritet at få det rettet. Hvis først communitiet accepterer at en situation med mere end 0 fejlende unit tests, så er man på en glidebane der fører til dårligere software.

Det er ok at starte nye eksperimentelle branches hvor man kan udvikle på kode som skader eksisterende unit tests eller som indfører nye unit tests som ikke virker endnu. Men den slags skal selvfølgelig bringes i orden før det kommer i head eller et release branch.

Man kan argumentere for at 50% unit test coverage og ingen fejlende unit tests er et bedre udgangspunkt end 90% unit test coverage og en fejlende unit test som er blevet accepteret som værende måden man gør ting på.

Artiklen kan godt misforstås som mudderkastning. Og måske ville nogle af php udviklerne se dette indlæg som mudderkastning.

Jeg håber der er nogle personer som ser sig selv som en del af communitiet der udvikler php og tager afstand fra denne mudderkastning ved at lade være med at kommenterer på det. Jeg håber de personer i stedet beviser at de er i stand til at forbedre situationen og går i gang med målrettet at rette de 192 unit tests der i øjeblikket fejler. Jo flere unit tests man har rettet, jo mere vægt er der bag ordene når man kritiserer andre for at introducere nye fejl.

Hvis det lykkes dem at komme ned på nul fejlende unit tests så er de i en god position til at kræve at nye ændringer holder det der. Den nemmeste måde at øge unit test coverage på er ved at kræve at alt kode der ændres eller tilføjes er unit testet.

At skrive unit tests for gammel kode der allerede er checket ind er surt arbejde, som der nok ikke er nogen der gider gøre frivilligt med mindre de bliver betalt for det. Men skulle der dukke sådanne ændringer op, så skal de selvfølgelig bydes velkomne.

Når man retter en fejl er det en god idé at først skrive en unit test der ville have fanget fejlen og efterfølgende rette fejlen. Hvis man kræver en lignende fremgangsmåde for alle ændringer til eksisterende kode, så vil unit test coverage langsomt gå op. Men man skal ikke fokusere for hårdt på det før man er nede på nul fejlende unit tests.

Php communitiet kan komme ud af denne episode styrket, men det kommer an på hvordan de håndterer situationen. Lad os se om tre måneder hvor stort unit test coverage de har og hvor mange der fejler. At de overhovedet har unit tests er bedre end mange open source projekter.

#18

Jeg er meget enig med bloggeren og dig.

Det enste acceptable antal fejl i unit tests for software der releases er nul.

Og det virker lidt uprofessionelt når nogen af dem der kommenterer på bloggen bliver fornærmet over at få det påpeget.