Original URL: http://www.theregister.co.uk/2014/05/01/stob_bleeding_heart/

Today's bugs have BRANDS? Be still my bleeding heart [logo]

Code-slinger Verity reviews the rash of groovy-named open-source security vulns

By Verity Stob

Posted in Verity Stob, 1st May 2014 07:02 GMT

Stob In view of the ongoing security-holed far-too-open source situation, I have decided to convene an emergency meeting of ERCOCC, the El Reg Committee Of Competent Coders, to review what has occurred and how we should go forward.

No time for chitter-chattering. Settle down everybody, and juice up the PowerPoint projector please Eric.

Bug #1: Goto Fail

The last time I spotted a goto in the wild was about 15 years ago. It was embedded in a C++ date-time class I had lazily poached from a certain code-sharing site, where the file had been made available under the controversial WYGIWYD licence (What You Get Is What You Deserve).

That use of goto was quite gratuitous. It shone in the source out like the bright colourings adopted by certain venomous insects to warn birds not to eat them. And, like the stupidest, freshly-fledged squab, I inhaled the wasp and endured the consequences. Needless to say, it would have been quicker and more fun to copy out Letts 2002 Diary for the Dimmer Bulb by hand.

Because nobody uses goto in real code, right? Sure, back in the day, Donald Knuth - not an obvious Nigel Farage-stylee hero of reactionary principles - bashed out a quick paper in its defence. But that was in 1974, when digital watches were still a pretty neat idea, smoking fags was both funny and clever and certain Top of the Pops presenters blah-di-blah ugh.

So - and I'm sure you got here before me - when the Goto Fail SSL kerfuffle occurred, you could have smacked my gob into last Tuesday with a bent feather. Naturally I clicked through the links to the C file in question: go here and scroll down to SSLVerifySignedServerKeyExchange to see for yourself, or here for a grown up analysis.

Or, to save time and excess clickage, take a gander at this dramatic reconstruction. See if you can spot that problematic line:

OSStatus SSLVerifySignedServerKeyExchange(/*loadsa params*/)
{
    /* Declare some local variables, including: */
    SSLBuffer       signedHashes;
    /* More mucking about with local variables, eg */
    signedHashes.data = 0;
    /* Then the main rhythm of the function: */
    if ((err = SSLImportantManInTheMiddleDefence(hashCtx, otherStuff)) != 0)
        goto fail;
    if ((err = SSLBitTwiddlingBogeymanBaffling(whatever)) != 0)
        goto fail;
    if ((err = SSLTumTittyTumTum(tum)) != 0)
        goto fail;
    if ((err = SSLNotForYourTinyBrainYouEarthling(params)) != 0)
        goto fail;
    if ((err = SSLAnotherFunctionYouDontUnderstand(hohoHo)) != 0)
        goto fail;
    /* ... */
    /* After many more lines of the same ilk: */
    if ((err = SSLYadaYadaYada(stuff)) != 0)
        goto fail;
        goto fail;
    if ((err = SSLDoTheActualImportantCheck(stuff)) != 0)
        goto fail;
fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}

With its repeated goto fail refrain, the code's structure seems familiar. Ah yes, I know: the bawdy song Three German Officers crossed the Rhine. Substitute 'goto fail' for 'parlez-vous' and you can practically whistle it:

if err equals ess ess ell tum-tee-tum
  goto fail,
if err equals ess ess ell tum-tee-dum
  goto fail...

The repeated goto is like the phrase "Simon says" in a game of "Simon says": it exists only to trip you up. It makes for a bored, twitchy style.

Programmers shouldn't be inflicting boring, repetitive tasks on themselves. That's why they make friends with computers.

And this goto just isn't necessary. Despite the non-comittal line taken by this page, there are better ways to do it.

For example, the code maintainer could simply drop the gotos and code the whole lot with nested ifs. Sure, he would likely want to refactor the function into smaller pieces, to avoid ending up with his logic uncomfortably cramped up against the right hand margin, like the face of a hungry child squished against a restaurant window in a heart-warming movie. But so what? This program could use a little extra reorganisation.

Or - and this is Baby's favoured solution - he could just turn on the C++ switch of that great big compiler he has there, and protect his resources with a bit of RAII. With that in place, his ghastly gotos become happy returns.

Reader: Is there any other feature to which you would wish to draw my attention?

Stob: To the curious incident of the signedHashes variable in the body of the function.

Reader, gamely playing along: But the signedHashes variable does nothing in the body of the function.

Stob, triumphantly: That is the curious incident.

Bug #2: GnuTLS

From Apple's woes to old Gnus. Again, you will find an excellent analysis of the GnuTLS bug here; but for those of you who can't be bothered to click through, here again is the mystery voice with a summary of the C code in question:

