Why Application Security do matter. (or why you should care more)

tl;dr. Just a reminder that #Heartbleed is not a one-time event and that companies have much more bugs (partly written by themselves). They need to be aware that their developers don’t have the expertise to write safe code, and that they should invest in people who have the right skill-set – but also that at the end of the day we need better products and not software security solutions that just increase the attack surface.

In my previous article, I covered the vulnerable function to Heartbleed and its fix but also how confusing reading the code is.

After witnessing the panic and all the media attention (i.e. Colbert) this bug had been generating this week, it is finally obvious to the non-security community that software/application security is a key factor of the security of The Internet but also of our Critical Infrastructures. This isn’t the first critical bug we have seen, this one just happened to have much more media exposure than the previous ones – and it won’t be the last one either.

Software needs proper Quality Assurance and Risk Assessment. If a widely deployed open-source software like OpenSSL is showing such a poor code quality & security control due to its lack of resource, you can easily imagine how bad (i.e. worse!) it is with the rest of software used by critical infrastructures. Financial (Retail Banking, Investment Banking, Sovereign Wealth Funds, Hedge Funds, etc.), Industrial (Cars, Oil&Gas, etc.), Telecom (ISP, etc.) sectors, are all developing their internal software to address specific needs but in most of cases the resources allocated to Application Security are nonexistence or low and badly managed. And I’m not even mentioning the Enterprise softwares they are using who don’t benefit of a coverage like OpenSSL due to their restrictive licences.

What would be considered “correct” amount of resources ? Well, Risk management creates a value when its resources expended to mitigate risk should be less than the consequence of inaction. But since the consequence of inaction in the case of software control are infinite, a company can’t just wait for a regulation to be mandatory to take action.

But even if the resources are present, it often goes to inefficient security products that are just temporary mitigation and increasing the attack surface of the company instead of investing in building more robust products. But I understand how difficult it can be for a non-technical decision makers to identify what candidates/contractors/companies are qualified. Moreover, it is an industry where it is actually hard and rare to find skilled engineers (which is partly why Obama was recently promoting programming). This is mainly due to the fact that InfoSec is an industry of autodidacts experts. In addition to that, since InfoSec is becoming a “hype” industry, we are also witnessing growing number of “experts” with limited programming skills and who will summarize their exposure to Information Technology Security by mentioning the certifications they are holding (ed. superficial knowledge).

Books like Threat Modeling, exist for more than 10 years – it is only now that we start to see new ISO standards such as ISO 30111 Vulnerability handling processes (Kudos to @k8em0!) or ISO/IEC TS 17961:2013 (has an access fee). Which is one step further for the Enterprise World.

Having a CISO and following ISO 2700X are a good things, but it doesn’t necessary mean a company understands its problems and how to efficiently address them. But things are evolving and there are obviously a lot of progress made on “security awareness” compared to 10 years ago, but there are still a lot of things still need to change.

#Eyebleed. A technical analysis of the fix (not the bug!) for the Heartbleed issue

eyebleed
Heartbleed (CVE-2014-0160) is a bug found in OpenSSL due to a misimplementation of the Heartbeat functionality, and found by Neel Mehta (Google Security Team)

As a side note, this only affects OpenSSL which is NOT used by Microsoft ! See official statement here

A lot of people (Ted Angust, Existentialize, IOActive, etc.) already provided an analysis of the CVE-2014-0160 bug, widely known under the name “Heartbleed”, but nobody has analyzed the fix yet.

I had a look at both the code and the RFC, and it raised few questions to me.

1. The fix (and the original code)

