Dette indlæg er alene udtryk for skribentens egen holdning.

Apple's SSL-brøler

24. februar 2014 kl. 06:5834
Artiklen er ældre end 30 dage
Manglende links i teksten kan sandsynligvis findes i bunden af artiklen.

I fredags udgav Apple en opdatering til iOS, 7.0.6, der retter en fejl i deres SSL-kode, som lader hvem som helst med adgang til din forbindelse lytte med på eller manipulere (næsten) alle dine SSL-forbindelser -- tilmed dem, der bruger certificate pinning.

(Hvis du ikke allerede har opdateret, bør du opdatere alle dine iOS-enheder til 7.0.6. OS X 10.9 er også påvirket, men der er endnu ingen opdatering tilgængelig. Google Chrome og Firefox er ikke berørt, da de ikke bruger Apple's SecureTransport-API.)

Det viser sig at deres kode, som er open source, i lang tid ikke faktisk har tjekket at et SSL-"handshake" er gyldigt, dvs. ikke har bekræftet at serveren der kommunikeres med faktisk har den private nøgle til det certifikatet, der bliver præsenteret, så hvem som helst kan lade som om de er eksempelvis mail.google.com.

Fejlen skyldes hvad der ligner en copy-paste-ommer:

  1. static OSStatus
  2. SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
  3. uint8_t *signature, UInt16 signatureLen)
  4. {
  5. OSStatus err;
  6. ...
  7.  
  8. if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
  9. goto fail;
  10. if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
  11. goto fail;
  12. goto fail;
  13. if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
  14. goto fail;
  15. ...
  16.  
  17. fail:
  18. SSLFreeBuffer(&signedHashes);
  19. SSLFreeBuffer(&hashCtx);
  20. return err;
  21. }

Den duplikerede "goto fail;" får funktionen til at returnere uden fejl før det mest kritiske tjek.

En uskyldig fejl eller NSA-bagdør?

Der er meget spekulation om årsagen til fejlen. Nogle mener at det er lige lovlig underligt at NSA en måned efter fejlen blev introduceret producerede PowerPoint-slides, der viste at "Apple var deres nyeste partner" og at "de havde 100% succesrate på iPhone."

Artiklen fortsætter efter annoncen

Det er nok bare en uskyldig fejl, men hvorfor er der ingen der fangede den? Statisk analyse? Unittests? Code review? Hvad ville have hjulpet mest?

34 kommentarer.  Hop til debatten
Denne artikel er gratis...

...men det er dyrt at lave god journalistik. Derfor beder vi dig overveje at tegne abonnement på Version2.

Digitaliseringen buldrer derudaf, og it-folkene tegner fremtidens Danmark. Derfor er det vigtigere end nogensinde med et kvalificeret bud på, hvordan it bedst kan være med til at udvikle det danske samfund og erhvervsliv.

Og der har aldrig været mere akut brug for en kritisk vagthund, der råber op, når der tages forkerte it-beslutninger.

Den rolle har Version2 indtaget siden 2006 - og det bliver vi ved med.

Debatten
Log ind eller opret en bruger for at deltage i debatten.
settingsDebatindstillinger
33
26. februar 2014 kl. 00:15

med 10.9.2 er SSL issuet nu lukket og det krævede trods alt at man benyttede SSl og ikke TLS 1.2 (som ikke var berørt) og at angriberen havde en priviligetet status på det lokalnetværk man sad på. Så hvis man nu var paranoid eller bare ville ha hurtigere internet end cafeens wifi (der ofte er meget langsomere end LTE) så kunne man jo bare slå wifi fra og koble ens LTE dims på sin computer via USB og så skulle dem med sorte hatte trods alt hacke ind hos et teleselskab for at få adgang til netværket.

Så man må sige at turnaround ikke har været specielt langsom specielt da 10.9.2 ikke er verdens mindste opdatering og der skal trods alt chekkes en del ting før en så stor release kan frigives fra QA.

34
26. februar 2014 kl. 00:20

Det er ligegyldigt hvad serveren faktisk bruger, om det er TLS 1.2 eller SSLv2. Hvis jeg er MITM kan jeg sige at jeg kun understøtter TLS <= 1.1, hvorefter Apple-klienten ville godtage mit ugyldige certifikat.

