Bug 191595 – [FreeType] Problem under WebCore::FontPlatformData::FontPlatformData

• Component: WebKitGTK
• Status: RESOLVED MOVED

[Michael Catanzaro]

Created attachment 354692 [details]
Full backtrace

Reproducer: https://thepointsguy.com/2017/09/experience-only-on-virgin-america/

Full backtrace attached. Truncated backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fee8eade515 in __GI_abort () at abort.c:79
#2  0x00007fee8eb354c8 in __libc_message (action=action@entry=do_abort, 
    fmt=fmt@entry=0x7fee8ec3e1fb "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007fee8eb3b8da in malloc_printerr (str=str@entry=0x7fee8ec3c402 "free(): invalid pointer")
    at malloc.c:5350
#4  0x00007fee8eb3d06c in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>)
    at malloc.c:4157
#5  0x00007fee8fedd993 in cairo_ft_apply_variations (face=face@entry=0x7fed3aba7100, 
    scaled_font=<optimized out>, scaled_font=<optimized out>) at ../../src/cairo-ft-font.c:2396
#6  0x00007fee8fee0a7f in cairo_ft_scaled_font_lock_face (
    abstract_font=abstract_font@entry=0x55c6020ea1b0) at ../../src/cairo-ft-font.c:3859
#7  0x00007fee95085334 in WebCore::CairoFtFaceLocker::CairoFtFaceLocker (scaledFont=0x55c6020ea1b0, 
    this=<synthetic pointer>) at ../Source/WebCore/platform/graphics/cairo/CairoUtilities.h:55
#8  WebCore::FontPlatformData::FontPlatformData (this=0x7ffc53bd7d30, fontFace=<optimized out>, 
    description=..., bold=<optimized out>, italic=<optimized out>)
    at ../Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:168
#9  0x00007fee95084873 in WebCore::FontCustomPlatformData::fontPlatformData (this=<optimized out>, 
    description=..., bold=<optimized out>, italic=<optimized out>)
    at ../Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:61
#10 0x00007fee949767f4 in WebCore::CachedFont::platformDataFromCustomData (fontCustomPlatformData=..., 
    fontDescription=..., bold=<optimized out>, italic=<optimized out>, fontFaceFeatures=..., 
    fontFaceVariantSettings=..., fontFaceCapabilities=...)
    at ../Source/WebCore/loader/cache/CachedFont.cpp:150

[Michael Catanzaro]

(This is with 2.22.3)

[Carlos Garcia Campos]

I can't reproduce this.

[Carlos Garcia Campos]

Looking at the bt, I think the problem is that cairo is trying to free a FT_MM_Var with free when using freetype 2.9. I can't reproduce it because i have freetype 2.8.1.

[Carlos Garcia Campos]

See https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5

[Michael Catanzaro]

Thanks

[Michael Catanzaro]

Reopening

[Michael Catanzaro]

Closing again. :)

[Michael Catanzaro]

Actually let's use this bug to workaround the issue based on cairo version.

[Michael Catanzaro]

Created attachment 355346 [details]
Patch

[Adrian Perez]

Comment on attachment 355346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355346&action=review

There are some changes needed in this patch, please take a look.

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:71
> +    if (version >= CAIRO_VERSION_ENCODE(1, 15, 0) && version < CAIRO_VERSION_ENCODE(1, 16, 1)) {

If I understand correctly, we want to *avoid* using fastMalloc() and friends
for Cairo 1.16.0, but here the code is *enabling* their usage for Cairo
versions between [1.15.0, 1.16.1) — which goes against the comment above.
I think here the condition should be reversed:

    if (version < CAIRO_VERSION_ENCODE(1, 16, 0) && version >= CAIRO_VERSION_ENCODE(1, 16, 1))

This way the custom memory allocator is set for every version except the ones
in the range [1.16.0, 1.16.1).

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:91
>      }

When “FT_New_Library” is not being used, FreeType is not being initialized
at all! You will want to initialize the library using “FT_Init_FreeType()”
in an else branch. The following is missing:

   else {
       if (FT_Init_FreeType(&library))
           return false;
   }

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:93
>      FT_Add_Default_Modules(library);

You will want to move this call to “FT_Add_Default_Modules()” above, right
after using “FT_New_Library()”. It is not needed to call this when using
“FT_Init_FreeType()” because it's called implicitly in that case.

[Michael Catanzaro]

Comment on attachment 355346 [details]
Patch

Good catches, it's a disaster. Let me try again.

