
CVE-2015-8126: Multiple buffer overflows in libpng - pjmlp
https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2015-8126
======
jimrandomh
Uh oh. It's time for the monthly global computer security meltdown. Again, but
worse this time. Security vulnerabilities in libpng are a huge deal; they
affect lots and lots of different programs, including things people don't
usually think of, and many them embed their own copies of libpng which makes
them hard to update. The last time there was a security vulnerability in
libpng, it took _years_ before it was fixed everywhere.

Patching this properly will require not just replacing a centralized copy of
libpng, but scanning all binaries for statically-linked copies. There might be
some tools left over from the last time libpng got into trouble; this would be
a very good time to update and link to them. Some categories of software that
are at particularly high risk right now:

* Web browsers. Sandboxing likely helps, but there's a possibility that there are some usages (eg favicons) that decode outside the sandbox.

* MMS on Android (possibly remote root on phones with no user interaction)

* Any server that accepts PNG uploads and processes them (eg, user avatar thumbnail generation)

* Anything which shows people user-provided avatar images (eg, IM clients)

* Video games which download levels that can contain images

* File browsers which show image previews

* Music players which display album covers

Expect malicious images to start appearing everywhere, and soon. If you're
responsible for the computer security anywhere, this is a drop-everything
priority.

~~~
throwaway2048
This is exactly why static linking/bundled libs/containerization is such a
piss poor idea. We are going to be dealing with this vuln for years and years,
if not decades to come. The exact same insanity has has happened with embedded
copies of zlib in the past.

The solution to dependency management in way too many cases becomes "never
update". And those mechanisms allow negligence like that to fester for years
ignored/unnoticed.

~~~
marssaxman
Containerization might well be the _solution_ to this kind of problem:
libpng's vulnerabilities no longer matter much if you've isolated it into its
own VM with no access to resources beyond the ones it needs to decode a PNG
into a pixel buffer. I've been working on a little OS project along these
lines, where every process runs in its own VM, and every shared service is
provided by a local server. There's no dynamic linking. Instead of dealing
with the cost of constantly churning code, and all the instability and
unreliability that comes along with it, the idea is to pay for security up
front by isolating everything. If you want to decode a PNG you have to spin up
a VM, stream the PNG data in, and stream the decoded pixels out. The PNG
decoder has no access to the file system, network interfaces, user interfaces,
or really anything but stdin and stdout and some memory. If it crashes, it
crashes, and from the client's perspective the app just exited with a non-zero
error code.

~~~
epoch1970
> The PNG decoder has no access to the file system, network interfaces, user
> interfaces, or really anything but stdin and stdout and some memory.

So you've rediscovered what UNIX has allowed us to do for decades now.

Things like process isolation, memory isolation, file isolation, and hardware
isolation aren't new. UNIX systems offered mature implementations of them in
the 1980s, if not before then.

Best of all, this functionality was offered without needing to resort to
resource-intensive virtual machines or anything of that sort.

Those who want the level of isolation you propose have been able to get it for
a long time now. They just tend not to, because it's inconvenient and
impractical for real world systems.

~~~
wbl
That's just not true. A program needs to be started root to execute setuid(2):
there is no way for a program started by an unprivileged user to drop
privileges. Nor can it drop network privileges. That's why tools like
pledge(2) and seccomp had to be added, but they have their own problems. To
get the level of isolation GP described, you need some fairly esoteric
solutions.

~~~
ploxiln
Well seccomp() strict mode (only allows read, write, exit) has been available
since linux-2.6.12 (2005). So no need to invent something new, or set up a
bunch of cgroups and namespaces, to get processes that can only read stdin and
write stdout and use some of their own memory.

------
killercup
A good opportunity to check how [https://github.com/PistonDevelopers/image-
png](https://github.com/PistonDevelopers/image-png) is doing (a PNG decoder
written in Rust). Looks like it includes bindings to use miniz.c for DEFLATE
decoding as well as "inflate" (which seems to be DEFLATE in Rust). Also, it
seems to have a fuzzing driver (png-afl). Good times!

~~~
mike_hearn
There are PNG decoders written in a bunch of safe languages.

For instance the JVM uses a Java PNG decoder. So it isn't vulnerable.

