[PATCHv3 5/5] xeniface: Bound NumberPermissions before checking the input length

[Césaire Mounah <cesaire.mounah@xxxxxxxxxx>]

IoctlStoreSetPermissions() compares InLen with
FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, Permissions[N]), whose
64-bit result is truncated to ULONG. The count was only capped to 255
later, in __ConvertPermissions(), so the length check alone could be
satisfied by a huge count.

Check the cap first, with the same STATUS_INVALID_PARAMETER as before.
The check in __ConvertPermissions() is removed and fails are updated
accordingly.

Signed-off-by: Césaire Mounah <cesaire.mounah@xxxxxxxxxx>
---
 src/xeniface/ioctl_store.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/xeniface/ioctl_store.c b/src/xeniface/ioctl_store.c
index 97bef75..b28b44d 100644
--- a/src/xeniface/ioctl_store.c
+++ b/src/xeniface/ioctl_store.c
@@ -417,6 +417,11 @@ IoctlStoreSetPermissions(
         goto fail1;
     }
 
+    status = STATUS_INVALID_PARAMETER;
+    if (In->NumberPermissions > 255)
+        goto fail2;
+
+    status = STATUS_INVALID_BUFFER_SIZE;
     if (InLen != (ULONG)FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, 
Permissions[In->NumberPermissions]))
         goto fail2;
 
@@ -427,17 +432,15 @@ IoctlStoreSetPermissions(
     }
 
     Permissions = __ConvertPermissions(In->NumberPermissions, In->Permissions);
-    if (Permissions == NULL)
-        goto fail4;
 
     status = __CaptureUserBuffer(In->Path, In->PathLength, &Path);
     if (!NT_SUCCESS(status))
-        goto fail5;
+        goto fail4;
 
     Path[In->PathLength - 1] = 0;
     status = STATUS_INVALID_PARAMETER;
     if (!__IsValidPath(Path, In->PathLength))
-        goto fail6;
+        goto fail5;
 
     Trace("> Path '%s', NumberPermissions %lu\n", Path, In->NumberPermissions);
 
@@ -457,25 +460,22 @@ IoctlStoreSetPermissions(
                           In->NumberPermissions);
 
     if (!NT_SUCCESS(status))
-        goto fail7;
+        goto fail6;
 
     __FreePermissions(Permissions);
     __FreeCapturedBuffer(Path);
     return status;
 
-fail7:
-    Error("Fail7\n");
-
 fail6:
     Error("Fail6\n");
-    __FreeCapturedBuffer(Path);
 
 fail5:
     Error("Fail5\n");
-    __FreePermissions(Permissions);
+    __FreeCapturedBuffer(Path);
 
 fail4:
     Error("Fail4\n");
+    __FreePermissions(Permissions);
 
 fail3:
     Error("Fail3\n");
-- 
2.55.0.windows.5



--
Césaire Mounah | Vates Windows Guest Tools Engineer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech