Summary: | [FreeType] Problem under WebCore::FontPlatformData::FontPlatformData | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||
Component: | WebKitGTK | Assignee: | 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
Michael Catanzaro
2018-11-13 13:17:02 PST
(This is with 2.22.3) I can't reproduce this. 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. Thanks Reopening Closing again. :) Actually let's use this bug to workaround the issue based on cairo version. Created attachment 355346 [details]
Patch
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 on attachment 355346 [details]
Patch
Good catches, it's a disaster. Let me try again.
Created attachment 355376 [details]
Patch
Here's a fixed patch for unofficial review. We might as well just commit this to stable and not pollute trunk with it. 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 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 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 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. 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. (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. 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. :) 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? (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? 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.... Well, it's an invalid free, not a double free, but all the same applies. (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 Ah cool, so I actually did the right thing for a change. :D (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. ;) (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...? (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 :( |