~~~
kibwen
What killercup might be implying is that Rust is both a "safe language" and
that Rust code can expose a C-compatible ABI, which means that a library
written in Rust could theoretically replace one written in C regardless of
which other language is ultimately making use of it. For example, this is what
Mozilla is working on doing in Firefox by replacing security-conscious
components with Rust implementations (
[https://bugzilla.mozilla.org/show_bug.cgi?id=1151899](https://bugzilla.mozilla.org/show_bug.cgi?id=1151899)
,
[https://bugzilla.mozilla.org/show_bug.cgi?id=1161350](https://bugzilla.mozilla.org/show_bug.cgi?id=1161350)
).

However, I don't know if piston-image specifically provides such a C
interface.

~~~
tedunangst
However, if the bug is that the library writes to a too small allocation by
the application (based on a lie told it by the library) then it doesn't much
matter what language the library is in.

~~~
kibwen
I see there are some downvotes here, which I don't think are deserved. It's
true that at some level of your stack you're going to have some code that's
just poking bytes into buffers, and that generally takes some manual effort to
verify.

For what it's worth, I'd expect this sort of thing to be walled up behind an
`unsafe` block in Rust, which if nothing else would lend increased scrutiny in
an audit.

~~~
agwa
I believe the point tedunangst is trying to make is that if the API works
something like this:

    
    
      size_t sz = lib_get_buffer_size();
      char *buf = malloc(sz);
      lib_fill_buffer(buf);
    

and there's a bug in lib_get_buffer_size that causes it to return too small a
number, but lib_fill_buffer assumes it's big enough, you've got a
vulnerability regardless of what language the library is written in. This
isn't a bug in the application, because it's doing what it was told to do. It
will be hard to spot in an audit of the library because the bug is in
lib_get_buffer_size, which probably contains no unsafe blocks, while
lib_fill_buffer probably looks fine.

It's a deficiency in the API, which should require the buffer size to be
passed to lib_fill_buffer so lib_fill_buffer doesn't have to make any
assumptions about the size. But if you're trying to preserve compatibility
with existing C APIs, you might be stuck with APIs like this.

~~~
kibwen
To clarify further, the idiomatic usage of `unsafe` in Rust stipulates that if
you can't guarantee that your function is memory-safe for all possible inputs,
then you must mark the function itself as `unsafe` to force callers to be
aware of the risk. Obviously if you're both calling this theoretical function
from a language without an `unsafe` construct and if you're also striving to
maintain exact API compatibility with the C function then you can't really
make this aware to the caller. If you do have control of the API then the way
that this would generally be presented on the Rust side would be to have two
functions: a safe one named "foo" that also takes the length as an argument so
that you can check at runtime and an unsafe function named "unsafe_foo" that
has the same behavior as the C function.

------
whoopdedo

        Some applications might read the bit depth from the IHDR chunk and allocate
        memory for a 2^N entry palette, while libpng can return a palette with up
        to 256 entries even when the bit depth is less than 8.
    

On initial review, this will require assumptions in the client application to
be exploitable. I haven't found a place where libpng itself causes the
overrun. It is just an enabler by returning a smaller value for bit_depth than
num_palette. If the application double-checks the result from png_get_PLTE or
uses PNG_MAX_PALETTE_LENGTH instead of 1<<bit_depth, it won't have a problem
except wasting memory.

Unfortunately RFC 2083 contains the requirement that a compliant PLTE will not
be larger than the bit depth. So I'm sure many applications that trusted
libpng to do the validation will be exploitable. I consider this a caution
against promising too much in the spec, as well as not overly relying on an
implementation to exactly follow the spec. Remember the robustness principle:
be conservative in what you send, be liberal in what you accept.

~~~
revelation
Does anyone actually interface libpng themselves? The library is pure insanity
if all you care about is getting the image out in some format.

I'd wager that there is one libpng and then only about 4 or 5 distinct pieces
of code that use it that everyone else in turn is using. If that code is
wrong, that would still make it widely vulnerable.

~~~
whoopdedo
Github search: png_get_PLTE

    
    
        C    48892
        C++   2916
        Obj-C   36
    

Apple programmers aren't using it directly at least. Nor Android, which is not
exploitable[1]. Chromium also uses SKIA I believe. The C number is obviously
inflated by the number of repositories that contain a copy of libpng.

If everyone is using common glue code then I think it's more likely this won't
be a problem since the middleware library probably noticed the bug and works
around it. It's when developers hastily reinvent the wheel that sloppy
validation bugs like this are problems. It's why the maxim "never write your
own crypto" exists. Or as Mark Twain said[2], "put all your eggs in one basket
and _watch_that_basket_."

But as I said, there's no actual buffer overflow in libpng. This is a
validation bug. The headline and the NVD summary are misleading.

[1][https://android.googlesource.com/platform/external/skia/+/ma...](https://android.googlesource.com/platform/external/skia/+/master/src/codec/SkCodec_libpng.cpp#171)

[2]maybe

~~~
mdaniel
Ah, that's what I came to the comments to find out, having just heard about an
Android vulnerability in Chrome. The way the news of that vuln read (that one
just needs to visit a web page) I thought this vuln would certainly fit that
description.

------
Jerry2
From CVE:

>and 1.6.x before 1.6.19

Unfortunately, the latest version on libpng site [0] is 1.6.18.

Why was this CVE announced before the patch and version update was released?

[0]
[http://www.libpng.org/pub/png/libpng.html](http://www.libpng.org/pub/png/libpng.html)

~~~
ncw33
Actually, the patch was announced before the details of the bug: I subscribe
to the libpng mailing list, and got about a day's notice to ship the patch out
before the CVE and associated details were widely publicised. They did a good
job of notifying us quickly, before there was chance for the 'bad guys' to
jump on my customers.

~~~
cpncrunch
One day isn't really long enough. I see there is no update for Centos 6 as
yet.

------
0xcde4c3db
Do I understand correctly that for this to be potentially exploitable, the
application must allocate too little memory for the palette? In other words,
could this be mitigated on the application side by always allocating at least
enough memory for a 256-entry palette?

I don't mean to suggest that this is "really" an application bug or that
people shouldn't upgrade libpng ASAP; I'm just wondering whether there are
other options for installations/applications where that would be painful for
some reason.

------
userbinator
I wonder if this was found by fuzzing or inspection, because the former seems
a quite popular method recently.

~~~
wmt
The bug was found with American Fuzzy Lop from optipng
([https://bugs.debian.org/cgi-
bin/bugreport.cgi?bug=787647](https://bugs.debian.org/cgi-
bin/bugreport.cgi?bug=787647)) and later the optipng author noticed that the
same bug also affects libpng ([http://sourceforge.net/p/png-mng/mailman/png-
mng-implement/t...](http://sourceforge.net/p/png-mng/mailman/png-mng-
implement/thread/CA%2BPdXcuhLXJ89s6qjOEcm%2B99eWLmPBFcYSwcwJkajkLrNRLTeQ%40mail.gmail.com/#msg34581085))

------
deanclatworthy
What are the practical implications of this?

~~~
packetized
No input validation in the IHDR as to the number of bits in the palette of the
image. As I understand it, you just create an arbitrary image with the palette
in the IHDR tweaked to a large number of bits, and watch as the process dies
while trying to malloc() enough space to handle a palette that large.

[http://seclists.org/oss-sec/2015/q4/264](http://seclists.org/oss-
sec/2015/q4/264)

~~~
0xcde4c3db
I think it's the other way around: the malicious image specifies a _small_
bits-per-pixel value in IHDR (thus in some sense advertising that the palette
will have 2^bpp entries) but then contains a palette with more than 2^bpp
entries, which libpng will copy in its entirety when asked to read the
palette, overflowing the destination buffer if the application only allocated
enough memory for the expected number of entries.

~~~
cesarb
_If_ that's the case, _and_ bpp is always 8 or less, then software where the
programmers were "lazy" and always allocated 256 entries for the palette might
not be vulnerable.

From a quick search in MXR ([https://mxr.mozilla.org/mozilla-
central/](https://mxr.mozilla.org/mozilla-central/)) for the identifiers
"png_set_PLTE" and "png_set_PLTE", it seems that at least Mozilla (Firefox)
did the lazy thing and always allocated 256 entries.

Edit: I was right, from
[https://bugzilla.mozilla.org/show_bug.cgi?id=1224244](https://bugzilla.mozilla.org/show_bug.cgi?id=1224244)
"Mozilla is not vulnerable to the security issues that it fixes, when using
either the in-tree libpng or the system libpng."

------
comex
Right, so the announcement is kind of vague; let's see what's actually going
on. Here are the recent commits to libpng:

[https://github.com/glennrp/libpng/commits/libpng16](https://github.com/glennrp/libpng/commits/libpng16)

The first/latest one ("avoid potential pointer overflow") sounds scary, but I
believe it's just about the pathological case where the library is handed
(valid) buffers that extend to only a few bytes away from the maximum possible
value (i.e. 0xfffff...). If your OS does this, it probably shouldn't.

Going down the list, there are three relevant-looking commits. Listed in
chronological order:

[libpng16] Reject attempt to write over-length PLTE chunk
[https://github.com/glennrp/libpng/commit/81f44665cce4cb1373f...](https://github.com/glennrp/libpng/commit/81f44665cce4cb1373f049a76f3904e981b7a766)

[libpng16] Prevent reading over-length PLTE chunk (Cosmin Truta).
[https://github.com/glennrp/libpng/commit/a901eb3ce6087e0afee...](https://github.com/glennrp/libpng/commit/a901eb3ce6087e0afeef988247f1a1aa208cb54d)

[libpng16] Silently truncate over-length PLTE chunk while reading.
[https://github.com/glennrp/libpng/commit/1bef8e97995c3312366...](https://github.com/glennrp/libpng/commit/1bef8e97995c33123665582e57d3ed40b57d5978)

\- The first one only substantively changes png_write_PLTE, so it could only
be an issue for applications that actually write out PNGs. It changes a check
on the ‘num_pal’ argument from a hardcoded 256, aka PNG_MAX_PALETTE_LENGTH, to
be based on the bit depth. The png_write_PLTE function is internal and called
only from png_write_info, which passes it ‘info_ptr->palette’ and
‘info_ptr->num_palette’ as ‘palette’ and ‘num_pal’, respectively.

\- The second one confusingly changes png_write_PLTE again, but just to rename
a variable. The substantive change is a similar check on an argument to
png_set_PLTE, which is a _public_ function and, incidentally, the only thing
that can set ‘info_ptr->num_palette’, unless the application accesses it
directly, which is deprecated. (Thus, outside of the deprecated case, it
should ensure that the behavior change in the previous patch never actually
gets exercised.)

\- The third one adds yet another check, to png_handle_PLTE, which is called
when reading a PLTE chunk in a PNG file - this time, the variable is called
‘num’. After the check, ‘num’ is used to fill an array of size
PNG_MAX_PALETTE_LENGTH - but there was already an earlier check for the length
being too much for that, in which case it may longjmp out of the function with
png_chunk_error or just return. png_handle_PLTE then calls png_set_PLTE with
the same argument, so, in lieu of the added check, the previous commit’s check
would still trigger and fail the image load. The new check just changes an
error into something the library might be able to recover from.

So the second commit is the important one, and the third demonstrates how
png_set_PLTE can be called while reading a PNG with a size argument greater
than appropriate for the bit depth, but still <= PNG_MAX_PALETTE_LENGTH. What
happens then? The oss-security post says:

> Some applications might read the bit depth from the IHDR chunk and allocate
> memory for a 2^N entry palette, while libpng can return a palette with up to
> 256 entries even when the bit depth is less than 8.

So are only applications that do something odd affected? The png_get_PLTE
function gives the client the actual num_palette value as an out argument, so
any client that uses that to size buffers wouldn’t be affected; nor would one
that hardcoded 256 or PNG_MAX_PALETTE_LENGTH. For example, Skia does the
latter in SkImageDecoder_libpng.cpp.

Are there any libpng-internal uses that could be affected? I grepped for
‘->(num_)?palette\b’. TLDR: I don’t think so, but if you want the pointless
detail, among other less interesting uses were:

    
    
       /* Report invalid palette index; added at libng-1.5.10 */
       if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE &&
          png_ptr->num_palette_max > png_ptr->num_palette)
    

num_palette being too high could only make this fail, so it doesn’t matter.

png_image_read_header uses it to set image->colormap_entries, which has its
own long list of uses. I think this is then used (by the application, via
PNG_IMAGE_COLORMAP_SIZE) to determine the size of the colormap buffer, but
there may be an issue somewhere.

png_image_read_colormap uses it to generate actual color map data, but this is
not obviously dependent on the bit depth, and checked against
image->colormap_entries (…in case it changed from earlier?).

png_set_quantize uses a user-specified num_palette value and in any case is
not important functionality.

Some PNG_COMPOSE code uses num_palette to write to palette; not a problem…

png_do_expand_palette just looks up entries in palette (png_ptr->palette) by
byte-sized indices without checking anything. The palette buffer must be at
least 256 entries long, or else it will read out of bounds, which is
presumably why ->palette is only set here:

    
    
       /* Changed in libpng-1.2.1 to allocate PNG_MAX_PALETTE_LENGTH instead
        * of num_palette entries, in case of an invalid PNG file or incorrect
        * call to png_set_PLTE() with too-large sample values.
        */
       png_ptr->palette = png_voidcast(png_colorp, png_calloc(png_ptr,
           PNG_MAX_PALETTE_LENGTH * (sizeof (png_color))));
    
    

(it’s set differently in the dither case, which I don’t think is relevant?)
Anyway, it’s not affected by this.

png_handle_tRNS and png_handle_hIST have separate arrays whose count is
expected to be <= and ==, respectively, num_palette.

\- For trans, the ‘trans_alpha’ buffer itself is always PNG_MAX_PALETTE_LENGTH
sized, so there can be no overflow there; it also sets info_ptr->num_trans.
This can affect the application if it uses png_get_tRNS.

\- The hist buffer is also always maximum size and doesn’t have a separate
length field, so it’s covered by me checking uses of num_palette.

png_do_check_palette_indexes checks the bit-depth invariant separately.

hIST again in pngwrite.c and pngwutil.c, nothing else interesting in those
files.

I think num_trans is safe in the same way, but I haven’t looked as closely.

------
mmaunder
Curious if this affects PHP. AFAIKT GD is compiled with libpng for PHP (it's
required) and is presumably used for functions like getimagesize() in PHP.
That is used by WordPress core for uploads (although you need to be signed
in). However I suspect many plugins use GD for unauthenticated image upload,
so this could have a wide impact.

~~~
kalleboo
If I'm reading this comment
[https://news.ycombinator.com/item?id=10567353](https://news.ycombinator.com/item?id=10567353)
and this code
[https://github.com/libgd/libgd/blob/4751b606fa38edc456d62714...](https://github.com/libgd/libgd/blob/4751b606fa38edc456d627140898a7ec679fcc24/src/gd_png.c)
correctly, then it looks like GD isn't vulnerable since it uses num_palette to
size the palette rather than info_ptr->palette or bit_depth

------
jMyles
Unlike, for example, heartbleed, a vulnerability in libpng really requires
some contemplation to come up with all the places and ways that a typical
production application uses the vulnerable code.

I have web applications that probably use libpng five different ways, each
from a different client application and libpng version.

------
contingencies
This vulnerability is quite ... foretold. We have had issues with image
libraries before. Instead of just running about patching again for this one
bug, why not use the opportunity to discuss some architecture-level approaches
that may have mitigated this attack?

Knowing that image libraries may contain vulnerabilities, how would you design
a secure image ingestion process?

Some reasonable strategies...

(0) Attack surface minimization: Similarly to other types of input, input is
segregated and treated as toxic and dangerous until proven otherwise. (Eg.
Don't store random inbound files where you store those you will re-use; never
allow batch processes to touch temporary files received from external systems;
delete external input as soon as possible; store input only after
normalization)

(1) Attack surface minimization: only accept one file format. This provides no
guarantees but helps to limit exposure to unknown vulnerabilities, though may
not always be feasible.

(2) Parallelization (opposite of the above): explicitly convert all formats of
input to some common format (using throwaway containers/VMs), ideally using
multiple versions of multiple libraries on multiple operating systems, and
compare the results. If the results of every environment do not match, raise
an alarm and do not continue processing.

(3) Reverse-conversion check. Convert the file to another (non-lossy
conversion possible) format, then convert it back (also in a throwaway
environment). If the result matches (or the portion of the result that is non-
lossy matches) continue. This could be combined with #2 for additional
strength.

(4) "Application-level firewall" approach: progressively determine the various
format-specific and general properties of the image in question (eg. starting
with libmagic; and preferably all in relatively secured throwaway
environments, using the above strategies!) then apply a neural network or
basic statistical analysis to generate alerts for anomiles. For instance, the
reduced pallette featured in this attack would not be normal for many types of
image input (scans or photographs come to mind). Other useful checks may
include "are the image dimensions sane?", "is the aspect ratio sane?", "is the
image predominantly black and white?", etc.

Strategies #0 (in particular cases), #1 (with some luck), #2, #3 and #4 (with
some luck) could have mitigated this attack. Executed wrongly, any of those
strategies could also have triggered the vulnerability. #4 is ripe for a
web2.0 Silly-Valley outsource-the-problem startup (investors?).

PS. I enjoy these types of problems; anyone looking for ideas or development
get in touch.

------
pmalynin
Ah memories of flashing the PSP through an exploit in libtiff and ChickHen.
Perhaps people could do something similar with libpng?

------
alkonaut
To assess the impact of this I would really need to know a few factors (ball
park)

What fraction of applications that read png images use libpng?

How many of those applications have features that load 3rd party images
(uploadable avatar etc)?

My gut feeling is that the most common usage is for loading things like UI
elements, and not for external images? Are such programs safe from
exploitation?

------
ambrop7
Where does one get the patched libpng-1.6.19? The libpng site is broken for me
right now.

~~~
Natanael_L
[http://libpng.sourceforge.net/index.html](http://libpng.sourceforge.net/index.html)

~~~
whoopdedo
Sourceforge also seems to be under heavy load. I found this mirror:

[https://github.com/glennrp/linpng-
releases](https://github.com/glennrp/linpng-releases)

[https://github.com/glennrp/libpng](https://github.com/glennrp/libpng)

------
slicktux
I just compiled libpng-1.6.19, and then I read this post right after. XD

------
rasz_pl
how about a sample "dangerous" png image?

