FreeType 2.5.3 multiple unchecked function calls returning FT_Error

[GoogleCodeExporter]

The CFF-related heap corruption vulnerability reported at 
https://savannah.nongnu.org/bugs/?43658 was caused by lack of function exit 
code validation and propagation (see the fix at 
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=5f201ab5c24cb
69bc96b724fd66e739928d6c5e2).

This might imply that the project code base might have more instances of 
function calls with unchecked return values, potentially denoting error 
conditions. Continuing execution in the caller function could then lead to 
operating on inconsistent memory state (based on the false assumption that all 
callees have succeeded) and consequently to various memory errors, often 
security related.

Fortunately, FreeType has its own type for completion status: "FT_Error" 
defined in include/fttypes.h:

---
287:  
/*************************************************************************/
288:  /*                                                                       
*/
289:  /* <Type>                                                                
*/
290:  /*    FT_Error                                                           
*/
291:  /*                                                                       
*/
292:  /* <Description>                                                         
*/
293:  /*    The FreeType error code type.  A value of~0 is always interpreted  
*/
294:  /*    as a successful operation.                                         
*/
295:  /*                                                                       
*/
296:  typedef int  FT_Error;

---

Thanks to this, it is relatively easy to use the gcc/clang compiler to point 
out all occurrences of unchecked FT_Error return values by replacing line 296 
with:

296:  #define FT_Error int __attribute__((warn_unused_result))

and adding the -Wno-attributes switch (in case of gcc):

$ CFLAGS=-Wno-attributes ./configure

When we then run "make", gcc will print out warnings of the following format:

---
freetype2/src/base/ftbitmap.c: In function 'FT_Bitmap_Embolden':
freetype2/src/base/ftbitmap.c:303:23: warning: ignoring return value of 
'FT_Bitmap_Done', declared with attribute warn_unused_result [-Wunused-result]
         FT_Bitmap_Done( library, bitmap );
                       ^
freetype2/src/base/ftglyph.c: In function 'ft_bitmap_glyph_done':
freetype2/src/base/ftglyph.c:117:19: warning: ignoring return value of 
'FT_Bitmap_Done', declared with attribute warn_unused_result [-Wunused-result]
     FT_Bitmap_Done( library, &glyph->bitmap );

---

Logs indicate that across the entire codebase (from git master as of today) 
reachable by the compiler on a GNU/Linux system, there are a total of 100 
instances of unchecked FT_Error:

$ make 2>&1 | grep "ignoring return value" | wc -l
100

A complete list of the warnings is attached. As "FT_Error" is designed 
specifically to pass error codes around, we assume it should be generally 
checked and propagated for all functions which return it. Of course not all 
unchecked function calls result in security issues or even bugs, but properly 
handling exit codes is a good practice and a way to prevent bugs in the future 
(and some of them in fact can lead to security vulnerabilities, as Savannah bug 
#43658 showed).

I am filing a bug for this to check with the FreeType maintainers if they are 
interested in cleaning up this class of bugs across the code base. The specific 
warnings have not been investigated to determine whether any of them represent 
an actual security issue, but we assume there very likely are some among the 
100 warnings.

After FT_Error, we could also apply the "warn_unused_result" attribute to other 
FreeType types; however, they are less important and less likely to exhibit 
actual bugs, because there might be legitimate reasons to ignore such return 
values.

Original issue reported on code.google.com by mjurc...@google.com on 24 Nov 2014 at 9:58

Attachments:

• unchecked_ft_error.txt

[GoogleCodeExporter]

Reported in https://savannah.nongnu.org/bugs/?43682.

Original comment by mjurc...@google.com on 24 Nov 2014 at 10:12

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Fixed in the following commits:

http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=ef439fd209563
3bfef876bbf56434cc3b8fb0fb4
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=b24e8ba28a971
1e72975c11a37f1269254e5ac3c
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=6689a009ced74
42c121df1224b3c529e81dc5017
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=3e86711ebf6ef
dea405f8f35bc34baf737b744df

The response from Werner Lemberg, a FreeType maintainer, was as follows (from 
bug):

---
It was an excellent idea to check this, thanks!  I've now reviewed the source
code, which resulted in a series of commits, please have a look.

My philosophy is to add casts to `void' only for functions that can return
non-trivial errors.  `Trivial' errors are caused by invalid arguments to API
functions, and they don't get a cast.

As far as I can see, none of the added error checkings are critical, but it's
good to have them fixed, too.
---

Original comment by mjurc...@google.com on 25 Nov 2014 at 11:29

• Changed state: Fixed
• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

All fixed by upstream:

FreeType 2.5.5

2014-12-30
FreeType 2.5.5 has been released. This is a minor bug fix release: All users of 
PCF fonts should update, since version 2.5.4 introduced a bug that prevented 
reading of such font files if not compressed.

FreeType 2.5.4

2014-12-06
FreeType 2.5.4 has been released. All users should upgrade due to another fix 
for vulnerability CVE-2014-2240 in the CFF driver. The library also contains a 
new round of patches for better protection against malformed fonts.

The main new feature, which is also one of the targets mentioned in the pledgie 
roadmap below, is auto-hinting support for Devanagari and Telugu, two widely 
used Indic scripts. A more detailed description of the remaining changes and 
fixes can be found here.

Original comment by cev...@google.com on 26 Jan 2015 at 5:27

• Added labels: ****
• Removed labels: Restrict-View-Commit