CVE-2018-7999: NULL Pointer Dereference (CWE-476)
value: In libgraphite2 in graphite2 1.3.11, a NULL pointer dereference vulnerability was found in Segment.cpp during a dumbRendering operation, which may allow attackers to cause a denial of service or possibly have unspecified other impact via a crafted .ttf file.
CWE description: The product dereferences a pointer that it expects to be valid but is NULL.
CVSS v3 base score: 8.8
Published: 2018-03-09T19:29Z
Repository: https://github.com/silnrsi/graphite
Language: C++
Files changed: include/graphite2/Font.h, src/GlyphCache.cpp, src/gr_face.cpp, tests/featuremap/CMakeLists.txt, tests/featuremap/featuremaptest.cpp, tests/vm/CMakeLists.txt
Commit date: 2018-03-06 12:42:16 +0700
Commit message: Deprecate and make ineffective gr_face_dumbRendering

Diff: diff --git a/include/graphite2/Font.h b/include/graphite2/Font.h
index efe2af9..a4e3524 100644
--- a/include/graphite2/Font.h
+++ b/include/graphite2/Font.h
@@ -54,11 +54,11 @@ GR2_API void gr_engine_version(int *nMajor, int *nMinor, int *nBugFix);
 * The values can be combined 
 */
 enum gr_face_options {
     /** No preload, no cmap caching, fail if the graphite tables are invalid */
     gr_face_default = 0,
-    /** Dumb rendering will be enabled if the graphite tables are invalid */
+    /** Dumb rendering will be enabled if the graphite tables are invalid. DEPRECATED. */
     gr_face_dumbRendering = 1,
     /** preload glyphs at construction time */
     gr_face_preloadGlyphs = 2,
     /** Cache the lookup from code point to glyph ID at construction time */
     gr_face_cacheCmap = 4,
diff --git a/src/GlyphCache.cpp b/src/GlyphCache.cpp
index c4ab807..1acf7f9 100644
--- a/src/GlyphCache.cpp
+++ b/src/GlyphCache.cpp
@@ -82,11 +82,11 @@ const SlantBox SlantBox::empty = {0,0,0,0};
 
 
 class GlyphCache::Loader
 {
 public:
-    Loader(const Face & face, const bool dumb_font);    //return result indicates success. Do not use if failed.
+    Loader(const Face & face);    //return result indicates success. Do not use if failed.
 
     operator bool () const throw();
     unsigned short int units_per_em() const throw();
     unsigned short int num_glyphs() const throw();
     unsigned short int num_attrs() const throw();
@@ -113,11 +113,11 @@ private:
 };
 
 
 
 GlyphCache::GlyphCache(const Face & face, const uint32 face_options)
-: _glyph_loader(new Loader(face, bool(face_options & gr_face_dumbRendering))),
+: _glyph_loader(new Loader(face)),
   _glyphs(_glyph_loader && *_glyph_loader && _glyph_loader->num_glyphs()
         ? grzeroalloc<const GlyphFace *>(_glyph_loader->num_glyphs()) : 0),
   _boxes(_glyph_loader && _glyph_loader->has_boxes() && _glyph_loader->num_glyphs()
         ? grzeroalloc<GlyphBox *>(_glyph_loader->num_glyphs()) : 0),
   _num_glyphs(_glyphs ? _glyph_loader->num_glyphs() : 0),
@@ -237,11 +237,11 @@ const GlyphFace *GlyphCache::glyph(unsigned short glyphid) const      //result m
     return p;
 }
 
 
 
-GlyphCache::Loader::Loader(const Face & face, const bool dumb_font)
+GlyphCache::Loader::Loader(const Face & face)
 : _head(face, Tag::head),
   _hhea(face, Tag::hhea),
   _hmtx(face, Tag::hmtx),
   _glyf(face, Tag::glyf),
   _loca(face, Tag::loca),
@@ -263,56 +263,53 @@ GlyphCache::Loader::Loader(const Face & face, const bool dumb_font)
     {
         _head = Face::Table();
         return;
     }
 