We can see below a copy of the fixed-version of the dtls1_process_heartbeat() function.

  1. int
  2. dtls1_process_heartbeat(SSL *s)
  3.   {
  4.   unsigned char *p = &s->s3->rrec.data[0], *pl;
  5.   unsigned short hbtype;
  6.   unsigned int payload;
  7.   unsigned int padding = 16; /* Use minimum padding */
  8.  
  9.   if (s->msg_callback)
  10.     s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
  11.       &s->s3->rrec.data[0], s->s3->rrec.length,
  12.       s, s->msg_callback_arg);
  13.  
  14.   /* Read type and payload length first */
  15.   if (1 + 2 + 16 > s->s3->rrec.length)
  16.     return 0; /* silently discard */
  17.   hbtype = *p++;
  18.   n2s(p, payload);
  19.   if (1 + 2 + payload + 16 > s->s3->rrec.length)
  20.     return 0; /* silently discard per RFC 6520 sec. 4 */
  21.   pl = p;
  22.  
  23.   if (hbtype == TLS1_HB_REQUEST)
  24.     {
  25.     unsigned char *buffer, *bp;
  26.     unsigned int write_length = 1 /* heartbeat type */ +
  27.               2 /* heartbeat length */ +
  28.               payload + padding;
  29.     int r;
  30.  
  31.     if (write_length > SSL3_RT_MAX_PLAIN_LENGTH)
  32.       return 0;
  33.  
  34.     /* Allocate memory for the response, size is 1 byte
  35.      * message type, plus 2 bytes payload length, plus
  36.      * payload, plus padding
  37.      */
  38.     buffer = OPENSSL_malloc(write_length);
  39.     bp = buffer;
  40.  
  41.     /* Enter response type, length and copy payload */
  42.     *bp++ = TLS1_HB_RESPONSE;
  43.     s2n(payload, bp);
  44.     memcpy(bp, pl, payload); < ==== The initial bug was here, because it was copying "too-much" data from the source buffer.
  45.     bp += payload;
  46.     /* Random padding */
  47.     RAND_pseudo_bytes(bp, padding);
  48.  
  49.     r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, write_length);
  50.  
  51.     if (r >= 0 && s->msg_callback)
  52.       s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,
  53.         buffer, write_length,
  54.         s, s->msg_callback_arg);
  55.  
  56.     OPENSSL_free(buffer);
  57.  
  58.     if (r < 0)
  59.       return r;
  60.     }

1.1 Misleading comments & undefined constants

I found the function so hard to read that it looked like it was obfuscated JavaScript code, especially with all those undefined constants everywhere and misleading comments. For instance, what is described as “heartbeat length” is in fact the “payload length of the heartbeat message” but what intrigued me the most was the static constant for “padding length” (mentioned as “padding” grrr.)

1.2 Padding Length

After a look at the RFC 6520 “Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) Heartbeat Extension”, I didn’t see any point mentioning that padding length was supposed to be a have static size (16) as done in the code. On the contrary, it is quite confusing to understand who is supposed to take care of it.

1.2.1 RFC 6520. Heartbeat messages

RFC 6520: Therefore, the padding_length is TLSPlaintext.length – payload_length – 3 for TLS and
DTLSPlaintext.length – payload_length – 3 for DTLS. The padding_length MUST be at least 16.
The sender of a HeartbeatMessage MUST use a random padding of at
least 16 bytes. The padding of a received HeartbeatMessage message
MUST be ignored.

The Heartbeat protocol messages consist of their type and an arbitrary payload and padding.

Read the full RFC 6520.

1.2.2 My understanding

My understanding is that there are two types of messages:

  • HeartbeatResponse
  • HeartbeatRequest

And that *both* have a padding, including the padding of a received HeartbeatMessage message MUST be ignored. But by that, I understand that the RFC it means the content and not the field itself.
This is my I concluded that OpenSSL should not be the one deciding of the size of the padding buffer, which therefore would have prevented to allocate a buffer and to use memcpy().

