CVE-2019-7637: Fix in integer overflow in SDL_CalculatePitch

If a too large width is passed to SDL_SetVideoMode() the width travels to SDL_CalculatePitch() where the width (e.g. 65535) is multiplied by BytesPerPixel (e.g. 4) and the result is stored into Uint16 pitch variable. During this arithmetics an integer overflow can happen (e.g. the value is clamped as 65532). As a result SDL_Surface with a pitch smaller than width * BytesPerPixel is created, too small pixel buffer is allocated and when the SDL_Surface is processed in SDL_FillRect() a buffer overflow occurs. This can be reproduced with "./graywin -width 21312312313123213213213" command. This patch fixes is by using a very careful arithmetics in SDL_CalculatePitch(). If an overflow is detected, an error is reported back as a special 0 value. We assume that 0-width surfaces do not occur in the wild. Since SDL_CalculatePitch() is a private function, we can change the semantics. CVE-2019-7637 https://bugzilla.libsdl.org/show_bug.cgi?id=4497 Signed-off-by: Petr P?sa? <ppisar@redhat.com>
libsdl-org · Mar 17, 2019 · 40d97bf · 40d97bf
1 parent 8906afb
commit 40d97bf
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 7 deletions.
diff --git a/src/video/SDL_pixels.c b/src/video/SDL_pixels.c
@@ -286,26 +286,53 @@ void SDL_DitherColors(SDL_Color *colors, int bpp)
 	}
 }
 /* 
- * Calculate the pad-aligned scanline width of a surface
+ * Calculate the pad-aligned scanline width of a surface. Return 0 in case of
+ * an error.
  */
 Uint16 SDL_CalculatePitch(SDL_Surface *surface)
 {
-	Uint16 pitch;
+	unsigned int pitch = 0;
 
 	/* Surface should be 4-byte aligned for speed */
-	pitch = surface->w*surface->format->BytesPerPixel;
+	/* The code tries to prevent from an Uint16 overflow. */;
+	for (Uint8 byte = surface->format->BytesPerPixel; byte; byte--) {
+		pitch += (unsigned int)surface->w;
+		if (pitch < surface->w) {
+			SDL_SetError("A scanline is too wide");
+			return(0);
+		}
+	}
 	switch (surface->format->BitsPerPixel) {
 		case 1:
-			pitch = (pitch+7)/8;
+			if (pitch % 8) {
+				pitch = pitch / 8 + 1;
+			} else {
+				pitch = pitch / 8;
+			}
 			break;
 		case 4:
-			pitch = (pitch+1)/2;
+			if (pitch % 2) {
+				pitch = pitch / 2 + 1;
+			} else {
+				pitch = pitch / 2;
+			}
 			break;
 		default:
 			break;
 	}
-	pitch = (pitch + 3) & ~3;	/* 4-byte aligning */
-	return(pitch);
+	/* 4-byte aligning */
+	if (pitch & 3) {
+		if (pitch + 3 < pitch) {
+			SDL_SetError("A scanline is too wide");
+			return(0);
+		}
+		pitch = (pitch + 3) & ~3;
+	}
+	if (pitch > 0xFFFF) {
+		SDL_SetError("A scanline is too wide");
+		return(0);
+	}
+	return((Uint16)pitch);
 }
 /*
  * Match an RGB value to a particular palette index

diff --git a/src/video/gapi/SDL_gapivideo.c b/src/video/gapi/SDL_gapivideo.c
@@ -733,6 +733,9 @@ SDL_Surface *GAPI_SetVideoMode(_THIS, SDL_Surface *current,
 	video->w = gapi->w = width;
 	video->h = gapi->h = height;
 	video->pitch = SDL_CalculatePitch(video); 
+	if (!current->pitch) {
+		return(NULL);
+	}
 
 	/* Small fix for WinCE/Win32 - when activating window
 	   SDL_VideoSurface is equal to zero, so activating code

diff --git a/src/video/nanox/SDL_nxvideo.c b/src/video/nanox/SDL_nxvideo.c
@@ -378,6 +378,10 @@ SDL_Surface * NX_SetVideoMode (_THIS, SDL_Surface * current,
         current -> w = width ;
         current -> h = height ;
         current -> pitch = SDL_CalculatePitch (current) ;
+        if (!current->pitch) {
+            current = NULL;
+            goto done;
+        }
         NX_ResizeImage (this, current, flags) ;
     }
 

diff --git a/src/video/ps2gs/SDL_gsvideo.c b/src/video/ps2gs/SDL_gsvideo.c
@@ -479,6 +479,9 @@ static SDL_Surface *GS_SetVideoMode(_THIS, SDL_Surface *current,
 	current->w = width;
 	current->h = height;
 	current->pitch = SDL_CalculatePitch(current);
+	if (!current->pitch) {
+		return(NULL);
+	}
 
 	/* Memory map the DMA area for block memory transfer */
 	if ( ! mapped_mem ) {

diff --git a/src/video/ps3/SDL_ps3video.c b/src/video/ps3/SDL_ps3video.c
@@ -339,6 +339,9 @@ static SDL_Surface *PS3_SetVideoMode(_THIS, SDL_Surface * current, int width, in
 	current->w = width;
 	current->h = height;
 	current->pitch = SDL_CalculatePitch(current);
+	if (!current->pitch) {
+		return(NULL);
+	}
 
 	/* Alloc aligned mem for current->pixels */
 	s_pixels = memalign(16, current->h * current->pitch);

diff --git a/src/video/windib/SDL_dibvideo.c b/src/video/windib/SDL_dibvideo.c
@@ -675,6 +675,9 @@ SDL_Surface *DIB_SetVideoMode(_THIS, SDL_Surface *current,
 	video->w = width;
 	video->h = height;
 	video->pitch = SDL_CalculatePitch(video);
+	if (!current->pitch) {
+		return(NULL);
+	}
 
 	/* Small fix for WinCE/Win32 - when activating window
 	   SDL_VideoSurface is equal to zero, so activating code

diff --git a/src/video/windx5/SDL_dx5video.c b/src/video/windx5/SDL_dx5video.c
@@ -1127,6 +1127,9 @@ SDL_Surface *DX5_SetVideoMode(_THIS, SDL_Surface *current,
 		video->w = width;
 		video->h = height;
 		video->pitch = SDL_CalculatePitch(video);
+		if (!current->pitch) {
+			return(NULL);
+		}
 
 #ifndef NO_CHANGEDISPLAYSETTINGS
 		/* Set fullscreen mode if appropriate.

diff --git a/src/video/x11/SDL_x11video.c b/src/video/x11/SDL_x11video.c
@@ -1225,6 +1225,10 @@ SDL_Surface *X11_SetVideoMode(_THIS, SDL_Surface *current,
 		current->w = width;
 		current->h = height;
 		current->pitch = SDL_CalculatePitch(current);
+		if (!current->pitch) {
+			current = NULL;
+			goto done;
+		}
 		if (X11_ResizeImage(this, current, flags) < 0) {
 			current = NULL;
 			goto done;