-    if (!dumb_font)
+    if ((m_pGlat = Face::Table(face, Tag::Glat, 0x00030000)) == NULL
+        || (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
+        || m_pGloc.size() < 8)
     {
-        if ((m_pGlat = Face::Table(face, Tag::Glat, 0x00030000)) == NULL
-            || (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
-            || m_pGloc.size() < 8)
-        {
-            _head = Face::Table();
-            return;
-        }
-        const byte    * p = m_pGloc;
-        int       version = be::read<uint32>(p);
-        const uint16    flags = be::read<uint16>(p);
-        _num_attrs = be::read<uint16>(p);
-        // We can accurately calculate the number of attributed glyphs by
-        //  subtracting the length of the attribids array (numAttribs long if present)
-        //  and dividing by either 2 or 4 depending on shor or lonf format
-        _long_fmt              = flags & 1;
-        int tmpnumgattrs       = (m_pGloc.size()
-                                   - (p - m_pGloc)
-                                   - sizeof(uint16)*(flags & 0x2 ? _num_attrs : 0))
-                                       / (_long_fmt ? sizeof(uint32) : sizeof(uint16)) - 1;
-
-        if (version >= 0x00020000 || tmpnumgattrs < 0 || tmpnumgattrs > 65535
-            || _num_attrs == 0 || _num_attrs > 0x3000  // is this hard limit appropriate?
-            || _num_glyphs_graphics > tmpnumgattrs
-            || m_pGlat.size() < 4)
-        {
-            _head = Face::Table();
-            return;
-        }
+        _head = Face::Table();
+        return;
+    }
+    const byte    * p = m_pGloc;
+    int       version = be::read<uint32>(p);
+    const uint16    flags = be::read<uint16>(p);
+    _num_attrs = be::read<uint16>(p);
+    // We can accurately calculate the number of attributed glyphs by
+    //  subtracting the length of the attribids array (numAttribs long if present)
+    //  and dividing by either 2 or 4 depending on shor or lonf format
+    _long_fmt              = flags & 1;
+    int tmpnumgattrs       = (m_pGloc.size()
+                               - (p - m_pGloc)
+                               - sizeof(uint16)*(flags & 0x2 ? _num_attrs : 0))
+                                   / (_long_fmt ? sizeof(uint32) : sizeof(uint16)) - 1;
+
+    if (version >= 0x00020000 || tmpnumgattrs < 0 || tmpnumgattrs > 65535
+        || _num_attrs == 0 || _num_attrs > 0x3000  // is this hard limit appropriate?
+        || _num_glyphs_graphics > tmpnumgattrs
+        || m_pGlat.size() < 4)
+    {
+        _head = Face::Table();
+        return;
+    }
 
-        _num_glyphs_attributes = static_cast<unsigned short>(tmpnumgattrs);
-        p = m_pGlat;
-        version = be::read<uint32>(p);
-        if (version >= 0x00040000 || (version >= 0x00030000 && m_pGlat.size() < 8))       // reject Glat tables that are too new
-        {
-            _head = Face::Table();
-            return;
-        }
-        else if (version >= 0x00030000)
-        {
-            unsigned int glatflags = be::read<uint32>(p);
-            _has_boxes = glatflags & 1;
-            // delete this once the compiler is fixed
-            _has_boxes = true;
-        }
+    _num_glyphs_attributes = static_cast<unsigned short>(tmpnumgattrs);
+    p = m_pGlat;
+    version = be::read<uint32>(p);
+    if (version >= 0x00040000 || (version >= 0x00030000 && m_pGlat.size() < 8))       // reject Glat tables that are too new
+    {
+        _head = Face::Table();
+        return;
+    }
+    else if (version >= 0x00030000)
+    {
+        unsigned int glatflags = be::read<uint32>(p);
+        _has_boxes = glatflags & 1;
+        // delete this once the compiler is fixed
+        _has_boxes = true;
     }
 }
 
 inline
 GlyphCache::Loader::operator bool () const throw()
diff --git a/src/gr_face.cpp b/src/gr_face.cpp
index 65a4b11..e69c7fb 100644
--- a/src/gr_face.cpp
+++ b/src/gr_face.cpp
@@ -45,12 +45,11 @@ namespace
     {
 #ifdef GRAPHITE2_TELEMETRY
         telemetry::category _misc_cat(face.tele.misc);
 #endif
         Face::Table silf(face, Tag::Silf, 0x00050000);
-        if (silf)   options &= ~gr_face_dumbRendering;
-        else if (!(options &  gr_face_dumbRendering))
+        if (!silf)
             return false;
 
         if (!face.readGlyphs(options))
             return false;
 
@@ -72,11 +71,11 @@ namespace
             }
             else
                 return true;
         }
         else