Below is my pseudo-code interpretation of the RFC 6520 and what the code should be:

  1. typedef struct heartbeat_msg_st
  2. {
  3.     /*
  4.         RFC 6520: The total length of a HeartbeatMessage MUST NOT exceed 2^14 or
  5.         max_fragment_length when negotiated as defined in [RFC6066].
  6.     */
  7.     char type;
  8.     unsigned short payload_length;
  9.     unsigned char payload[1];  /* Any size array. RFC 6520 only mentions "arbitrary content" which is (probably?) assumed to be non-empty. */
  10.     unsigned char padding[16]; /*  The padding_length MUST be at least 16. */
  11. } HEARTBEAT_MSG;
  12.  
  13. int
  14. dtls1_process_heartbeat(SSL *s)
  15. {
  16.     HEARTBEAT_MSG *hb_msg = &s->s3->rrec.data[0];
  17.     char hb_type;
  18.     unsigned short payload_length;
  19.     unsigned int padding_length;
  20.     unsigned int hb_msg_length = s->s3->rrec.length;
  21.  
  22.     if (s->msg_callback)
  23.         s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT,
  24.             hb_msg, hb_msg_length,
  25.             s, s->msg_callback_arg);
  26.  
  27.     if (hb_msg_length < sizeof(HEARTBEAT_MSG)) return 0; // silently discard.
  28.     if (hb_msg_length < (hb_msg->payload_length + sizeof(payload_length) + sizeof(hb_type))) return 0; // silently discard.
  29.     padding_length = hb_msg_length - hb_msg->payload_length - sizeof(payload_length) - sizeof(hb_type);
  30.     if (padding_length < 16) return 0; // silently discard.
  31.  
  32.     hb_type = hb_msg->type;
  33.     payload_length = hb_msg->payload_length;
  34.  
  35.     if (hb_type == TLS1_HB_REQUEST)
  36.     {
  37.         unsigned char *padding = (unsigned char *)&hb_msg;
  38.         padding = &padding[hb_msg_length - padding_length];
  39.         if (hb_msg_length > SSL3_RT_MAX_PLAIN_LENGTH) return 0;
  40.  
  41.         hb_msg->type = TLS1_HB_RESPONSE;
  42.        
  43.         /* Random padding */
  44.         RAND_pseudo_bytes(padding, padding_length);
  45.  
  46.         r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, hb_msg, hb_msg_length);
  47.  
  48.         if (r >= 0 && s->msg_callback)
  49.             s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT,
  50.             hb_msg, hb_msg_length,
  51.             s, s->msg_callback_arg);
  52.  
  53.         if (r < 0) return r;
  54.     }

2. Any thoughts ?

Reading a RFC can be hard, including widely deployed code such as OpenSSL. I'd be curious to hear (@msuiche) your interpretation of the RFC.

Faces of Yemen

Access to the full album here, and the second album (B&W) here.

I recently had the chance to travel across Yemen, which is a not well known but beautiful country with a unique ecosystem.

Even through the country is listed as high security level threat, the areas I’ve been visited were relatively safe and I didn’t experience any troubles during my trip. From what I can say, Yemenis are part of the most welcoming, and friendly people I had the chance to meet.

As you can see, people are full of life and their faces tell lots of stories. That’s why I called the Album “Faces of Yemen“.

Sana’a


Great Mosque of Sana’a


Happy kids from Kawkaban


Sunset on Sana’a


Confidences over Sana’a

Socotra Island

Socotra Island, is an Island between Yemen and Somalia. Mainly known for her dragonblood trees which are a unique specie that can only be found on the Island. There are several legends about its origins but my favorite is this one:
In the Middle Ages, an European King was looking for a potion to cure his ill unique daughter. He offered her hand to anyone who would bring her the medicine. One of the candidates, mentioned he knew about an Island with a special remedy that could save his daughter. The King supported the Chevalier who went to Socotra to find the medicine. Once the Chevalier arrived on the Island, he had to fight with a Giant bird/dragon that he managed to severely injure during his battle. Then the bird/dragon tried to escape and flew all over the Island, and for each drop of its blood, a Dragonblood tree grew out of it.

Socotra Island as several protected area established by the Yemeni Environment Protection Authority (EPA), and counts several NGO such as the Triangle or the Yemeni Biological Society founded and presided by Dr. Abdul Karim Nasher, a well known and respect Yemeni scientist, who I had the chance to meet and who is involved with multiple projects such as the protection of sea turtles across the country. I recommend you to watch his talk from TEDxSanaa: Yemen is richer with its plants to know more about the unique ecosystem of Yemen.
I look at Zoology/Biology as a kind of reverse engineering study applied to life and living organisms, which is the reason why I find it so fascinating.


Dragonblood tree


Kid on the beach


Happy girl


Fisherman


Bottletree next to a lagoon


Altitude


Goatkeeper