[Michael Catanzaro]

Created attachment 355376 [details]
Patch

[Michael Catanzaro]

Here's a fixed patch for unofficial review. We might as well just commit this to stable and not pollute trunk with it.

[EWS Watchlist]

Comment on attachment 355376 [details]
Patch

Attachment 355376 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10094493

New failing tests:
webanimations/leak-document-with-web-animation.html

[EWS Watchlist]

Created attachment 355380 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Carlos Garcia Campos]

Comment on attachment 355376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355376&action=review

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:71
> +    // Workaround crash in cairo 1.16.0. See https://webkit.org/b/191595.
> +    int version = cairo_version();
> +    if (version >= CAIRO_VERSION_ENCODE(1, 15, 0) && version < CAIRO_VERSION_ENCODE(1, 16, 1)) {

This is only broken if freetype version is >= 2.9

[Michael Catanzaro]

Are you sure? I can't reproduce the crash anymore either: that doesn't mean it's not a security problem. FT_Get_MM_Var was added in 2004 and I can see it's been using FT_ALLOC (in TT_Get_MM_Var) since then, so my guess is it has never been tested with custom allocators and the first cairo version to use it (1.16) is the first bad version.

[Carlos Garcia Campos]

Yes, I guess previous versions of freetype used malloc directly, the docs said you had to free it with free (not with FC functions). It doesn't crash for me with current cairo.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #18)
> Yes, I guess previous versions of freetype used malloc directly, the docs
> said you had to free it with free (not with FC functions).

I think the docs were wrong and there was no safe way to use it before.

> It doesn't crash for me with current cairo.

Me either, that's just luck I think.

[Michael Catanzaro]

Committed workaround to 2.22 branch only. This should be fixed in cairo long before 2.24 and we don't have to argue about whether to check the freetype version or not if it's only on 2.22. :)

[Miguel Gomez]

This hasn't been fixed in cairo, and it's affecting 2.26 and 2.28 as well. Maybe we should add this workaround there too?

[Michael Catanzaro]

https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/5 is merged...?

[Miguel Gomez]

(In reply to Michael Catanzaro from comment #22)
> https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/5 is merged...?

But there hasn't been any new release that includes the fix (other than the 1.17.2 snapshot), or am I wrong?

[Michael Catanzaro]

Ugh, you're right. I didn't realize it was a snapshot release. :(

It's not WebKit's responsibility to work around cairo vulnerabilities forever. We could report a CVE against cairo to get them to fix it. It's a double free that can be triggered by remote web content, right? (Do you have an updated reproducer? Something that works today? My original reproducer doesn't work anymore.) Anyway, that's the recipe for a remote code execution CVE. Good way to magically attract attention to an issue. Should have done that way back when the issue was originally reported, but oh well....

[Michael Catanzaro]

Well, it's an invalid free, not a double free, but all the same applies.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #24)
> Ugh, you're right. I didn't realize it was a snapshot release. :(
> 
> It's not WebKit's responsibility to work around cairo vulnerabilities
> forever. We could report a CVE against cairo to get them to fix it. It's a
> double free that can be triggered by remote web content, right? (Do you have
> an updated reproducer? Something that works today? My original reproducer
> doesn't work anymore.) Anyway, that's the recipe for a remote code execution
> CVE. Good way to magically attract attention to an issue. Should have done
> that way back when the issue was originally reported, but oh well....

But you already asked for one? you comment about CVE-2018-19876 on https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/5

[Michael Catanzaro]

Ah cool, so I actually did the right thing for a change. :D

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> Committed workaround to 2.22 branch only. This should be fixed in cairo long
> before 2.24 and we don't have to argue about whether to check the freetype
> version or not if it's only on 2.22. :)

BTW: if you ever need concrete proof that I am a fool, this is it right here, forever enshrined in Bugzilla. ;)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #24)
> (Do you have
> an updated reproducer? Something that works today? My original reproducer
> doesn't work anymore.)

Miguel, did you find a layout test that was hitting this? Or just general web browsing...?

[Miguel Gomez]

(In reply to Michael Catanzaro from comment #29)
> (In reply to Michael Catanzaro from comment #24)
> > (Do you have
> > an updated reproducer? Something that works today? My original reproducer
> > doesn't work anymore.)
> 
> Miguel, did you find a layout test that was hitting this? Or just general
> web browsing...?

I hit it when browsing a client's test page, both testing 2.26 and 2.28, but I'm afraid I cannot share the link :(