-            return options & gr_face_dumbRendering;
+            return false;
     }
 }
 
 extern "C" {
 
diff --git a/tests/featuremap/CMakeLists.txt b/tests/featuremap/CMakeLists.txt
index fc57f96..a438f66 100644
--- a/tests/featuremap/CMakeLists.txt
+++ b/tests/featuremap/CMakeLists.txt
@@ -18,10 +18,10 @@ endif (${CMAKE_SYSTEM_NAME} STREQUAL "Windows")
 if (GRAPHITE2_ASAN)
     set_target_properties(featuremaptest PROPERTIES LINK_FLAGS "-fsanitize=address")
 endif (GRAPHITE2_ASAN)
 target_link_libraries(featuremaptest graphite2 graphite2-base graphite2-segcache graphite2-base)
 
-add_test(NAME featuremaptest COMMAND $<TARGET_FILE:featuremaptest> ${testing_SOURCE_DIR}/fonts/tiny.ttf)
+add_test(NAME featuremaptest COMMAND $<TARGET_FILE:featuremaptest> ${testing_SOURCE_DIR}/fonts/small.ttf)
 set_tests_properties(featuremaptest PROPERTIES TIMEOUT 3)
 if (GRAPHITE2_ASAN)
     set_property(TEST featuremaptest APPEND PROPERTY ENVIRONMENT "ASAN_SYMBOLIZER_PATH=${ASAN_SYMBOLIZER}")
 endif (GRAPHITE2_ASAN)
diff --git a/tests/featuremap/featuremaptest.cpp b/tests/featuremap/featuremaptest.cpp
index fcd9b87..a9aa497 100644
--- a/tests/featuremap/featuremaptest.cpp
+++ b/tests/featuremap/featuremaptest.cpp
@@ -241,11 +241,11 @@ face_handle dummyFace;
 
 template <class T> void testFeatTable(const T & table, const char * testName)
 {
     FeatureMap testFeatureMap;
     dummyFace.replace_table(TtfUtil::Tag::Feat, &table, sizeof(T));
-    gr_face * face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, gr_face_dumbRendering);
+    gr_face * face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, 0);
     if (!face) throw std::runtime_error("failed to load font");
     bool readStatus = testFeatureMap.readFeats(*face);
     testAssert("readFeats", readStatus);
     fprintf(stderr, testName, NULL);
     testAssertEqual("test num features %hu,%hu\n", testFeatureMap.numFeats(), table.m_header.m_numFeat);
@@ -283,13 +283,12 @@ int main(int argc, char * argv[])
 		testFeatTable<FeatTableTestE>(testDataE, "E\n");
 
 		// test a bad settings offset stradling the end of the table
 		FeatureMap testFeatureMap;
 		dummyFace.replace_table(TtfUtil::Tag::Feat, &testBadOffset, sizeof testBadOffset);
