Thursday, August 12, 2010

Upping the Ante: A Thought Exercise

WARNING: This post contains a trip into the wondrous what-if-land to explore a different means of increasing security for the end-user.

I was thinking the other day about how the landing pages for browsers (and other products) usually tout security as one of its main attributes. Below are a few examples:

Firefox:
What makes Firefox the best?

Keeping you safe while you surf is our top priority, which is why Firefox includes advanced anti-phishing and anti-malware technologies plus features like private browsing and “forget this site” to ensure your privacy.

Plus, our open source security process means we have experts around the globe working around the clock to keep you (and your personal information) safe.

Google Chrome:
Google Chrome includes features to help protect you and your computer from malicious websites as you browse the web. Chrome uses technologies such as Safe Browsing, sandboxing, and auto-updates to help protect you against phishing and malware attacks.

Internet Explorer:
Browser Security

Other browsers leave you more exposed to phishing, malware and other online threats. Or they require you to download and configure third-party add-ons just to get the security Internet Explorer 8 comes with right out of the box

Opera:
Safe and secure

Reduce your exposure to threats on the Web. Industry-leading security and features such as Web Threat Protection and a security bar, ensure your safety.

To me each of them is being _very_ conservative when it comes to describing how secure their product is. Each of those companies has extremely intelligent people from the security field working for them, about whom each company could boast. They could also talk about how few bugs have been found in their product, or their awesome fuzzing-farm that should dramatically reduce the number of 0days available for researchers to find. Or, if they wanted to get really crazy, they could even publicly bash their competitors, saying things like "Browser A sucks! BB security vulnerabilities were found in it in the past CC months! Only DD vulns were found in ours!"

What if companies actually _did_ focus on publicly bashing their competitors? The end goal for a company making a product is to increase their user-base and hence their profits, right? If they could make their competitor's product look like swiss cheese because of the number of security holes in it, they might very effectively keep existing users from switching to a competitor. They could also try to "prove" how little their product resembles swiss cheese, drawing in more users.

So far, this all doesn't seem too far-fetched. Often, I think of competing companies as being relatively nice towards each other when they find security vulnerabilities in the competitor's product. The vuln might get reported responsibly and then isn't really mentioned once it's fixed. But this is all different in what-if land. Imagine a world where they weren't publicly "nice" to each other. In this world, each company's security team would have two main job functions: fix our product, and find security holes in the competition so we can publicly humiliate them!

If this were the case, everything would be nuts and highly entertaining! Security marketing for a product would be more along the lines of "Don't use product EE! Our security team alone has recently found FF vulnerabilities in their product, we have only had [some # smaller than FF]!" or "Our security team finds more bugs in our competitor's products than they can manage to find and fix on their own! Imagine how secure our product is with such a team securing it!" or even more crass "Product GG has been found to be vulnerable to HH! We're not, use us! If you use them, we'll pwn you with the 0days we've been saving!" Once a company released a statement publicly bashing a competitor, the competitor would be forced to defend themselves in some form or another in order to keep their existing user-base. They might counter-attack, releasing advisories and embarrassing the attacker, or they might show how security feature MM effectively thwarts those attack vectors. If a company has no comeback, their user-base will dwindle until they can save face before their users.

Mainstream media would, of course, have a heyday with any and all public bashing. News stories with headlines such as "Company II publicly humiliates Company JJ! JJ declares war on II!" would abound, increasing the drama and upping the ante and humiliation.