Det er godt, problemet er løst, men det er lidt irriterende at de var lang tid om at inkludere det i en 450MB opdatering, der tager ~30 mins at installere. Det kunne sagtens have været en lille opdatering, der blev udgivet på samme tid med iOS-opdateringen og bare løste det problem.

29
24. februar 2014 kl. 17:20

Det er et spørgsmål om vane og holdning om ekstra krøllede parenteser gør koden mere læsbar eller ej. Jeg kan sagtens argumentere for begge dele; og indtil nogen fremviser dokumentation for at {} er gode eller dårlige og kvantificerer det, betragter jeg det som en holdning. I don't give a shit.

Ethvert review af funktionen som helhed vil have fanget den bøf. Det springer lige op i mit ansigt at der er en ekstra linje. Men der er flere ting der springer i mine øjne ved 2 minutters kig: l. 4 (missing asserts), l. 9-72 (indent), l. 21 (spacing efter "if"), l. 49 (tom), l. 28-37 (timing), l. 50-60 (timing), l. 58 (...), l. 69-70 (syntax). Det er vel 12 år siden jeg har skrevet mere end 10 linjer C om måneden i snit; kompetente folk kan med garanti gøre det meget bedre.

Jeg vil gerne se en git blame mv. over den.

30
24. februar 2014 kl. 22:13

Men der er flere ting der springer i mine øjne ved 2 minutters kig: l. 4 (missing asserts), l. 9-72 (indent), l. 21 (spacing efter "if"), l. 49 (tom), l. 28-37 (timing), l. 50-60 (timing), l. 58 (...), l. 69-70 (syntax).

Og hvis vi også kigger på resten af koden i filen tror jeg vi har et svar på hvorfor fejlen før nu hverken er fundet af mennesker eller automatiske review værktøjer, i begge tilfælde drukner den i alle mulige småting som ingen har taget sig af.

27
24. februar 2014 kl. 16:45

clang har ikke -Wunreachable-code som default med i -Wall, så deres alm compiler suit der kan ikke hjælp dem, og med gcc Mja den har slet ikke -Wunreachable-code mere, den gør ingen ting

25
24. februar 2014 kl. 15:12

Jeg synes kodekonventionen om ALTID at have curly brackets efter if, while og alle de andre fine konstruktioner er ganske undervurderet og ville have hindret denne lille upser.

Det kan sgu godt være det fylder lidt mere, men læsbarheden øges og risikoen for fejl mindskes.

26
24. februar 2014 kl. 16:33

Vi kan sagtens starte en lille religiøs krig om curly brackets og gotos, men ærligt talt tror jeg mere, der er tale om hastværk og manglende kodereview. Det er faktisk en ganske iøjnefaldende fejl.

22
24. februar 2014 kl. 14:52

Apple fixer i det mindste fejlen. Hvor er der ikke flere artikler om fejl i Android som ikke rettes, ihvertfald ikke på de telefoner der er solgt ? Man får hele tiden at vide, at Apple er en lille ubetydelig nicheplayer, og Android de eneste med deres store markedsandel.

Mig bekendt har over 50% af alle Android devices sikkerhedshuller, heraf også nogle der er exploit kits til, og som er lettere at udnytte. Men fokus går på at svine den lille ubetydelige spiller til. Er Android telefoner er køb og smid væk. Det virker nærmest som tåbeligt at man sætter flash-RAM i dem i stedet for bare en masked ROM når de nu ikke opdateres alligevel.

Hvorfor er der heller ingen kritik at at sikkerhedshuller der er så store at Microsoft udsender out-of-band fixes ikke engang kommer med i Windows update ? Men straks Apple kommer med en bugfix der skubbes ud til alle, så hagler det ned over dem.

Er iOS og OS-X virkerligt så meget mere betydningsfuldt en Android og Windows ? Er alle ligeglade med de 2 store platforme ? Det vil da ramme flere læsere at skrive om det.

Eller er problemet, at man går TV3 style. Læserne/seerne må ikke få fornemmelsen af at der findes klogere valg end de valg de har truffet ? Så kan læseren med Android føle sig selvfed. Ligesom når han ser Amalie eller Linse Kessler på TV3 og tænker at der da er nogen der ikke er væsentligt mere intelligente end ham.

24
24. februar 2014 kl. 15:02