-		face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, gr_face_dumbRendering);
-		bool readStatus = testFeatureMap.readFeats(*face);
-		testAssert("fail gracefully on bad table", !readStatus);
+		face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, 0);
+		testAssert("fail gracefully on bad table", !face);
 	}
 	catch (std::exception & e)
 	{
 		fprintf(stderr, "%s: %s\n", argv[0], e.what());
 		gr_face_destroy(face);
diff --git a/tests/vm/CMakeLists.txt b/tests/vm/CMakeLists.txt
index 4a0b823..24fbd3b 100644
--- a/tests/vm/CMakeLists.txt
+++ b/tests/vm/CMakeLists.txt
@@ -39,21 +39,21 @@ if  (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
 	if ("${CMAKE_BUILD_TYPE}" STREQUAL "Release")
 		add_definitions(-DNDEBUG -fomit-frame-pointer)
 	endif ("${CMAKE_BUILD_TYPE}" STREQUAL "Release")
 endif  (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
 
-add_test(vm-test-call-threading vm-test-call ${testing_SOURCE_DIR}/fonts/tiny.ttf 1)
+add_test(vm-test-call-threading vm-test-call ${testing_SOURCE_DIR}/fonts/small.ttf 1)
 set_tests_properties(vm-test-call-threading PROPERTIES
         PASS_REGULAR_EXPRESSION "simple program size:    14 bytes.*result of program: 42"
         FAIL_REGULAR_EXPRESSION "program terminated early;stack not empty")
 if (GRAPHITE2_ASAN)
     set_target_properties(vm-test-call PROPERTIES LINK_FLAGS "-fsanitize=address")
     set_property(TEST vm-test-call-threading APPEND PROPERTY ENVIRONMENT "ASAN_SYMBOLIZER_PATH=${ASAN_SYMBOLIZER}")
 endif (GRAPHITE2_ASAN)
 
 if  (${CMAKE_COMPILER_IS_GNUCXX})
-	add_test(vm-test-direct-threading vm-test-direct ${testing_SOURCE_DIR}/fonts/tiny.ttf 1)
+	add_test(vm-test-direct-threading vm-test-direct ${testing_SOURCE_DIR}/fonts/small.ttf 1)
 	set_tests_properties(vm-test-direct-threading PROPERTIES
 			PASS_REGULAR_EXPRESSION "simple program size:    14 bytes.*result of program: 42"
 			FAIL_REGULAR_EXPRESSION "program terminated early;stack not empty")
     if (GRAPHITE2_ASAN)
         set_target_properties(vm-test-direct PROPERTIES LINK_FLAGS "-fsanitize=address")

Vulnerable code:     /** Dumb rendering will be enabled if the graphite tables are invalid */
    Loader(const Face & face, const bool dumb_font);    //return result indicates success. Do not use if failed.
: _glyph_loader(new Loader(face, bool(face_options & gr_face_dumbRendering))),
GlyphCache::Loader::Loader(const Face & face, const bool dumb_font)
    if (!dumb_font)
        if ((m_pGlat = Face::Table(face, Tag::Glat, 0x00030000)) == NULL
            || (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
            || m_pGloc.size() < 8)
        {
            _head = Face::Table();
            return;
        }
        const byte    * p = m_pGloc;
        int       version = be::read<uint32>(p);
        const uint16    flags = be::read<uint16>(p);
        _num_attrs = be::read<uint16>(p);
        // We can accurately calculate the number of attributed glyphs by
        //  subtracting the length of the attribids array (numAttribs long if present)
        //  and dividing by either 2 or 4 depending on shor or lonf format
        _long_fmt              = flags & 1;
        int tmpnumgattrs       = (m_pGloc.size()
                                   - (p - m_pGloc)
                                   - sizeof(uint16)*(flags & 0x2 ? _num_attrs : 0))
                                       / (_long_fmt ? sizeof(uint32) : sizeof(uint16)) - 1;

        if (version >= 0x00020000 || tmpnumgattrs < 0 || tmpnumgattrs > 65535
            || _num_attrs == 0 || _num_attrs > 0x3000  // is this hard limit appropriate?
            || _num_glyphs_graphics > tmpnumgattrs
            || m_pGlat.size() < 4)
        {
            _head = Face::Table();
            return;
        }
        _num_glyphs_attributes = static_cast<unsigned short>(tmpnumgattrs);
        p = m_pGlat;
        version = be::read<uint32>(p);
        if (version >= 0x00040000 || (version >= 0x00030000 && m_pGlat.size() < 8))       // reject Glat tables that are too new
        {
            _head = Face::Table();
            return;
        }
        else if (version >= 0x00030000)
        {
            unsigned int glatflags = be::read<uint32>(p);
            _has_boxes = glatflags & 1;
            // delete this once the compiler is fixed
            _has_boxes = true;
        }
        if (silf)   options &= ~gr_face_dumbRendering;
        else if (!(options &  gr_face_dumbRendering))
            return options & gr_face_dumbRendering;
add_test(NAME featuremaptest COMMAND $<TARGET_FILE:featuremaptest> ${testing_SOURCE_DIR}/fonts/tiny.ttf)
    gr_face * face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, gr_face_dumbRendering);
		face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, gr_face_dumbRendering);
		bool readStatus = testFeatureMap.readFeats(*face);
		testAssert("fail gracefully on bad table", !readStatus);
add_test(vm-test-call-threading vm-test-call ${testing_SOURCE_DIR}/fonts/tiny.ttf 1)
	add_test(vm-test-direct-threading vm-test-direct ${testing_SOURCE_DIR}/fonts/tiny.ttf 1)
Fixed code:     /** Dumb rendering will be enabled if the graphite tables are invalid. DEPRECATED. */
    Loader(const Face & face);    //return result indicates success. Do not use if failed.
: _glyph_loader(new Loader(face)),
GlyphCache::Loader::Loader(const Face & face)
    if ((m_pGlat = Face::Table(face, Tag::Glat, 0x00030000)) == NULL
        || (m_pGloc = Face::Table(face, Tag::Gloc)) == NULL
        || m_pGloc.size() < 8)
        _head = Face::Table();
        return;
    }
    const byte    * p = m_pGloc;
    int       version = be::read<uint32>(p);
    const uint16    flags = be::read<uint16>(p);
    _num_attrs = be::read<uint16>(p);
    // We can accurately calculate the number of attributed glyphs by
    //  subtracting the length of the attribids array (numAttribs long if present)
    //  and dividing by either 2 or 4 depending on shor or lonf format
    _long_fmt              = flags & 1;
    int tmpnumgattrs       = (m_pGloc.size()
                               - (p - m_pGloc)
                               - sizeof(uint16)*(flags & 0x2 ? _num_attrs : 0))
                                   / (_long_fmt ? sizeof(uint32) : sizeof(uint16)) - 1;

    if (version >= 0x00020000 || tmpnumgattrs < 0 || tmpnumgattrs > 65535
        || _num_attrs == 0 || _num_attrs > 0x3000  // is this hard limit appropriate?
        || _num_glyphs_graphics > tmpnumgattrs
        || m_pGlat.size() < 4)
    {
        _head = Face::Table();
        return;
    }
    _num_glyphs_attributes = static_cast<unsigned short>(tmpnumgattrs);
    p = m_pGlat;
    version = be::read<uint32>(p);
    if (version >= 0x00040000 || (version >= 0x00030000 && m_pGlat.size() < 8))       // reject Glat tables that are too new
    {
        _head = Face::Table();
        return;
    }
    else if (version >= 0x00030000)
    {
        unsigned int glatflags = be::read<uint32>(p);
        _has_boxes = glatflags & 1;
        // delete this once the compiler is fixed
        _has_boxes = true;
        if (!silf)
            return false;
add_test(NAME featuremaptest COMMAND $<TARGET_FILE:featuremaptest> ${testing_SOURCE_DIR}/fonts/small.ttf)
    gr_face * face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, 0);
		face = gr_make_face_with_ops(&dummyFace, &face_handle::ops, 0);
		testAssert("fail gracefully on bad table", !face);
add_test(vm-test-call-threading vm-test-call ${testing_SOURCE_DIR}/fonts/small.ttf 1)
	add_test(vm-test-direct-threading vm-test-direct ${testing_SOURCE_DIR}/fonts/small.ttf 1)