commit e73a5f0dff3b879bfcad79df2e44dea382fb607e
author Sam Lantinga <slouken@libsdl.org> Mon Oct 16 14:57:42 2017 -0700
committer Sam Lantinga <slouken@libsdl.org> Mon Oct 16 14:57:42 2017 -0700
tree 174eab9a5bc5300a4125be4a0e98647674082f8b
parent fc917367af5ad5053044c593c6244943072db7aa

Fixed bug 3890 - Incomplete fix for CVE-2017-2888

Felix Geyer

http://hg.libsdl.org/SDL/rev/7e0f1498ddb5 tries to fix CVE-2017-2888.
Unfortunately compilers may optimize the second condition "(size / surface->pitch) != surface->h" away.
See https://bugzilla.redhat.com/show_bug.cgi?id=1500623#c2
I've verified that this is also the case on Debian unstable (gcc 7.2).

src/video/SDL_surface.c

diff --git a/src/video/SDL_surface.c b/src/video/SDL_surface.c
index 2c64291..38addea 100644
--- a/src/video/SDL_surface.c
+++ b/src/video/SDL_surface.c
@@ -37,6 +37,10 @@
         const void *src, int src_pitch,
         Uint32 dst_format, void *dst);
 
+/* Check to make sure we can safely check multiplication of surface w and pitch and it won't overflow size_t */
+SDL_COMPILE_TIME_ASSERT(surface_size_assumptions,
+    sizeof(int) == sizeof(Sint32) && sizeof(size_t) >= sizeof(Sint32));
+
 /* Public routines */
 
 /*
@@ -91,15 +95,16 @@
 
     /* Get the pixels */
     if (surface->w && surface->h) {
-        int size = (surface->h * surface->pitch);
-        if (size < 0 || (size / surface->pitch) != surface->h) {
+        /* Assumptions checked in surface_size_assumptions assert above */
+        Sint64 size = ((Sint64)surface->h * surface->pitch);
+        if (size < 0 || size > SDL_MAX_SINT32) {
             /* Overflow... */
             SDL_FreeSurface(surface);
             SDL_OutOfMemory();
             return NULL;
         }
 
-        surface->pixels = SDL_malloc(size);
+        surface->pixels = SDL_malloc((size_t)size);
         if (!surface->pixels) {
             SDL_FreeSurface(surface);
             SDL_OutOfMemory();