patrick mylund nielsen bloghoved

Apple's SSL-brøler

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:

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
  OSStatus        err;
   ...
 
   if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
      goto fail;
 if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
      goto fail;
     goto fail;
 if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;
 ...
 
fail:
 SSLFreeBuffer(&signedHashes);
  SSLFreeBuffer(&hashCtx);
   return err;
}

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."

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?

Kommentarer (34)
sortSortér kommentarer
  • Ældste først
  • Nyeste først
  • Bedste først
Peter Mogensen

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.

  • 12
  • 0
Kristian Rastrup

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.

  • 1
  • 1
Peter Mogensen

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:

{  
 //...  
   if (bad)  
      goto fail;  
   //...  
   if (also bad)  
      goto fail;  
   
   /* success */  
   return success;  
   
fail:  
   assert (err != succes);  
   return err;  
}

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.

  • 3
  • 1
Kasper Henriksen

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).

  • 4
  • 0
Kristian Rastrup

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:

static OSStatus  
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,  
                                 uint8_t *signature, UInt16 signatureLen)  
{  
    OSStatus        err;  
    SSLBuffer       hashOut, hashCtx, clientRandom, serverRandom;  
    uint8_t         hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];  
    SSLBuffer       signedHashes;  
    uint8_t            *dataToSign;  
    size_t         dataToSignLen;  
   
    signedHashes.data = 0;  
    hashCtx.data = 0;  
   
    clientRandom.data = ctx->clientRandom;  
    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;  
    serverRandom.data = ctx->serverRandom;  
    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;  
   
   
    if(isRsa) {  
        /* skip this if signing with DSA */  
        dataToSign = hashes;  
        dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;  
        hashOut.data = hashes;  
        hashOut.length = SSL_MD5_DIGEST_LEN;  
   
        if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)  
            goto fail;  
        if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)  
            goto fail;  
        if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)  
            goto fail;  
        if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)  
            goto fail;  
        if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)  
            goto fail;  
    }  
    else {  
        /* DSA, ECDSA - just use the SHA1 hash */  
        dataToSign = &hashes[SSL_MD5_DIGEST_LEN];  
        dataToSignLen = SSL_SHA1_DIGEST_LEN;  
    }  
   
    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;  
    hashOut.length = SSL_SHA1_DIGEST_LEN;  
    if ((err = SSLFreeBuffer(&hashCtx)) != 0)  
        goto fail;  
   
    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)  
        goto fail;  
    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)  
        goto fail;  
    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)  
        goto fail;  
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)  
        goto fail;  
        goto fail;  
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)  
        goto fail;  
   
    err = sslRawVerify(ctx,  
                       ctx->peerPubKey,  
                       dataToSign,             /* plaintext */  
                       dataToSignLen,          /* plaintext length */  
                       signature,  
                       signatureLen);  
    if(err) {  
        sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "  
                    "returned %d\n", (int)err);  
        goto fail;  
    }  
   
fail:  
    SSLFreeBuffer(&signedHashes);  
    SSLFreeBuffer(&hashCtx);  
    return err;  
   
}

Kilde http://opensource.apple.com/source/Security/Security-55471/libsecurity_s... en tredjedel nede

  • 2
  • 0
Robert Larsen

Goto er meget ofte brugt til fejlhåndtering til til at springe til slutningen af en funktion fra mange steder:
* http://en.wikipedia.org/wiki/Goto#Common_usage_patterns_of_Goto

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 ;)
int fun(int a)  
{  
    int result = 0;  
    char *buffer = kmalloc(SIZE);  
   
    if (buffer == NULL)  
        return -ENOMEM;  
   
    if (condition1) {  
        while (loop1) {  
            ...  
        }  
        result = 1;  
        goto out;  
    }  
    ...  
out:  
    kfree(buffer);  
    return result;  
}

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

  • 2
  • 0
Thomas Dynesen

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.

  • 1
  • 2
Peter Mogensen

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):

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

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

  • 1
  • 2
Lasse Hillerøe Petersen

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)...

  • 0
  • 0
Povl H. Pedersen

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.

  • 1
  • 17
Jesper Jepsen

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.

  • 8
  • 1
Michael Nielsen

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.

  • 2
  • 1
Jacob Pind

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

  • 1
  • 0
Lars Balker

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):

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.

  • 2
  • 0
Casper Thomsen

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.

  • 2
  • 0
Jacob Christian Munch-Andersen

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.

  • 2
  • 0
Peter Mogensen

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

Jeg mobber skam Perl i al venskabelighed ... og jeg kender slet ikke Ruby godtnok til at drille det.
Perl udgør stadig en ikke ubetydelig del af min dagligdag og hvis jeg endelig skulle finde noget ved Perl at blive oprigtig irriteret over, så var det noget helt andet.

  • 2
  • 0
Jakob Damkjær

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.

  • 0
  • 3
Patrick Mylund Nielsen

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.

  • 2
  • 0
Log ind eller Opret konto for at kommentere
IT Company Rank
maximize minimize