Use after free

[eleetas]

We ran some tests on an older version of libpng and found a use-after-free bug while running one the test cases. It looks like the bug is still there in the latest code. Here's the output from our tool

DTS_MSG: Stensal DTS detected a fatal program error!
DTS_MSG: Continuing the execution will cause unexpected behaviors, abort!
DTS_MSG: Access the memory block that is freed.
DTS_MSG: Diagnostic information:

Caution: the allocation info is correct only if the freed memory is not reused.
the memory block (start:0xffb4269c, size:24 bytes) was allocated at
file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/png.c::4451, 16
Stack trace (most recent call first):
-[1] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/pngerror.c::954, 4
-[2] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/png.c::4517, 13
-[3] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/pngread.c::4242, 19
-[4] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/contrib/libtests/pngstest.c::3024, 16
-[5] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/contrib/libtests/pngstest.c::3120, 11
-[6] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/contrib/libtests/pngstest.c::3446, 13
-[7] file:/home/sbuilder/workspace/stensal/aports/main/libpng/src/libpng-1.6.25/contrib/libtests/pngstest.c::3664, 20
-[8] file:/musl-1.1.10/src/env/__libc_start_main.c::180, 11

Basically, png_image_free() calls png_safe_execute(image, png_image_free_function, image). In png_safe_execute you have the following code:

if (result != 0)
  {

     image->opaque->error_buf = safe_jmpbuf;
     result = function(arg);         // <------png_image_free_function()
  }

  image->opaque->error_buf = saved_error_buf;

When result is != 0, image is freed, but then image->opaque->error_buf is assigned directly after.

[kcc]

I am seeing the same bug on the current master.
Under AddressSanitizer it manifests as stack-use-after-return.
Exact repro steps:

#!/bin/bash

[ ! -e libpng ] && git clone https://github.com/glennrp/libpng.git
(cd libpng; CC="clang" CFLAGS="-O1 -fsanitize=address -g"  ./configure && make -j $(nproc))

cat << EOF > png_uar.c
#include <string.h>
#include "png.h"

int main() {
  unsigned char data[] = {
      0x89, 0x50, 0x4e, 0x47, 0xd,  0xa,  0x1a, 0xa,  0x0,  0x0, 0x0, 0xd,
      0x49, 0x48, 0x44, 0x52, 0x0,  0x0,  0x0,  0x0,  0x0,  0x0, 0x0, 0x0,
      0x0,  0x0,  0x0,  0x0,  0x0,  0x2e, 0x90, 0x68, 0xf,  0x0, 0x0, 0x0,
      0x0,  0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
  };
  png_image image;
  memset(&image, 0, sizeof(image));
  image.version = PNG_IMAGE_VERSION;
  png_image_begin_read_from_memory(&image, data, sizeof(data));
}
EOF
clang -fsanitize=address png_uar.c libpng/.libs/libpng16.a -lz
ASAN_OPTIONS=detect_stack_use_after_return=1 ./a.out

Full report:

==11009==ERROR: AddressSanitizer: stack-use-after-return on address 0x7fa4241440b0 at pc 0x000000542f89 bp 0x7ffd487a2cf0 sp 0x7ffd487a2ce8
WRITE of size 8 at 0x7fa4241440b0 thread T0
    #0 0x542f88 in png_safe_execute /tmp/libpng/pngerror.c:954:29
    #1 0x53effe in png_image_free /tmp/libpng/png.c:4592:13
    #2 0x542e6d in png_safe_execute /tmp/libpng/pngerror.c:958:7
    #3 0x4f8c7a in main (/tmp/a.out+0x4f8c7a)
    #4 0x7fa4276b62b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #5 0x41dbc9 in _start (/tmp/a.out+0x41dbc9)

Address 0x7fa4241440b0 is located in stack of thread T0 at offset 48 in frame
    #0 0x53f03f in png_image_free_function /tmp/libpng/png.c:4523

  This frame has 1 object(s):
    [32, 80) 'c' (line 4526) <== Memory access at offset 48 is inside this variable

Discovered by extending a fuzz target on oss-fuzz

[kcc]

and here is the oss-fuzz report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12803

[ctruta]

There are two important conditions that must be met in general:

• png_image_free (or any other destructor) must never fail.
• png_image_free (or any other destructor) needs not (and must not) be executed under png_safe_execute.

The fix is simple: call png_image_free directly, without going via png_safe_execute.

diff --git a/png.c b/png.c
index 9d9926f638..efd1aecfbd 100644
--- a/png.c
+++ b/png.c
@@ -4588,8 +4588,7 @@ png_image_free(png_imagep image)
    if (image != NULL && image->opaque != NULL &&
       image->opaque->error_buf == NULL)
    {
-      /* Ignore errors here: */
-      (void)png_safe_execute(image, png_image_free_function, image);
+      png_image_free_function(image);
       image->opaque = NULL;
    }
 }

[ctruta]

I recommend opening a CVE. The next libpng update will contain this fix.

[eleetas]

Filed a request for a CVE.

[eleetas]

This issue has been assigned CVE-2019-7317.

[ddillard]

Can the affected versions in the CVE be updated? Currently it says it only affects 1.6.36 instead of all versions of 1.6 through 1.6, all versions of 1.4 through ..., etc.

[pghmcfc]

png_safe_execute() isn't present prior to libpng 1.6 so I guess 1.5.x and older versions are not affected by this?

[ddillard]

Ok, if that's the case then I suggest updating the CVE to say that versions 1.6 through 1.6.36 are affected and having the description explicitly call out that earlier versions are not affected - because people using earlier versions will ask if that's not included.

[tomashek]

Is the patch recommended above by ctruta the accepted fix for CVE-2019-7317?

[ctruta]

I will integrate it in the upcoming libpng-1.6.37.

[kcc]

Any chance to submit this patch to the master branch now?
This bug affects our automated fuzzing... (#274)

[strongcourage]

Which commands should I use for reproducing the bug? Thanks.

[ctruta]

Which commands should I use for reproducing the bug? Thanks.

See the comment by @kcc on Feb.1, at the top of this issue.

[ctruta]

Any chance to submit this patch to the master branch now?
This bug affects our automated fuzzing... (#274)

Sorry @kcc about the delay, yes I should have pushed it...
The fix is now in the master branch.

[LocutusOfBorg]

Can we then close this one?

[kcc]

Thanks! Is that a627bd2 ?
While we are here, would anyone be able to work on extending the existing libpng fuzz target?
#274

[LocutusOfBorg]

https://salsa.debian.org/debian/libpng1.6/blob/master/debian/patches/series
I used commits:
70d122a
8439534
82ae623
a627bd2
and
9c0d5c7 (CVE-2019-7317)
on top of 1.6.36

[ctruta]

Published libpng version 1.6.37, including this fix.