En grund kunne være at Apple ser sig selv om fejlfri ud fra deres opførsel når der påpeges fejl i deres produkter så er det brugeren der anvender dem forkert for de har ikke lavet fejl. Når man som Apple gør alt for at fremstå med et fejlfrit produkt alle kan bruge og kræver høje priser ja så falder man det tungere end andre. Desuden får MS også ganske store hak når der findes større fejl i deres systemer, vi er bare mere vant til det sker og MS påstår ikke at de aldrig laver fejl.

18
24. februar 2014 kl. 13:05

Siden W98 har der været meget stærk fokus på sikkerhed. At forestille sig at dette ikke også er tilfældet hos Apple er utænkeligt. Dette er central sikkerhedskode. Man tester ved at tjekke at den accepterer gyldige certifikater og at den afviser ugyldige certifikater. At koden ikke skulle være gået igennem denne simple test er helt utænkeligt. Den blev testet og man fandt at den opførte sig efter hensigten.

17
24. februar 2014 kl. 12:39

Må jeg spørge om noget naivt: Er der ikke unittest med code coverage-beregning for noget så centralt kode?

16
24. februar 2014 kl. 12:37

Hvad med den ikke-tekniske brøler? At glemme at patche OSX før offentliggørelsen..

15
24. februar 2014 kl. 12:35

Er en god code convention at all blokke skal pakkes ind i {}, ligesom i Perl (og det var vist saa det eneste gode der nogensinde kom ud af Perl).

20
24. februar 2014 kl. 13:18

Er en god code convention at all blokke skal pakkes ind i {}, ligesom i Perl (og det var vist saa det eneste gode der nogensinde kom ud af Perl).

Det udsagn kunne have været sandt hvis ikke det lige var fordi Perl faktisk overloader {} som hash literals. ... så den nogen gange er i tvivl om hvad {} faktisk betyder. (med mindre man gør det eksplicit).http://www.perlmonks.org/?node_id=1031629

I øvrigt kræver Perl ingen {} block hvis man bare vender sætningskonstruktionen om det til meget mere læsbare (ironi kan forekomme):

[perl]goto fail if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)[/perl]

De færreste sprog lader folk kode med hovedet under armen. - hvilket Apple nok har måtte sande.

28
24. februar 2014 kl. 16:46

Dvs. man behøves ikke {} omkring, hvis man bare har en enkelt linie. (en fejl folk, der koder for meget Perl nemt laver når de vender tilbage til C).

Ikke forstået. I Perl kan man da netop ikke udelade {}.

Det udsagn kunne have været sandt hvis ikke det lige var fordi Perl faktisk overloader {} som hash literals. ... så den nogen gange er i tvivl om hvad {} faktisk betyder. (med mindre man gør det eksplicit).

Stråmand. Det er jo slet ikke relevant for almindelige if-blokke.

I øvrigt kræver Perl ingen {} block hvis man bare vender sætningskonstruktionen om det til meget mere læsbare (ironi kan forekomme):</p>
<p>goto fail if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)

Jeg kan replikere med en Ingen Sand Skotte til din Stråmand:

Just because you CAN do something a particular way doesn't mean that you SHOULD do it that way.
[...]
because the second way hides the main point of the statement in a modifier

Fra perlstyle(1).

Er en god code convention at all blokke skal pakkes ind i {}, ligesom i Perl (og det var vist saa det eneste gode der nogensinde kom ud af Perl).

Buksevand ved lejlighed, Jacob.

PS: Er det ikke lidt umoderne at mobbe Perl efterhånden; følg nu lidt med tiden, og dril Ruby i stedet.

19
24. februar 2014 kl. 13:05

Ja denne fejl ville være opdaget med det samme hvis man ikke var doven/smart og udlader {} ved conditions (if, else, for, while etc) statements.

8
24. februar 2014 kl. 09:48

Jeg gætter på en merge konflikt hvor man har været lidt for rundhåndet med hvad man har picket.

4
24. februar 2014 kl. 08:40

Er der nogle der kan forklare mig, hvorfor compileren ikke finder ud af at der er kode der aldrig vil blive eksekveret - ligesom det f. eks. ville være tilfældet i C#?

6
24. februar 2014 kl. 09:19

En god grund til at bruge andre værktøjer end bare compileren, til at checke sin software. Den var ikke gået med vores C toolchain...

