[cairo-commit] 3 commits - src/cairo-cff-subset.c

[GitLab Mirror]

 src/cairo-cff-subset.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

New commits:
commit c56c3023bb67775ee6e831f7fc25103e94c09962
Merge: aeafbf554 cc656934d
Author: Uli Schlachter <psychon at znc.in>
Date:   Sat Dec 31 13:20:07 2022 +0000

    Merge branch 'oob-cff-subset' into 'master'
    
    Fix out-of-bounds access in cff subset
    
    See merge request cairo/cairo!382

commit cc656934da36943cc780e399d3853b213988be4b
Author: Uli Schlachter <psychon at znc.in>
Date:   Sat Dec 31 13:41:32 2022 +0100

    Fix a possible out-of-bounds read
    
    While working on the previous commit, I noticed that nothing makes sure
    that the entry points within the font data. Thus, this could easily
    cause out-of-bounds reads.
    
    This commit adds a suitable length check for this.
    
    Signed-off-by: Uli Schlachter <psychon at znc.in>

diff --git a/src/cairo-cff-subset.c b/src/cairo-cff-subset.c
index 38b6824b6..dd626e85c 100644
--- a/src/cairo-cff-subset.c
+++ b/src/cairo-cff-subset.c
@@ -430,7 +430,7 @@ cff_index_read (cairo_array_t *index, unsigned char **ptr, unsigned char *end_pt
         for (i = 0; i < count; i++) {
             end = decode_index_offset (p, offset_size);
             p += offset_size;
-            if (p > end_ptr || end < start)
+            if (p > end_ptr || end < start || data + end > end_ptr)
                 return CAIRO_INT_STATUS_UNSUPPORTED;
             element.length = end - start;
             element.is_copy = FALSE;
commit 52760fc90ea0472005708b8903b66dd00799b3eb
Author: Uli Schlachter <psychon at znc.in>
Date:   Sat Dec 31 13:36:42 2022 +0100

    Fix out-of-bounds access in cff subset
    
    I was looking at [1]. While trying to reproduce the problem that is
    described there, valgrind reported:
    
     Argument 'size' of function malloc has a fishy (possibly negative) value: -8
        at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
        by 0x4B20E92: cairo_cff_font_read_name (cairo-cff-subset.c:895)
        by 0x4B221AD: cairo_cff_font_read_font (cairo-cff-subset.c:1351)
        by 0x4B24EF2: cairo_cff_font_generate (cairo-cff-subset.c:2587)
        by 0x4B25EA3: _cairo_cff_subset_init (cairo-cff-subset.c:2979)
    
    This commit is about fixing the above.
    
    The function decode_index_offset() returns an unsigned long. This value
    was cast to an "int" in cff_index_read(), leading to a possibility for
    over/underflow. Also, nothing checked that an entry in the index table
    had a non-zero length, leading to an entry with length -8 as reported by
    valgrind.
    
    Fix this by using "unsigned long" for the local variables and checking
    the length to be non-negative.
    
    With the above fixed, the original test case started crashing.
    Apparently, cairo_cff_font_read_name() does not expect nor handle
    failures from cff_index_read(). Thus, a check for this case was added to
    make the new crash go away.
    
    [1]: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51324
    
    Signed-off-by: Uli Schlachter <psychon at znc.in>

diff --git a/src/cairo-cff-subset.c b/src/cairo-cff-subset.c
index 4bf22e2b7..38b6824b6 100644
--- a/src/cairo-cff-subset.c
+++ b/src/cairo-cff-subset.c
@@ -412,8 +412,8 @@ cff_index_read (cairo_array_t *index, unsigned char **ptr, unsigned char *end_pt
     cff_index_element_t element;
     unsigned char *data, *p;
     cairo_status_t status;
-    int offset_size, count, start, i;
-    int end = 0;
+    int offset_size, count, i;
+    unsigned long start, end = 0;
 
     p = *ptr;
     if (p + 2 > end_ptr)
@@ -430,7 +430,7 @@ cff_index_read (cairo_array_t *index, unsigned char **ptr, unsigned char *end_pt
         for (i = 0; i < count; i++) {
             end = decode_index_offset (p, offset_size);
             p += offset_size;
-            if (p > end_ptr)
+            if (p > end_ptr || end < start)
                 return CAIRO_INT_STATUS_UNSUPPORTED;
             element.length = end - start;
             element.is_copy = FALSE;
@@ -875,7 +875,7 @@ cairo_cff_font_read_name (cairo_cff_font_t *font)
 
     cff_index_init (&index);
     status = cff_index_read (&index, &font->current_ptr, font->data_end);
-    if (!font->is_opentype) {
+    if (status == CAIRO_INT_STATUS_SUCCESS && !font->is_opentype) {
         element = _cairo_array_index (&index, 0);
 	p = element->data;
 	len = element->length;