Bug 302858 – Double free / use-after-free of static FcPattern in FontPlatformDataFreeType::create

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Security] Double free / use-after-free of static FcPattern in FontPlatformDataFreeType::create

Hello WebKit Security Team,

We believe that we have discovered a potential security vulnerability in WebKit’s FreeType font pipeline that allows remote web content to trigger a double free and use-after-free of a shared `FcPattern`, leading to likely GPU-process code execution.

**Vulnerability Details**

The bug occurs in `FontPlatformData::create()` where a static `FcPattern*` is incorrectly shared across multiple `FontPlatformData` instances, each of which assumes exclusive ownership.

**Root Cause:**

1. **Static pattern creation** (line 327): A single `FcPattern*` is created once and cached as a static variable:
   ```cpp
   // Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
   static FcPattern* pattern = nullptr;
   static std::once_flag flag;
   std::call_once(flag, [](FcPattern*) {
       pattern = FcPatternCreate();  // Created once, refcount = 1
       // ... pattern initialization ...
   }, pattern);
   ```

2. **Multiple adoptions of the same pointer** (line 354): Every call to `create()` adopts the same static pointer:
   ```cpp
   return FontPlatformData(fontFace.get(), adoptRef(pattern), ...);
   ```

3. **The `adoptRef` problem**: The `adoptRef()` function transfers ownership to a `RefPtr` without incrementing the reference count. Unlike normal `RefPtr` construction (which calls `FcPatternReference`), `adoptRef` assumes the caller already owns the object and will destroy it when the `RefPtr` is destroyed.

   **Where this happens in code:**
   
   ```cpp
   // Line 354 in FontPlatformDataFreeType.cpp - THE BUG:
   return FontPlatformData(fontFace.get(), adoptRef(pattern), ...);
   ```
   
   **How `adoptRef` works (RefPtr.h:212-216):**
   ```cpp
   template<typename T, typename U, typename V>
   inline RefPtr<T, U, V> adoptRef(T* p)
   {
       adopted(p);
       return RefPtr<T, U, V>(p, RefPtr<T, U, V>::Adopt);  // Uses Adopt constructor
   }
   ```
   
   **The Adopt constructor (RefPtr.h:107):**
   ```cpp
   RefPtr(T* ptr, AdoptTag) : m_ptr(ptr) { }  // NO refIfNotNull() call!
   ```
   
   **Compare to normal RefPtr construction (RefPtr.h:49):**
   ```cpp
   RefPtr(T* ptr) : m_ptr(RefDerefTraits::refIfNotNull(ptr)) { }  // Calls FcPatternReference
   ```
   
   **What `refIfNotNull` does (RefPtrFontconfig.cpp:28-33):**
   ```cpp
   FcPattern* DefaultRefDerefTraits<FcPattern>::refIfNotNull(FcPattern* ptr)
   {
       if (ptr) [[likely]]
           FcPatternReference(ptr);  // Increments refcount - NOT called by adoptRef!
       return ptr;
   }
   ```
   
   So `adoptRef(pattern)` creates a `RefPtr` that stores the pointer directly without calling `FcPatternReference()`, meaning the refcount stays at 1 even though multiple `RefPtr` instances now "own" it.

4. **Destruction behavior**: When a `RefPtr<FcPattern>` is destroyed, it calls:
   ```cpp
   // Source/WebCore/platform/graphics/freetype/RefPtrFontconfig.cpp
   void DefaultRefDerefTraits<FcPattern>::derefIfNotNull(FcPattern* ptr)
   {
       if (ptr)
           FcPatternDestroy(ptr);  // Destroys the pattern unconditionally
   }
   ```
**Proposed Fix**
Create a cached base pattern, but duplicate it per `FontPlatformData` so each instance owns its copy:

