flak rss random

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.

X

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?

Android

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?

Tarsnap

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.

mysql

MySQL would let you login using one out of every 256 randomly generated passwords because ints are bigger than bools.

--- sql/password.c      2011-07-03 15:47:37 +0000
+++ sql/password.c      2012-04-06 09:04:07 +0000
@@ -531,7 +531,7 @@
   mysql_sha1_reset(&sha1_context);
   mysql_sha1_input(&sha1_context, buf, SHA1_HASH_SIZE);
   mysql_sha1_result(&sha1_context, hash_stage2_reassured);
-  return memcmp(hash_stage2, hash_stage2_reassured, SHA1_HASH_SIZE);
+  return test(memcmp(hash_stage2, hash_stage2_reassured, SHA1_HASH_SIZE));
 }
 
 

I can’t even.

lessons

What do all these earlier mistakes have in common? First, they’re all exemples of “catastrophic loss of structural integrity” as I used to say in my Star Trek days. Second, they all date from before 2013. That’s how we know the NSA wasn’t involved.

update

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.

sendmail

Another one for the record books. CVE-2014-3956.

--- gnu/usr.sbin/sendmail/sendmail/conf.c	12 Jun 2013 21:27:22 -0000	1.35
+++ gnu/usr.sbin/sendmail/sendmail/conf.c	5 Jun 2014 10:16:18 -0000
@@ -5267,8 +5267,8 @@ closefd_walk(lowest, fd)
 */
 
 void
-sm_close_on_exec(highest, lowest)
-	int highest, lowest;
+sm_close_on_exec(lowest, highest)
+	int lowest, highest;
 {
 #if HASFDWALK
 	(void) fdwalk(closefd_walk, &lowest);

Posted 01 Mar 2014 15:00 by tedu Updated: 03 Feb 2015 21:58
Tagged: c programming rants security