But who would all of this actually benefit? Well, that should be fairly obvious, actually: the end-user! (Hey, isn't that what all this security stuff is about anyways?!) With all of the public bashing, humiliating, security dream-team putting-togethering going on, the actual products themselves will become more secure, either directly or indirectly. The rate at which bugs are found and fixed in a product would be amplified tremendously. New security features would constantly be thought up to stave off attacks and to undermine the competitor's claims. New policies would be put into place in companies to _quickly_ provide fixes for the end-user. Companies would scour the globe looking for the best in the security field to help them defend their product and attack the competitors. In the end, by the time the product reaches the end-user, it would be more secure. The end-user would also have a good time sitting back and enjoying the drama! Heck, companies could even sell tickets! We could make a sporting event out of it!

However, this wouldn't only help the end-user: it would help security researchers tremendously. As a result of the need for sufficiently good bash-ammunition and defensive armor a company will need against its competitors in order to stay alive, large companies that currently do not pay for vulnerabilities would be forced to pay. Not only would they be forced to pay, they would have to compete for security researcher's attention by constantly increasing the incentive. It's a win-win situation!

Of course there are some pitfalls in this thinking that I haven't mentioned, such as a large company picking on a small start-up who doesn't yet have the means to compete or defend themselves, or companies targeting specific individuals in the competition instead of the company they work for. In such a case, referees would have to be appointed who could rebuke the offending, rule-breaking company and publicly shame them for being a bully. The larger company then might have to help the smaller company out with it's security team and policies until it's able to defend itself. If an individual was personally harassed, the company should have to publicly apologize and send him a fruit-basket and a "Get-Well" card.

Crazy idea, huh? There are several other points I thought about bringing up, but I think I demonstrated what I wanted to. Would this really work? Probably not. It'd be crazy if it did though. Am I serious about this post? Partly (remember, this was only a thought-exercise!)

Even though this is an extreme example, the point I really wanted to make with this post is how different things might be for the end-user if companies personally had more at stake for creating (and selling) insecure products. My conclusion is that things could be very different from how they are today. Companies would have bigger/better security teams, different bug-fixing policies requiring fast turn around times for patch development, and the incentive for reporting vulnerabilities directly to the vendor would be at least better than the possibility of having your name in the acknowledgment section of an advisory. I think it might be a good thing.

PS - On a side note, in case you needed more food for thought: Does the problem of finding and fixing every security vulnerability in a product belong in P, NP, or is it NP complete?

Wednesday, July 21, 2010

libpng extra row (CVE-2010-1205)

This bug is a fairly straightforward one. I am interested in it for two main reasons. The first, and smaller reason, is that it's an interesting bug in that two parties are at fault if an application is vulnerable: the makers of the application and libpng itself. Each had to do something wrong in order for this to be an actual problem. The second, larger reason why I am so interested in this bug is because I had fuzzed into this bug back in April and sat on it too long. What happened next? Lo and behold, I wasn't the only one looking at pngs -- someone else reported it (first public reference: Google Chrome Issue, see also the alert on the libpng home page, and most recently, a Mozilla Security Advisory). Props to the guy who reported it (Aki Helin). With that in mind, here's a brief description of the vuln: an extra row in the image data is bad [*sarcasm*].

Actually, it's slightly more complicated than that. Libpng uses callbacks to aid with progressive reading of a png. That way, as the image rows become available, the application can handle them. The root problem lies in the fact that libpng versions prior to the fix would call the row_callback() function in the application for _every_ row without checking to see if more rows had been processed than the declared height in the IHDR chunk (which is what memory allocations are [usually] based on). Since libpng didn't check the number of rows processed (or the number of times the row_callback() was called), the last place to catch this bug would be in the application. If the application checked how many times its callback had been called against the height of the png stored in the png_ptr, it could bail when too many rows were received. But, libpng has been around for quite a while (15+ years), so everyone trusts it and never does their own checks, merely copying the inflated image rows into their own buffers, using a calculation based on their base buffer address, image width, row number, and the number of bytes per pixel.

Below is a snippet of Firefox's row_callback function. The statement that copies the image row data to the application's buffer is on line 822:
// Firefox 3.6.3
// modules/libpr0n/decoders/png/nsPNGDecoder.cpp  <-- who named this module??

725 void
726 row_callback(png_structp png_ptr, png_bytep new_row,
727              png_uint_32 row_num, int pass)
728 {
...
762   if (new_row) {
...
791     switch (decoder->format) {
...
819       case gfxASurface::ImageFormatARGB32:
820       {
821         for (PRUint32 x=width; x>0; --x) {
822           *cptr32++ = GFX_PACKED_PIXEL(line[3], line[0], line[1], line[2]);
823           if (line[3] != 0xff)
824             rowHasNoAlpha = PR_FALSE;
825           line += 4;
826         }
827       }
828       break;
...
833     }
...
851   }
852 }
Below is a snippet of Google Chrome's row_callback. The destination address in the code below is calculated on line 356, and the statement that copies the image row data to the destination is on line 360:
// Google Chrome, r44471
// gfx/codec/png_codec.cc

338 void DecodeRowCallback(png_struct* png_ptr, png_byte* new_row,
339                        png_uint_32 row_num, int pass) {
340   PngDecoderState* state = static_cast(
341       png_get_progressive_ptr(png_ptr));
342 
343   DCHECK(pass == 0) << "We didn't turn on interlace handling, but libpng is "
344                        "giving us interlaced data.";
345   if (static_cast(row_num) > state->height) {
346     NOTREACHED() << "Invalid row";
347     return;
348   }
349 
350   unsigned char* base = NULL;
351   if (state->bitmap)
352     base = reinterpret_cast(state->bitmap->getAddr32(0, 0));
353   else if (state->output)
354     base = &state->output->front();
355 
356   unsigned char* dest = &base[state->width * state->output_channels * row_num];
357   if (state->row_converter)
358     state->row_converter(new_row, state->width, dest, &state->is_opaque);
359   else
360     memcpy(dest, new_row, state->width * state->output_channels);
361 }
Well, enough of the rant-ish speaking. Below is how the libpng code worked before the fix:

First, the function png_process_data() is called by the application:
  // pngpread.c

  30 void PNGAPI
  31 png_process_data(png_structp png_ptr, png_infop info_ptr,
  32    png_bytep buffer, png_size_t buffer_size)
  33 {
  34    if (png_ptr == NULL || info_ptr == NULL)
  35       return;
  36 
  37    png_push_restore_buffer(png_ptr, buffer, buffer_size);
  38 
  39    while (png_ptr->buffer_size)
  40    {
  41       png_process_some_data(png_ptr, info_ptr);
  42    }
  43 }
This ends up calling png_process_some_data(), which iterates through and processes each chunk. Once the IDAT chunk (the chunk where the image data [and extra row] are stored) has been read and handled (via line 62), it is then in the PNG_READ_IDAT_MODE. At this point, it then drops down into the case at line 68, calling png_push_read_IDAT():
  // pngpread.c

  45 /* What we do with the incoming data depends on what we were previously
  46  * doing before we ran out of data...
  47  */
  48 void /* PRIVATE */
  49 png_process_some_data(png_structp png_ptr, png_infop info_ptr)
  50 {
  51    if (png_ptr == NULL)
  52       return;
  53 
  54    switch (png_ptr->process_mode)
  55    {
  ...
  62       case PNG_READ_CHUNK_MODE:
  63       {
  64          png_push_read_chunk(png_ptr, info_ptr);
  65          break;
  66       }
  67 
  68       case PNG_READ_IDAT_MODE:
  69       {
  70          png_push_read_IDAT(png_ptr);
  71          break;
  72       }
  ...
 109    }
 110 }
The png_push_read_IDAT() function is a bit longer, so I'm only going to show one relevant part:
 // pngpread.c

 735 void /* PRIVATE */
 736 png_push_read_IDAT(png_structp png_ptr)
 737 {
 738    PNG_IDAT;
 ...
 765    if (png_ptr->idat_size && png_ptr->save_buffer_size)
 766    {
 767       png_size_t save_size;
 768 
 769       if (png_ptr->idat_size < (png_uint_32)png_ptr->save_buffer_size)
 770       {
 771          save_size = (png_size_t)png_ptr->idat_size;
 772 
 773          /* Check for overflow */
 774          if ((png_uint_32)save_size != png_ptr->idat_size)
 775             png_error(png_ptr, "save_size overflowed in pngpread");
 776       }
 777       else
 778          save_size = png_ptr->save_buffer_size;
 779 
 780       png_calculate_crc(png_ptr, png_ptr->save_buffer_ptr, save_size);
 781 
 782       if (!(png_ptr->flags & PNG_FLAG_ZLIB_FINISHED))
 783          png_process_IDAT_data(png_ptr, png_ptr->save_buffer_ptr, save_size);
 784 
 785       png_ptr->idat_size -= save_size;
 786       png_ptr->buffer_size -= save_size;
 787       png_ptr->save_buffer_size -= save_size;
 788       png_ptr->save_buffer_ptr += save_size;
 789    }
 ...
 826 }
Basically, this function gets called multiple times, going down a different code path depending on what state the png_ptr->mode is in. One of the code paths makes a call to png_process_IDAT_data(), which inflates the compressed image data and begins the row processing. Notice how libpng never checks to see if the number of rows processed is greater than the declared height in the IHDR chunk:
 // pngpread.c

 828 void /* PRIVATE */
 829 png_process_IDAT_data(png_structp png_ptr, png_bytep buffer,
 830    png_size_t buffer_length)
 831 {
 832    int ret;
 833 
 834    if ((png_ptr->flags & PNG_FLAG_ZLIB_FINISHED) && buffer_length)
 835       png_benign_error(png_ptr, "Extra compression data");
 836 
 837    png_ptr->zstream.next_in = buffer;
 838    png_ptr->zstream.avail_in = (uInt)buffer_length;
 839    for (;;)
 840    {
 841       ret = inflate(&png_ptr->zstream, Z_PARTIAL_FLUSH);
 842       if (ret != Z_OK)
 843       {
 844          if (ret == Z_STREAM_END)
 845          {
 846             if (png_ptr->zstream.avail_in)
 847                png_benign_error(png_ptr, "Extra compressed data");
 848 
 849             if (!(png_ptr->zstream.avail_out))
 850             {
 851                png_push_process_row(png_ptr);
 852             }
 853 
 854             png_ptr->mode |= PNG_AFTER_IDAT;
 855             png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 856             break;
 857          }
 858          else if (ret == Z_BUF_ERROR)
 859             break;
 860 
 861          else
 862             png_error(png_ptr, "Decompression Error");
 863       }
 864       if (!(png_ptr->zstream.avail_out))
 865       {
 866          if ((
 867 #ifdef PNG_READ_INTERLACING_SUPPORTED
 868              png_ptr->interlaced && png_ptr->pass > 6) ||
 869              (!png_ptr->interlaced &&
 870 #endif
 871              png_ptr->row_number == png_ptr->num_rows))
 872          {
 873            if (png_ptr->zstream.avail_in)
 874              png_warning(png_ptr, "Too much data in IDAT chunks");
 875            png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 876            break;
 877          }
 878          png_push_process_row(png_ptr);
 879          png_ptr->zstream.avail_out =
 880              (uInt) PNG_ROWBYTES(png_ptr->pixel_depth,
 881              png_ptr->iwidth) + 1;
 882          png_ptr->zstream.next_out = png_ptr->row_buf;
 883       }
 884 
 885       else
 886          break;
 887    }
 888 }
As soon as a complete row is available, it calls the png_push_process_row() function (I'm leaving out the interlaced code for simplicity):
 // pngpread.c

 890 void /* PRIVATE */
 891 png_push_process_row(png_structp png_ptr)
 892 {
 893    png_ptr->row_info.color_type = png_ptr->color_type;
 894    png_ptr->row_info.width = png_ptr->iwidth;
 895    png_ptr->row_info.channels = png_ptr->channels;
 896    png_ptr->row_info.bit_depth = png_ptr->bit_depth;
 897    png_ptr->row_info.pixel_depth = png_ptr->pixel_depth;
 898 
 899    png_ptr->row_info.rowbytes = PNG_ROWBYTES(png_ptr->row_info.pixel_depth,
 900        png_ptr->row_info.width);
 901 
 902    png_read_filter_row(png_ptr, &(png_ptr->row_info),
 903       png_ptr->row_buf + 1, png_ptr->prev_row + 1,
 904       (int)(png_ptr->row_buf[0]));
 905 
 906    png_memcpy(png_ptr->prev_row, png_ptr->row_buf, png_ptr->rowbytes + 1);
 907 
 908    if (png_ptr->transformations || (png_ptr->flags&PNG_FLAG_STRIP_ALPHA))
 909       png_do_read_transformations(png_ptr);
 910 
 911 #ifdef PNG_READ_INTERLACING_SUPPORTED
 ...
1088 #endif
1089    {
1090       png_push_have_row(png_ptr, png_ptr->row_buf + 1);
1091       png_read_push_finish_row(png_ptr);
1092    }
1093 }

Notice the two calls on line 1090 and 1091. The first call, png_push_have_row(), calls the row_callback function defined in the application:
// pngpread.c

1682 void /* PRIVATE */
1683 png_push_have_row(png_structp png_ptr, png_bytep row)
1684 {
1685    if (png_ptr->row_fn != NULL)
1686       (*(png_ptr->row_fn))(png_ptr, row, png_ptr->row_number,
1687          (int)png_ptr->pass);
1688 }
The second call, png_read_push_finish_row(), merely increments png_ptr->row_number (it does more post-processing for interlaced images as well):
// pngpread.c

1095 void /* PRIVATE */
1096 png_read_push_finish_row(png_structp png_ptr)
1097 {
...
1117    png_ptr->row_number++;
...
1157 }
So what is the error? The number of actual rows in the image data is never checked against the rows declared in the IHDR chunk. When libpng fixed this bug, they rewrote most of the png_process_IDAT_data() function. The critical fix they added was this:
 // pngpread.c, libpng-1.4.3

 827 void /* PRIVATE */
 828 png_process_IDAT_data(png_structp png_ptr, png_bytep buffer,
 829    png_size_t buffer_length)
 830 {
 ...
 845    while (png_ptr->zstream.avail_in > 0 &&
 846       !(png_ptr->flags & PNG_FLAG_ZLIB_FINISHED))
 847    {
 ...
 891       /* Did inflate output any data? */
 892       if (png_ptr->zstream.next_out != png_ptr->row_buf)
 893       {
 894      /* Is this unexpected data after the last row?
 895       * If it is, artificially terminate the LZ output
 896       * here.
 897       */
 898          if (png_ptr->row_number >= png_ptr->num_rows ||
 899          png_ptr->pass > 6)
 900          {
 901         /* Extra data. */
 902         png_warning(png_ptr, "Extra compressed data in IDAT");
 903             png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 904         /* Do no more processing; skip the unprocessed
 905          * input check below.
 906          */
 907             return;
 908      }
 ...
 918    }
 ...
 926 }

Notice how it has the explicit check against the row number and the height of the image on line 898. Libpng now only calls the row_callback function at a maximum height times.

I think this is a prime example about trusting third-party code. Like a firearm safety instructor told me once about using the safety, "we use them, but we don't trust them". I think that is how third-party code should be used.