Mon 24 Nov 2014 10:12:23 AM UTC, original submission:
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=5f201ab5c24cb69bc96b724fd66e739928d6c5e2).
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).
Please let me know about your thoughts on this. It probably doesn't make sense to fix all those warnings (some of the function invocations are of the "(void)function()" form, indicating that ignoring the return value is intentional and safe), but just looking through them and fixing legitimate bugs should greatly improve the security posture of the code.
While I am not very familiar with FreeType as a developer, I can probably help investigating the warnings / developing patches, if need be. I'm looking forward to your comments.
|