```diff
diff --git a/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp b/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
--- a/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
+++ b/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
@@
-    static FcPattern* pattern = nullptr;
-    static bool fixedWidth = false;
+    static RefPtr<FcPattern> basePattern;
+    static bool fixedWidth = false;
@@
-        std::call_once(flag, [](FcPattern*) {
-            pattern = FcPatternCreate();
-            FcConfigSubstitute(nullptr, pattern, FcMatchPattern);
-            cairo_ft_font_options_substitute(getDefaultCairoFontOptions(), pattern);
-            FcDefaultSubstitute(pattern);
-            FcPatternDel(pattern, FC_FAMILY);
-            FcConfigSubstitute(nullptr, pattern, FcMatchFont);
-        }, pattern);
-
-        int spacing;
-        if (FcPatternGetInteger(pattern, FC_SPACING, 0, &spacing) == FcResultMatch && spacing == FC_MONO)
-            fixedWidth = true;
-        fontFace = adoptRef(cairo_ft_font_face_create_for_pattern(pattern));
+        std::call_once(flag, [&] {
+            basePattern = adoptRef(FcPatternCreate());
+            FcConfigSubstitute(nullptr, basePattern.get(), FcMatchPattern);
+            cairo_ft_font_options_substitute(getDefaultCairoFontOptions(), basePattern.get());
+            FcDefaultSubstitute(basePattern.get());
+            FcPatternDel(basePattern.get(), FC_FAMILY);
+            FcConfigSubstitute(nullptr, basePattern.get(), FcMatchFont);
+            int spacing;
+            if (FcPatternGetInteger(basePattern.get(), FC_SPACING, 0, &spacing) == FcResultMatch && spacing == FC_MONO)
+                fixedWidth = true;
+        });
+        RefPtr<FcPattern> pattern = adoptRef(FcPatternDuplicate(basePattern.get()));
+        fontFace = adoptRef(cairo_ft_font_face_create_for_pattern(pattern.get()));
+        return FontPlatformData(fontFace.get(), pattern.releaseNonNull(), data.m_size, fixedWidth, data.m_syntheticBold, data.m_syntheticOblique, data.m_orientation, custom);
     }
-    return FontPlatformData(fontFace.get(), adoptRef(pattern), data.m_size, fixedWidth, data.m_syntheticBold, data.m_syntheticOblique, data.m_orientation, custom);
+    RefPtr<FcPattern> pattern = adoptRef(FcPatternDuplicate(basePattern.get()));
+    return FontPlatformData(fontFace.get(), pattern.releaseNonNull(), data.m_size, fixedWidth, data.m_syntheticBold, data.m_syntheticOblique, data.m_orientation, custom);
```

Best wishes,
Luigino Camastra
Aisle Research

[Radar WebKit Bug Importer]

<rdar://problem/165124069>

[Michael Catanzaro]

Instead of duplicating the pattern, I assume we can probably just ref it by removing the adoptRef:

diff --git a/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp b/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
index bfe9b209a1e9..521d7c7bb4b6 100644
--- a/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
+++ b/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
@@ -351,7 +351,7 @@ FontPlatformData FontPlatformData::create(const Attributes& data, const FontCust
         fontFace = adoptRef(cairo_ft_font_face_create_for_pattern(pattern));
     }
 
-    return FontPlatformData(fontFace.get(), adoptRef(pattern), data.m_size, fixedWidth, data.m_syntheticBold, data.m_syntheticOblique, data.m_orientation, custom);
+    return FontPlatformData(fontFace.get(), pattern, data.m_size, fixedWidth, data.m_syntheticBold, data.m_syntheticOblique, data.m_orientation, custom);
 }
 
 FontPlatformData::Attributes FontPlatformData::attributes() const

To test the fix, we need to build with -DUSE_CAIRO=ON, which is normally only used on big endian architectures like s390x.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> To test the fix, we need to build with -DUSE_CAIRO=ON, which is normally
> only used on big endian architectures like s390x.

I'm just going to make sure that the change builds successfully. Whether WebKit actually works on these architectures can be left as an exercise to whoever cares.

[Michael Catanzaro]

This code is actually also used by PlayStation port.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/54346

[Michael Catanzaro]

I was planning to create the pull request in the security remote instead of the normal remote, but it seems I failed. Oh well. The bug does not affect Apple platforms, so not a big deal.

Thank you, could you please clarify the process WebKit follows for generating CVEs? Additionally, I’d like to know whether CVE creation should be initiated from my side or if it is handled on your end. Thank you.

[Michael Catanzaro]

Since this bug does not affect Apple, we can request a CVE from MITRE or Red Hat. In this case, I decided not to spend time on it since the impact is so low: this bug only affects PlayStation, which I believe does not allow viewing untrusted web content, and also WPE WebKit or WebKitGTK on big endian platforms like IBM mainframes. Or also obsolete versions predating the Skia backend. Or anybody who is silly enough to build with -DUSE_CAIRO=ON using a little endian architecture, which is probably almost nobody.

That said, it's a valid vulnerability, so feel free to request a CVE yourself if you wish. Just let us know here when you have a CVE ID.

[EWS]

Committed 303513@main (d4f46e50d65d): <https://commits.webkit.org/303513@main>

Reviewed commits have been landed. Closing PR #54346 and removing active labels.