From 0cdb323800acc8d84011f9129117bcc44bfd0d3c Mon Sep 17 00:00:00 2001
From: Augustin Cavalier <waddlesplash@gmail.com>
Date: Sat, 11 Dec 2021 21:44:59 -0500
Subject: [PATCH] app_server & Application Kit: Fix bitmap cursor handling.

Using a memcpy here is supremely dangerous, because we are writing to
an app_server buffer that we chose the length for, but using a size
that came from the client. And, indeed, because the buffer can contain
padding if the BBitmap was allocated with a non-standard BytesPerRow,
we will overflow the buffer and corrupt memory, causing app_server to crash.

So, instead, reorganize parameters a bit, and pass BytesPerRow along
with the other data needed to instantiate the bitmap, and then use
ImportBits.

Fixes an app_server crash I triggered with the experimental libX11
compatibility layer.
---
 src/kits/app/Cursor.cpp       |  7 ++++---
 src/servers/app/ServerApp.cpp | 11 ++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/kits/app/Cursor.cpp b/src/kits/app/Cursor.cpp
index b5f3cf6695..806752beb0 100644
--- a/src/kits/app/Cursor.cpp
+++ b/src/kits/app/Cursor.cpp
@@ -96,20 +96,21 @@ BCursor::BCursor(const BBitmap* bitmap, const BPoint& hotspot)
 	if (bitmap == NULL)
 		return;
 
-	int32 size = bitmap->BitsLength();
 	BRect bounds = bitmap->Bounds();
 	color_space colorspace = bitmap->ColorSpace();
 	void* bits = bitmap->Bits();
+	int32 size = bitmap->BitsLength();
 	if (bits == NULL || size <= 0)
 		return;
 
 	// Send data directly to server
 	BPrivate::AppServerLink link;
 	link.StartMessage(AS_CREATE_CURSOR_BITMAP);
-	link.Attach<int32>(size);
 	link.Attach<BRect>(bounds);
-	link.Attach<color_space>(colorspace);
 	link.Attach<BPoint>(hotspot);
+	link.Attach<color_space>(colorspace);
+	link.Attach<int32>(bitmap->BytesPerRow());
+	link.Attach<int32>(size);
 	link.Attach(bits, size);
 
 	status_t status;
diff --git a/src/servers/app/ServerApp.cpp b/src/servers/app/ServerApp.cpp
index 198dc2fb87..3e8452ef73 100644
--- a/src/servers/app/ServerApp.cpp
+++ b/src/servers/app/ServerApp.cpp
@@ -1204,16 +1204,17 @@ ServerApp::_DispatchMessage(int32 code, BPrivate::LinkReceiver& link)
 
 			status_t status = B_ERROR;
 
-			int32 size = 0;
+			int32 size = 0, bytesPerRow = 0;
 			BRect cursorRect;
 			color_space colorspace = B_RGBA32;
 			BPoint hotspot;
 			ServerCursor* cursor = NULL;
 
-			if (link.Read<int32>(&size) == B_OK
-				&& link.Read<BRect>(&cursorRect) == B_OK
-				&& link.Read<color_space>(&colorspace) == B_OK
+			if (link.Read<BRect>(&cursorRect) == B_OK
 				&& link.Read<BPoint>(&hotspot) == B_OK
+				&& link.Read<color_space>(&colorspace) == B_OK
+				&& link.Read<int32>(&bytesPerRow) == B_OK
+				&& link.Read<int32>(&size) == B_OK
 				&& size > 0) {
 
 				BStackOrHeapArray<uint8, 256> byteArray(size);
@@ -1225,7 +1226,7 @@ ServerApp::_DispatchMessage(int32 code, BPrivate::LinkReceiver& link)
 					if (cursor == NULL)
 						status = B_NO_MEMORY;
 					else
-						memcpy(cursor->Bits(), byteArray, size);
+						cursor->ImportBits(byteArray, size, bytesPerRow, colorspace);
 				}
 			}