/* Original code comment: this function returns a boolean */
/* VS meta-comment: oh yeah? */
int check_if_ca (/*loadsa params*/)
{
    int result;
    /* Bit of initialisation, then we get to business: */
    result = _gnuls_x509_underscores_are_us(params);
    if (result < 0) {
        /* left out the assert() here */
        goto cleanup;
    }
    result = _bit_twiddling_bogeyman_baffling(more);
    if (result < 0) {
        goto cleanup;
    }
    result = _zippity_doo_da(yet_more);
    if (result < 0) {
        goto cleanup;
    }
    /* ... */
    /* After dozens more lines in this style, we come to: */
    result = 0; /* ie false - VS */
cleanup:

    /* Call _gnutls_free_datum() a few times */
    /* Nice use of the singular 'datum' by the way */
    return result;      
}

You will have spotted that this code uses the same inky-dinky-goto-vous pattern as the previous sample. In mitigation, the author at least names his label "cleanup" and not "fail" - he has a sunnier perspective on life. Also, because of the extra curly brackets, this sample is not so vulnerable to an accidentally repeated goto line rendering swathes of logic unreachable.

He has, however, fallen foul of a very C-ish problem. As you know, C functions - especially library functions - very often return an integer error code. Because there are typically many different ways to get it wrong and only one to get it right, and because one can conveniently test for zero using the ! logical not operator, a much-used API convention is to return 0 if all went well and an error code otherwise.

So you end up writing code like this:

if (!conquer_earth()) {
  /* go hollow out the Earth's core */

where the logic appears to read face-after-fundament:

if the Earth is NOT conquered then
    Hollow out its core. HoldOnAMinuteWtf?

The fix is to have your function instead return boolean true on success and false on failure. This is what Matey has done here, or rather what he says he has done in his comment. Unfortunately, the comment is as far as he got before falling asleep. He didn't even bother to change the return type of his function to bool... oh wait, he's writing in C, so he couldn't. Bummer.

Now, had he been writing in C++, I claim he would have started by fixing his return type. Had he once again fallen prematurely into the arms of Morpheus, on his next compile, he would have got a message like

warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)

(OK, that is the Microsoft C++ compiler's opinion, but it is what I have to hand, ok?)

This message amounts to a polite cough in a situation that calls for a yell of "What gives, moron?" - but for that sort of thing we'd to need to rewrite in something like Pascal, which I recognise is an event comfortably beyond the infinite improbability horizon. But the act of writing a little logic to suppress that tiny squeak of C++ protest would surely have awakened our dreamy programmer from his slumber at the wheel.

Bonus observation: you can go inspect the now-repaired code here. You'll see that the chosen solution is to add another label called (can you guess?) fail.

fail:
result = 0; /* Means boolean false */
cleanup:
// Do datum clean up stuff...

This change guarantees that all the intertwined code paths through this function perform at least one goto. Well done. With this touch, the fixer has performed an act of alchemy: he has turned rubbish into spaghetti.

Bug #3: Heartbleed

Heartbleed is the first bug of my experience to come with its own  logo. If this catches on, the designer fees alone will soon send the most agile development cycle spiralling out of control.

The pithiest explanation of Heartbleed is xkcd's; a detailed investigation can be found here.

Needless to say, it is a fragment of C code that causes the trouble. However, unlike the previous efforts, I would say that this is proper C code, in the sense that it is full of the clatter of pointer arithmetic and chatter of the in-place post-increment operator. Atari BASIC could not cut this mustard.

By all means have a look; the action all takes place around line 2400.

With a crunching noise, an in-place macro pseudo-function bites off the length target buffer and hands the result to malloc(). A swift memcpy() and some random bytes piped on top by way of decoration and before you have had time to think 'I don't much care for this bracing style' whoosh! it's all done, with the smoothness and grace of a Bruce Forsyte fresh from an invigorating massage. To complain that it has just sent over a few clear text passwords and a brace of private keys, flotsam caught up in the mesh of an oversized buffer, feels like cavilling.

But RFC 6520 Section 4 says If a received HeartbeatResponse message does not contain the expected payload, the message MUST be discarded silently. So, there.

And I hope you will forgive my banging on about this, but if it were written in C++, it would be easier to arrange things so that, for example, transmission buffers automatically checked the size of data they were supposed to be copying. Just saying.

The Flensing of C

The Heartbleed fiasco and its antecedents have cast doubt on the famous golden ratio of eyeball to bug that open source allegedly offers.

Perhaps because I find cryptographic code deeply dull, I have always been sceptical of this claim. It seems to me that the only folks properly motivated to pore over the inner details of SSL are those naughtily looking for a vulnerabilities. Everybody else just mutters 'looks ok to me' and carries on squeezing their blackheads.

But now that I have myself invested nearly quarter of an hour of personal eyeball time with this supposedly well-inspected code, I have come to a different conclusion. No bug is shallow if it lives in a bug-camouflaging environment. It really is time that C was abandoned as the automatic choice for this work. Its use just sets up these failures.

OpenBSD is essaying a grand rewrite of OpenSSL called LibreSSL. They say their first task is to 'flense' the code. If this were to include using C++ to get a teensy bit of type safety, and stopping the antediluvian practice of depending on goto statements for resource protection, then I would overlook the ghastly verb flense in the expectation that our electronic secrets could enjoy a little more privacy in their cloudy nests. ®