Merge pull request #4268 from marti4d/fix_get_raw_input_device_info

Fix buffer overflow in GetRawInputDeviceInfo proxy
sandboxie-plus · Sep 29, 2024 · fbafb1f · fbafb1f
2 parents d22f9d7 + f20fc93
commit fbafb1f
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 34 deletions.
diff --git a/Sandboxie/core/dll/guimisc.c b/Sandboxie/core/dll/guimisc.c
@@ -1330,10 +1330,13 @@ _FX LONG Gui_GetRawInputDeviceInfo_impl(
     GUI_GET_RAW_INPUT_DEVICE_INFO_REQ* req;
     GUI_GET_RAW_INPUT_DEVICE_INFO_RPL* rpl;
 
-    // Note: pcbSize seems to be in tchars not in bytes!
     ULONG lenData = 0;
-    if (pData && pcbSize)
-        lenData = (*pcbSize) * (bUnicode ? sizeof(WCHAR) : 1);
+    if (pData && pcbSize) {
+        lenData = *pcbSize;
+        if (uiCommand == RIDI_DEVICENAME && bUnicode) {
+            lenData *= sizeof(WCHAR);
+        }
+    }
 
     ULONG reqSize = sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_REQ) + lenData + 10;
     req = Dll_Alloc(reqSize);
@@ -1344,34 +1347,42 @@ _FX LONG Gui_GetRawInputDeviceInfo_impl(
     req->hDevice = (ULONG64)hDevice;
     req->uiCommand = uiCommand;
     req->unicode = bUnicode;
+    req->hasData = !!pData;
+    req->hasSize = !!pcbSize;
+
+    if (pcbSize) {
+        // *pcbSize is allowed to be uninitialized if pData == nullptr.
+        // It would be UB to access it as a UINT, so it's important that
+        // we simply copy the bytes without interpretation.
+        memcpy(&req->cbSize, pcbSize, sizeof(*pcbSize));
+    }
+
     if (lenData) {
         memcpy(reqData, pData, lenData);
-        req->hasData = TRUE;
-    } else
-        req->hasData = FALSE;
-    req->cbSize = pcbSize ? *pcbSize : -1;
+    }
 
     rpl = Gui_CallProxy(req, reqSize, sizeof(*rpl));
 
     Dll_Free(req);
 
     if (!rpl)
         return -1;
-    else {
-        ULONG error = rpl->error;
-        ULONG retval = rpl->retval;
 
-        if (pcbSize)
-            *pcbSize = rpl->cbSize;
-        if (lenData) {
-            LPVOID rplData = (BYTE*)rpl + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_RPL);
-            memcpy(pData, rplData, lenData);
-        }
+    ULONG error = rpl->error;
+    ULONG retval = rpl->retval;
 
-        Dll_Free(rpl);
-        SetLastError(error);
-        return retval;
+    if (pcbSize) {
+        memcpy(pcbSize, &rpl->cbSize, sizeof(rpl->cbSize));
+    }
+
+    if (lenData) {
+        LPVOID rplData = (BYTE*)rpl + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_RPL);
+        memcpy(pData, rplData, lenData);
     }
+
+    Dll_Free(rpl);
+    SetLastError(error);
+    return retval;
 }
 
 

diff --git a/Sandboxie/core/svc/GuiServer.cpp b/Sandboxie/core/svc/GuiServer.cpp
@@ -3532,31 +3532,34 @@ ULONG GuiServer::GetRawInputDeviceInfoSlave(SlaveArgs *args)
         return STATUS_INFO_LENGTH_MISMATCH;
 
     LPVOID reqData = req->hasData ? (BYTE*)req + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_REQ) : NULL;
-    PUINT pcbSize = NULL;
-    if (req->cbSize != -1)
-        pcbSize = &req->cbSize;
+    PUINT pcbSize = req->hasSize ? &req->cbSize : NULL;
+
+    ULONG lenData = 0;
+    if (reqData && pcbSize) {
+        lenData = *pcbSize;
+        if (req->uiCommand == RIDI_DEVICENAME && req->unicode) {
+            lenData *= sizeof(WCHAR);
+        }
+    }
 
     SetLastError(ERROR_SUCCESS);
     if (req->unicode) {
         rpl->retval = GetRawInputDeviceInfoW((HANDLE)req->hDevice, req->uiCommand, reqData, pcbSize);
-    }
-    else {
+    } else {
         rpl->retval = GetRawInputDeviceInfoA((HANDLE)req->hDevice, req->uiCommand, reqData, pcbSize);
     }
     rpl->error = GetLastError();
 
-    rpl->cbSize = req->cbSize;
-    if (pcbSize && req->hasData)
-    {
-        // Note: pcbSize seems to be in tchars not in bytes!
-        ULONG lenData = (*pcbSize) * (req->unicode ? sizeof(WCHAR) : 1);
+    if (pcbSize) {
+        // It's possible that (*pcbSize) could still be uninitialized.
+        // It would be UB to access it as a UINT.
+        memcpy(&rpl->cbSize, pcbSize, sizeof(*pcbSize));
+    }
 
-        rpl->hasData = TRUE;
+    if (lenData) {
         LPVOID rplData = (BYTE*)rpl + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_RPL);
         memcpy(rplData, reqData, lenData);
     }
-    else
-        rpl->hasData = FALSE;
 
     args->rpl_len = args->req_len;
 

diff --git a/Sandboxie/core/svc/GuiWire.h b/Sandboxie/core/svc/GuiWire.h
@@ -697,15 +697,14 @@ struct tagGUI_GET_RAW_INPUT_DEVICE_INFO_REQ
     UINT uiCommand;
     BOOLEAN unicode;
     BOOLEAN hasData;
+    BOOLEAN hasSize;
     UINT cbSize;
 };
 
 struct tagGUI_GET_RAW_INPUT_DEVICE_INFO_RPL
 {
-    ULONG status;
     ULONG error;
     ULONG retval;
-    BOOLEAN hasData;
     UINT cbSize;
 };