iPhone and OS X Users Beware, All Your Data Is Public (eg. When at your fav Starbucks)

Apple released an important patch today to prevent potential interception and manipulation of encrypted connections. The bug had been introduced almost 6 months ago, the security community believes this is a backdoor deliberately introduced by one of the Apple’s engineer.
EDIT:The patch is so far only available for iOS 7.0.6 but OS X 10.9.1 is still vulnerable.Mac users: DO NOT use Safari and other applications that are potentially using the OS X SSL/TLS libraries until a patch is available ! iPhone and iPad users: Update immediately to 7.0.6 !
EDIT2: 23rd Feb, 2014: You can check if you are vulnerable by going to www.gotofail.com.
EDIT3: 23rd Feb, 2014: Stefan Esser released an unofficial patch for the vulnerability on Mac OS X.
EDIT4: 23rd Feb, 2014: I added a comparison between different compilers.
EDIT5: 23rd Feb, 2014: My kiwi friend recommended me to change the title from “SSLVerifySignedServerKeyExchange() a.k.a. The “goto epicfail;” bug”.
EDIT6: 25th Feb, 2014: Apple finally released a patch for OS X.

Today, Apple released an important patch for iOS CVE-2014-1266 where Secure Transport (SSL/TLS) failed to validate the authenticity of “secure” connections. This issue was addressed by restoring missing validation steps. This translates as potential man-in-middle (interception and manipulation of encrypted data) weaknesses as highlighted by Apple “An attacker with a privileged network position may capture or modify data in sessions protected by SSL/TLS“.

The bug: goto fail;

As pointed out by pencilo on Hacker News few hours ago, Apple introduced a bug in the SSLVerifySignedServerKeyExchange() function. Stefan Esser later pointed out on Twitter that the bug seemed to have been introduced recently because the bug wasn’t present in OS X 10.8.5 (October 3, 2013) but is in 10.9.1 In order word, this bug lasted almost 6 months for iOS, and is hopefully gonna be patch in OS X 10.9.2 next week.

  1. static OSStatus
  2. SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
  3.                                  uint8_t *signature, UInt16 signatureLen)
  4. {
  5.     OSStatus        err;
  6. ()
  7.     if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
  8.         goto fail;
  9.     if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
  10.         goto fail;
  11.     if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
  12.         goto fail;
  13.     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
  14.         goto fail;
  15.         goto fail; < ============================================== BUG ! (backdoor ?)
  16.     if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
  17.         goto fail;
  18.  
  19.   err = sslRawVerify(ctx,
  20.                        ctx->peerPubKey,
  21.                        dataToSign,        /* plaintext */
  22.                        dataToSignLen,     /* plaintext length */
  23.                        signature,
  24.                        signatureLen);
  25.   if(err) {
  26.     sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
  27.                     "returned %d\n", (int)err);
  28.     goto fail;
  29.   }
  30.  
  31. fail:
  32.     SSLFreeBuffer(&signedHashes);
  33.     SSLFreeBuffer(&hashCtx);
  34.     return err;
  35. }

The bug is actually a duplicated “goto fail;”. For most of programmers, this is really a bit hard to believe that a such bug had been unintentional introduced and is the result of an “error mistake”. We (security researchers) are already expecting denials from Apple PRs.

Best-practices of compilation. A Comparison between different compilers on different platform.

One of the main reason this is hard to believe is that this is an easy mistake to catch for modern compilers, since all moderns compilers do have a “Treat warnings as errors” and “Catch all warnings” options.

Both Gccand Clang compilers do have compilations options to catch basic mistakes such as unreachable code as warnings but also to treat warnings as errors to prevent compilation if existing “warnings” had been detected in the code. EDIT: GCC do not have an option to detect unreachable code anymore!

EDIT:

