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_)

Created attachment 132557 [details]
Proof of concept

Created attachment 132562 [details] [review]
Check cmap size before allocating

Patch to check the size before allocating cmap table.

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().

Yes, _cairo_malloc() looks like it should be used more uniformly to avoid this bug.

Adrian, will you be landing this patch or is there any additional work needed?

At this point, the patch needs rebased to be landable.  Would anyone have a problem if I went ahead and did so?

(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?

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().

Created attachment 139090 [details] [review]
Check cmap size before allocating and replace malloc() with _cairo_malloc().

Created attachment 139094 [details] [review]
Check cmap size before allocating and replace malloc() with _cairo_malloc().

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.

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?

-- 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.