a brief history of one line fixes

Apple recently made a booboo, unlike any other booboo in the history of programming. Even though Apple’s bug is unprecedented, here’s a brief overview of some predecessor bugs.


Back in 2006, the X server checked to make sure the user was root, but forgot to actually call the function.

--- hw/xfree86/common/xf86Init.c +++ hw/xfree86/common/xf86Init.c @@ -1677,7 +1677,7 @@ } if (!strcmp(argv[i], "-configure")) { - if (getuid() != 0 && geteuid == 0) { + if (getuid() != 0 && geteuid() == 0) { ErrorF("The '-configure' option can only be used by root.\n"); exit(1); }

How is this possible? Does nobody use a compiler that warns about comparisons always being false?

Debian OpenSSL

Remember that time back in 2008 when Debian shipped a special limited edition OpenSSL? “As a result, cryptographic key material may be guessable.”

--- openssl-a/md_rand.c +++ openssl-b/md_rand.c @@ -271,10 +271,7 @@ else MD_Update(&m,&(state[st_idx]),j); -/* - * Don't add uninitialised data. MD_Update(&m,buf,j); -*/ MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c)); MD_Final(&m,local_md); md_c[1]++;

OK, I’m cheating here, it’s a three line fix. How is this possible? Does nobody read the OpenSSL mailing list or the Debian bug tracker? Whatever happened to code review?

Regular OpenSSL

Also in OpenSSL and also from 2008, “OpenSSL 0.9.8i and earlier does not properly check the return value from the EVP_VerifyFinal function, which allows remote attackers to bypass validation of the certificate chain via a malformed SSL/TLS signature for DSA and ECDSA keys.”

--- lib/libssl/src/ssl/s3_srvr.c +++ lib/libssl/src/ssl/s3_srvr.c @@ -2009,7 +2009,7 @@ static int ssl3_get_client_certificate(S else { i=ssl_verify_cert_chain(s,sk); - if (!i) + if (i <= 0) { al=ssl_verify_alarm_type(s->verify_result); SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE,SSL_R_NO_CERTIFICATE_RETURNED);

Bypass validation of the certificate chain? That’s bad, right? Like “worst security bug you could possibly imagine” bad, right?


Let’s look at the 2010 memset fix.

--- libc-a/memset.c +++ libc-b/memset.c @@ -1,6 +1,6 @@ void *memset(void *_p, unsigned v, unsigned count) { unsigned char *p = _p; - while(count-- > 0) *p++ = 0; + while(count-- > 0) *p++ = v; return _p; }

How is this possible? Does nobody use a compiler that warns about unused parameters? Where are the unit tests?


From 2011, “I took this opportunity to ‘refactor’ the AES-CTR code.”

--- tarsnap-autoconf-1.0.27/lib/crypto/crypto_file.c +++ tarsnap-autoconf-1.0.28/lib/crypto/crypto_file.c @@ -108,7 +108,7 @@ /* Encrypt the data. */ if ((stream = - crypto_aesctr_init(&encr_aes->key, encr_aes->nonce)) == NULL) + crypto_aesctr_init(&encr_aes->key, encr_aes->nonce++)) == NULL) goto err0; crypto_aesctr_stream(stream, buf, filebuf + CRYPTO_FILE_HLEN, len); crypto_aesctr_free(stream);

Pretty obvious what went wrong here: using goto with an unbraced if. Even novice programmers know that using the correct coding style prevents refactoring errors.


What do all these earlier mistakes have in common, apart from the obvious: being exemplars of “catastrophic loss of structural integrity”? They all date from before 2013. That’s how we know the NSA wasn’t involved.


I was fairly certain the sarcasm (or satire as I prefer to call it; sounds more intellectual) would be obvious. If the first few attempts didn’t work, surely the Tarsnap commentary would make my point unmistakeably clear. Alas, not. And no sooner was the latest GnuTLS diff announced than somebody asks “How is this possible?” I have failed utterly.

A few more thoughts.

Posted 2014-03-01 15:00:34 by tedu Updated: 2014-03-06 03:42:13
Tagged: c programming rants security