Bug 101547 - Heap buffer overflow at cairo-truetype-subset.c:1299, CVE-2017-9814
Summary: Heap buffer overflow at cairo-truetype-subset.c:1299, CVE-2017-9814
Status: RESOLVED MOVED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All All
: medium major
Assignee: Chris Wilson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-21 18:41 UTC by foca@salesforce.com
Modified: 2018-08-25 13:55 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Proof of concept (8.09 KB, application/pdf)
2017-07-07 21:36 UTC, foca@salesforce.com
Details
Check cmap size before allocating (915 bytes, patch)
2017-07-08 00:04 UTC, Adrian Johnson
Details | Splinter Review
Replace malloc() with _cairo_malloc() (117.11 KB, patch)
2017-07-08 00:07 UTC, Adrian Johnson
Details | Splinter Review
Check cmap size before allocating and replace malloc() with _cairo_malloc(). (145.95 KB, patch)
2018-04-25 03:17 UTC, qzheng
Details | Splinter Review
Check cmap size before allocating and replace malloc() with _cairo_malloc(). (144.28 KB, patch)
2018-04-25 03:42 UTC, qzheng
Details | Splinter Review
Check cmap size before allocating and replace malloc() with _cairo_malloc(). (143.90 KB, patch)
2018-04-25 06:43 UTC, qzheng
Details | Splinter Review
Convert boilerplate code to _cairo_malloc (5.64 KB, patch)
2018-05-08 01:03 UTC, Bryce Harrington
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description foca@salesforce.com 2017-06-21 18:41:23 UTC
Hi, 

I would like to report the details of this vulnerability in private to follow a responsible disclosure: https://en.wikipedia.org/wiki/Responsible_disclosure

Could you make this ticket private? Is it that possible?

Thank you.
Comment 1 foca@salesforce.com 2017-07-07 21:35:37 UTC
The CVE-2017-9814 has been assigned to this vulnerability.

There is a read out of bounds bug at cairo-truetype-subset.c:1299:

1293     size = be16_to_cpu (map->length);
1294     map = malloc (size);
1295     if (unlikely (map == NULL))
1296         return _cairo_error (CAIRO_STATUS_NO_MEMORY);
1297 
1298     status = backend->load_truetype_table (scaled_font,
1299                                            TT_TAG_cmap, table_offset,
1300                                            (unsigned char *) map,
1301                                            &size);
1302     if (unlikely (status))
1303         goto fail;
1304 
1305     num_segments = be16_to_cpu (map->segCountX2)/2;

The bug happens because in some scenarios the variable size can have a value of 0 at line 1288. And malloc(0) is not returning NULL as some people could expect: https://stackoverflow.com/questions/1073157/zero-size-malloc

malloc(0) returns the smallest chunk possible. So the line 1290 with the return is not execute. And the execution continues with an invalid map.

Since the size is 0 the variable map is not initialized correctly at load_trutype_table. So, later when the variable map is accessed previous values from a freed chunk are used. This could allows an attacker to control the variable map.

There is a check performed just after the bug:
1309     if (size < (8 + 4*num_segments)*sizeof(uint16_t))
1310         return CAIRO_INT_STATUS_UNSUPPORTED;

So it’s likely the attacker can't control the variable num_segments, and he can't trigger additional functionality to leverage this.

The solution could be to check for the size, or to use a malloc wrapper that handle the size = 0 case and returns NULL.

This bug was found when using a poppler util, pdftocairo. A PoC is attached. To reproduce the bug use:
pdftocairo -svg PoC.pdf

This vulnerability has been found by Offensive Research at Salesforce.com:
Alberto Garcia (@algillera), Francisco Oca (@francisco_oca) & Suleman Ali (@Salbei_)
Comment 2 foca@salesforce.com 2017-07-07 21:36:26 UTC
Created attachment 132557 [details]
Proof of concept
Comment 3 Adrian Johnson 2017-07-08 00:04:50 UTC
Created attachment 132562 [details] [review]
Check cmap size before allocating

Patch to check the size before allocating cmap table.
Comment 4 Adrian Johnson 2017-07-08 00:07:33 UTC
Created attachment 132563 [details] [review]
Replace malloc() with _cairo_malloc()

> The solution could be to check for the size, or to use a malloc wrapper
> that handle the size = 0 case and returns NULL.

We already have a wrapper that checks size = 0: _cairo_malloc(). It has not been used consistently. The attached patch replaces all calls to malloc() with _cairo_malloc().
Comment 5 Bryce Harrington 2017-07-10 17:08:24 UTC
Yes, _cairo_malloc() looks like it should be used more uniformly to avoid this bug.
Comment 6 Bryce Harrington 2017-07-10 17:10:21 UTC
Adrian, will you be landing this patch or is there any additional work needed?
Comment 7 Bryce Harrington 2017-09-20 21:32:30 UTC
At this point, the patch needs rebased to be landable.  Would anyone have a problem if I went ahead and did so?
Comment 8 qzheng 2018-04-25 02:37:56 UTC
(In reply to Bryce Harrington from comment #7)
> At this point, the patch needs rebased to be landable.  Would anyone have a
> problem if I went ahead and did so?

This patch has not been landed, am I correct?
Comment 9 qzheng 2018-04-25 03:17:32 UTC
Created attachment 139088 [details] [review]
Check cmap size before allocating and replace malloc() with _cairo_malloc().

Check cmap size before allocating and replace malloc() with _cairo_malloc().
Comment 10 qzheng 2018-04-25 03:42:57 UTC
Created attachment 139090 [details] [review]
Check cmap size before allocating and replace malloc() with _cairo_malloc().
Comment 11 qzheng 2018-04-25 06:43:02 UTC
Created attachment 139094 [details] [review]
Check cmap size before allocating and replace malloc() with _cairo_malloc().
Comment 12 Bryce Harrington 2018-05-07 23:42:01 UTC
I've gone ahead and rebased and landed Adrian's patch.

To ssh://git.freedesktop.org/git/cairo
   7554822..1998239  master -> master


qzheng's patch demonstrates there are additional malloc()'s worth reviewing as followup work, esp. in boilerplate.  However, the patch includes some s/malloc/_cairo_malloc/ changes that don't look quite right, such as:

-#include "cairo-malloc-private.h"
+#include "cairo-_cairo_malloc-private.h"

Some of the changes to tests and docs may not strictly be needed.  Also, we have cases of malloc(unsigned + number) which won't ever trigger the malloc(0) error; should we still change these cases for consistency, or leave them as is?

I'm going to leave this bug open until at least the mallocs in boilerplate/ have been applied.
Comment 13 Bryce Harrington 2018-05-08 01:03:09 UTC
Created attachment 139416 [details] [review]
Convert boilerplate code to _cairo_malloc

Here's a shot at converting the remaining malloc()'s in boilerplate/ to use _cairo_malloc.  I notice though that the boilerplate code has its own xmalloc() routine that calls exit() on error; I'm not certain whether or not that should be used here instead of _cairo_malloc?
Comment 14 GitLab Migration User 2018-08-25 13:55:15 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/264.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.