1
24. februar 2014 kl. 07:54

Hvorfor bruger de overhovedet goto? Dijkstras artikel om samme er 45 år gammel.

3
24. februar 2014 kl. 08:29

Hvorfor bruger de overhovedet goto?
Dijkstras artikel om samme er 45 år gammel.

Så vidt jeg forstår Dijkstras pointe, så er det at goto gør det meget svært at vide hvordan du er kommet til et bestemt punkt i eksekveringen. Ikke hvordan du kommer derfra. Mao. Det er kun et problem at springe ind i scope - ikke ud. Og den brug af goto, der er her er ikke meget anderledes end at have flere steder der forekommer er return statement i en funktion.

Næe... problemet her er ikke goto. Problemet er ikke at have styr på sine scopes. ... og det er tydeligvis godt hjulpet på vej af C's princip om at det, der følger efter if/else er et statement og ikke en block. Dvs. man behøves ikke {} omkring, hvis man bare har en enkelt linie. (en fejl folk, der koder for meget Perl nemt laver når de vender tilbage til C). Indrykningen har så skjult det faktum for programmøren.

Du kunne f.eks. have lavet en tilsvarende fejl med ikke at have styr på udstrækningen af dit scope (uden goto) i et hvilket som helst sprog med whitespace scoping. Som f.eks. python.

7
24. februar 2014 kl. 09:39

Problemet er også Goto, der her er brugt til at emulere exceptions i C. Men der mangler sporbarhed som stacktrace. Det er ikke til at se når vi står i fail: hvad der er årsag til fejlen og derfor er det heller ikke muligt at vide at vi er der af en forkert årsag. Hvis man havde brugt en fejlparameter kunne vi tjekke på den for at se hvor vi kom fra og erkende at vi var endt i fejlhåndtering uden at der var triggeret en fejl. Læg også mærke til at fail: også er del af standard eksekvering i metoden så vi har ikke en separat fejlhåndtering (catch clause). Desuden er SSL kryptering svær nok at forstå som det er, man behøver ikke gøre det sværere for programmøren med hoppen rundt i koden.

10
24. februar 2014 kl. 10:25

Problemet er også Goto, der her er brugt til at emulere exceptions i C.

Hvad vil du så gøre i stedet? Bruge setjmp og longjmp, som er helt umulige at gennemskue?

For i øvrigt, så er der endnu en ting, ved den her kode, der forekommer mig suspekt. Det ser ud som om den kunne være sårbar overfor timingangreb, da den forbipasserer alle efterfølgende tests såfremt een test fejler. Al kryptografisk kode bør tage den samme tid per funktionskald. Se f.eks. Boneh & Brumley (2003).

9
24. februar 2014 kl. 10:16

Hvis man havde brugt en fejlparameter kunne vi tjekke på den for at se hvor vi kom fra og erkende at vi var endt i fejlhåndtering uden at der var triggeret en fejl.

Det burde fremgå af kode at "err" ikke var sat. Og ja - "fail" er en del af standard code path, men det du kritiserer der er fejl-håndteringen - ikke goto. Man kunne sagtens have lavet den fejl-håndtering du snakker hvor man aldrig kommer til "succes" via goto, men stadig bruger goto. Det kræver kun en lille ændring:

  1. {
  2. //...
  3. if (bad)
  4. goto fail;
  5. //...
  6. if (also bad)
  7. goto fail;
  8.  
  9. /* success */
  10. return success;
  11.  
  12. fail:
  13. assert (err != succes);
  14. return err;
  15. }

Men nu har C jo altså ikke exception handling, så dit argument ender jo hurtigt som et argument for hvorfor exception handling er en god ting og "C considered harmfull". ... og det kan der jo være meget sandhed i. Men nu er det jo altså et C program.

12
24. februar 2014 kl. 10:35

Sådan er det formodenligt lavet. Det er tydeligvis kun et udsnit koden der er vist, afgrænset af "..." før og efter.

