Bug 191595

Summary: [FreeType] Problem under WebCore::FontPlatformData::FontPlatformData
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED MOVED    
Severity: Normal CC: aperez, bugs-noreply, cgarcia, clopez, ews-watchlist, magomez, mcatanzaro, mmaxfield
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Full backtrace
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews206 for win-future none

Description Michael Catanzaro 2018-11-13 13:17:02 PST
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
Comment 1 Michael Catanzaro 2018-11-13 13:17:23 PST
(This is with 2.22.3)
Comment 2 Carlos Garcia Campos 2018-11-19 02:24:32 PST
I can't reproduce this.
Comment 3 Carlos Garcia Campos 2018-11-19 03:40:38 PST
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.
Comment 4 Carlos Garcia Campos 2018-11-19 03:41:22 PST
See https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5
Comment 5 Michael Catanzaro 2018-11-19 08:08:17 PST
Thanks
Comment 6 Michael Catanzaro 2018-11-20 07:35:29 PST
Reopening
Comment 7 Michael Catanzaro 2018-11-20 07:41:56 PST
Closing again. :)
Comment 8 Michael Catanzaro 2018-11-20 07:42:25 PST
Actually let's use this bug to workaround the issue based on cairo version.
Comment 9 Michael Catanzaro 2018-11-20 08:19:28 PST
Created attachment 355346 [details]
Patch
Comment 10 Adrian Perez 2018-11-20 12:24:47 PST
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.
Comment 11 Michael Catanzaro 2018-11-20 15:30:26 PST
Comment on attachment 355346 [details]
Patch

Good catches, it's a disaster. Let me try again.
Comment 12 Michael Catanzaro 2018-11-20 18:45:19 PST
Created attachment 355376 [details]
Patch
Comment 13 Michael Catanzaro 2018-11-20 18:47:36 PST
Here's a fixed patch for unofficial review. We might as well just commit this to stable and not pollute trunk with it.
Comment 14 EWS Watchlist 2018-11-20 20:41:06 PST
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
Comment 15 EWS Watchlist 2018-11-20 20:41:17 PST
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
Comment 16 Carlos Garcia Campos 2018-11-21 01:14:26 PST
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
Comment 17 Michael Catanzaro 2018-11-21 04:31:36 PST
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.
Comment 18 Carlos Garcia Campos 2018-11-21 06:12:45 PST
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.
Comment 19 Michael Catanzaro 2018-11-21 08:32:52 PST
(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.
Comment 20 Michael Catanzaro 2018-11-21 08:52:39 PST
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. :)
Comment 21 Miguel Gomez 2020-03-17 01:55:41 PDT
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?
Comment 22 Michael Catanzaro 2020-03-17 10:04:04 PDT
https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/5 is merged...?
Comment 23 Miguel Gomez 2020-03-18 01:30:58 PDT
(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?
Comment 24 Michael Catanzaro 2020-03-18 07:14:20 PDT
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....
Comment 25 Michael Catanzaro 2020-03-18 07:21:59 PDT
Well, it's an invalid free, not a double free, but all the same applies.
Comment 26 Carlos Alberto Lopez Perez 2020-03-18 08:43:54 PDT
(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
Comment 27 Michael Catanzaro 2020-03-18 09:23:52 PDT
Ah cool, so I actually did the right thing for a change. :D
Comment 28 Michael Catanzaro 2020-03-18 09:35:04 PDT
(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. ;)
Comment 29 Michael Catanzaro 2020-03-18 12:21:41 PDT
(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...?
Comment 30 Miguel Gomez 2020-03-19 02:14:52 PDT
(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 :(