Compilers Option Description
Visual Studio /WX Make all warnings into errors
Visual Studio /Wall This enables all the warnings. Including unreachable code.
GCC -Werror Make all warnings into errors
GCC -Wall Enables all the warnings about constructions that some users consider questionable, and that are easy to avoid (or modify to prevent the warning), even in conjunction with macros.. *NOT* including unreachable code.
-Waddress
-Warray-bounds (only with -O2)
-Wc++11-compat
-Wchar-subscripts
-Wenum-compare (in C/ObjC; this is on by default in C++)
-Wimplicit-int (C and Objective-C only)
-Wimplicit-function-declaration (C and Objective-C only)
-Wcomment
-Wformat
-Wmain (only for C/ObjC and unless -ffreestanding)
-Wmaybe-uninitialized
-Wmissing-braces (only for C/ObjC)
-Wnonnull
-Wopenmp-simd
-Wparentheses
-Wpointer-sign
-Wreorder
-Wreturn-type
-Wsequence-point
-Wsign-compare (only in C++)
-Wstrict-aliasing
-Wstrict-overflow=1
-Wswitch
-Wtrigraphs
-Wuninitialized
-Wunknown-pragmas
-Wunused-function
-Wunused-label
-Wunused-value
-Wunused-variable
-Wvolatile-register-var
GCC -Wextra This enables some extra warning flags that are not enabled by -Wall. *NOT* including unreachable code.
-Wclobbered
-Wempty-body
-Wignored-qualifiers
-Wmissing-field-initializers
-Wmissing-parameter-type (C only)
-Wold-style-declaration (C only)
-Woverride-init
-Wsign-compare
-Wtype-limits
-Wuninitialized
-Wunused-parameter (only with -Wunused or -Wall)
-Wunused-but-set-parameter (only with -Wunused or -Wall)
GCC -Wunreachable-code Warn if the compiler detects that code will never be executed. This option was present up to GCC 4.4 but was later removed as stated here from a 2011 post: http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
Clang -Weverything (Clang only) Higher level than -Wextra. This enables all warnings which seems to also include -Wunreachable-code as pointed out by Peter Nelson and Adam Langley. Clang also has UnreachableCodeChecker.cpp present as part of its Static analyzer.

Here is an basic example of a similar bug on Windows using Visual Studio 2012 copmiler with the following options: /W4 or /Wall (Catch all warnings) and /WX (Treat warnings as error).

  1. int
  2. RandomCheck(int Input)
  3. {
  4.     int Status = FALSE;
  5.  
  6.     if (Input >= 2) Status = TRUE;
  7.  
  8.     return FALSE;
  9. }
  10.  
  11. int
  12. BuggedFunction(int Input)
  13. {
  14.     int Status = FALSE;
  15.     int Err;
  16.  
  17.     wprintf(L"Reachable code\n");
  18.     if ((Err = RandomCheck(Input)) != TRUE)
  19.         goto fail;
  20.         goto fail; < ================================== BUG
  21.  
  22.     Status = TRUE;
  23.  
  24.     wprintf(L"Unreachable code.\n");
  25.  
  26. fail:
  27.     return Status;
  28. }
  29.  
  30. int main(char **argv, int argc)
  31. {
  32.     int Err;
  33.  
  34.     UNREFERENCED_PARAMETER(argv);
  35.  
  36.     Err = BuggedFunction(argc);
  37.     wprintf(L"BuggedFunction(%d) = %d\n", argc, Err);
  38. }

If you are trying to compile the code above with the options mentioned above, you will get the following errors reported by the compiler and this would prevent the compiler to complete its task until the programmer manually fixes those errors.

1>------ Rebuild All started: Project: gotofail, Configuration: Release Win32 ------
1>  gotofail.c
1>  Generating code
1>c:\users\msuiche\documents\visual studio 2012\projects\gotofail\gotofail.c(27): error C2220: warning treated as error - no 'executable' file generated
1>c:\users\msuiche\documents\visual studio 2012\projects\gotofail\gotofail.c(27): warning C4702: unreachable code
1>LINK : fatal error LNK1257: code generation failed
========== Rebuild All: 0 succeeded, 1 failed, 0 skipped ==========
Conclusion: Human mistake ?

As I said above, this means Apple either does not use proper compilation options to enforce the security of its products, or that someone deliberately disabled them for this module to be able to enable this bug that would normally not even exist in 2014, especially if you are a company like Apple... This is a fail on so many levels from an engineering point of view, on the developer side, on the code review side, on the poor compilation tools used and on the Q&A side supposed to catch those kind of regressions.