Desværre ikke, her er den fulde metode:

  1. static OSStatus
  2. SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
  3. uint8_t *signature, UInt16 signatureLen)
  4. {
  5. OSStatus err;
  6. SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
  7. uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
  8. SSLBuffer signedHashes;
  9. uint8_t <em>dataToSign;
  10. size_t dataToSignLen;
  11.  
  12. signedHashes.data = 0;
  13. hashCtx.data = 0;
  14.  
  15. clientRandom.data = ctx->clientRandom;
  16. clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
  17. serverRandom.data = ctx->serverRandom;
  18. serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
  19.  
  20.  
  21. if(isRsa) {
  22. /</em> skip this if signing with DSA <em>/
  23. dataToSign = hashes;
  24. dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
  25. hashOut.data = hashes;
  26. hashOut.length = SSL_MD5_DIGEST_LEN;
  27.  
  28. if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
  29. goto fail;
  30. if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
  31. goto fail;
  32. if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
  33. goto fail;
  34. if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
  35. goto fail;
  36. if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
  37. goto fail;
  38. }
  39. else {
  40. /</em> DSA, ECDSA - just use the SHA1 hash <em>/
  41. dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
  42. dataToSignLen = SSL_SHA1_DIGEST_LEN;
  43. }
  44.  
  45. hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
  46. hashOut.length = SSL_SHA1_DIGEST_LEN;
  47. if ((err = SSLFreeBuffer(&hashCtx)) != 0)
  48. goto fail;
  49.  
  50. if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
  51. goto fail;
  52. if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
  53. goto fail;
  54. if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
  55. goto fail;
  56. if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
  57. goto fail;
  58. goto fail;
  59. if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
  60. goto fail;
  61.  
  62. err = sslRawVerify(ctx,
  63. ctx->peerPubKey,
  64. dataToSign, /</em> plaintext <em>/
  65. dataToSignLen, /</em> plaintext length */
  66. signature,
  67. signatureLen);
  68. if(err) {
  69. sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
  70. "returned %d\n", (int)err);
  71. goto fail;
  72. }
  73.  
  74. fail:
  75. SSLFreeBuffer(&signedHashes);
  76. SSLFreeBuffer(&hashCtx);
  77. return err;
  78.  
  79. }

Kilde http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c en tredjedel nede

21
24. februar 2014 kl. 14:48

Er det bare mig der ikke ved hvordan man koder, eller kunne den sidste kæde af if-sætninger reduceres betragteligt, ved at fjerne goto fail (og derved neste alle if-sætningerne i hinanden) og så invertere betingelsen til == 0?

Derudover vil jeg mene at fejlen burde være blevet fanget af selv en middelmådig compiler - selv Apples gamle MPW C gav advarsler ved unreachable statements dengang jeg brugte det tilbage i 90erne, så vidt jeg husker (men min hukommelse er selvfølgelig rusten)...

14
24. februar 2014 kl. 11:56

Desværre ikke, her er den fulde metode:

Ok. Men err bliver sat betingelsesløs (hvis ikke det var pga af fejlen) og det giver samme effekt. Da koden under fail lablen blot er oprydning der altid skal udføres og ikke fejlhåndtering. (man kan så diskuttere om fail er rette navn på label eller den burde hedde noget i stil med cleanup eller finally).

13
24. februar 2014 kl. 11:12

Goto er meget ofte brugt til fejlhåndtering til til at springe til slutningen af en funktion fra mange steder:

Og Linux kernens kode convention siger (beklager...man kan ikke have code tags under quote tags):

	Chapter 7: Centralized exiting of functions

Albeit deprecated by some people, the equivalent of the goto statement is used frequently by compilers in form of the unconditional jump instruction.

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done. If there is no cleanup needed then just return directly.

The rationale is:

  • unconditional statements are easier to understand and follow
  • nesting is reduced
  • errors by not updating individual exit points when making modifications are prevented
  • saves the compiler work to optimize redundant code away ;)
    1. int fun(int a)
    2. {
    3. int result = 0;
    4. char *buffer = kmalloc(SIZE);
    5.  
    6. if (buffer == NULL)
    7. return -ENOMEM;
    8.  
    9. if (condition1) {
    10. while (loop1) {
    11. ...
    12. }
    13. result = 1;
    14. goto out;
    15. }
    16. ...
    17. out:
    18. kfree(buffer);
    19. return result;
    20. }

Kilde: https://www.kernel.org/doc/Documentation/CodingStyle

2
24. februar 2014 kl. 08:16

Lige i den stump kode giver det faktisk mening og gør at det hele er meget nemmer at læse. "Dogmas are also considered harmful".