[rmessiant] 50066: [ADVAPI32] - ConvertStringSidToSidW: Stop writing 1 subauthority too much. Fixes a DWORD sized buffer overflow. Should fix bug #5764. - ConvertStringSidToSidW: Don't leak an allo...

Author: rmessiant
Date: Sun Dec 19 23:19:04 2010
New Revision: 50066

URL: http://svn.reactos.org/svn/reactos?rev=50066&view=rev
Log:
[ADVAPI32]
- ConvertStringSidToSidW: Stop writing 1 subauthority too much. Fixes a DWORD sized buffer
overflow. Should fix bug #5764.
- ConvertStringSidToSidW: Don't leak an allocated SID in case of failure.
[SETUPAPI]
- SetupDiClassNameFromGuidExW: Rewrite to prevent a buffer overflow and pass additional
winetests. Should fix bug #5474.
- SetupDiClassNameFromGuidExA: Return the required buffer size in failure cases.

Modified:
    trunk/reactos/dll/win32/advapi32/sec/sid.c
    trunk/reactos/dll/win32/setupapi/devinst.c

Modified: trunk/reactos/dll/win32/advapi32/sec/sid.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/sec/sid…
==============================================================================
--- trunk/reactos/dll/win32/advapi32/sec/sid.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/advapi32/sec/sid.c [iso-8859-1] Sun Dec 19 23:19:04 2010
@@ -1982,16 +1982,14 @@
     if (*StringSid == '-')
         StringSid++;
 
-    pisid->SubAuthority[i] = atoiW(StringSid);
-
     while (*StringSid)
     {
+        pisid->SubAuthority[i++] = atoiW(StringSid);
+
         while (*StringSid && *StringSid != '-')
             StringSid++;
         if (*StringSid == '-')
             StringSid++;
-
-        pisid->SubAuthority[++i] = atoiW(StringSid);
     }
 
     if (i != pisid->SubAuthorityCount)
@@ -2002,7 +2000,10 @@
 
 lend:
     if (!ret)
+    {
+        LocalFree(pisid);
         SetLastError(ERROR_INVALID_SID);
+    }
 
     TRACE("returning %s\n", ret ? "TRUE" : "FALSE");
     return ret;

Modified: trunk/reactos/dll/win32/setupapi/devinst.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/setupapi/devinst…
==============================================================================
--- trunk/reactos/dll/win32/setupapi/devinst.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/setupapi/devinst.c [iso-8859-1] Sun Dec 19 23:19:04 2010
@@ -1103,7 +1103,7 @@
     if (MachineName)
         MachineNameW = MultiByteToUnicode(MachineName, CP_ACP);
     ret = SetupDiClassNameFromGuidExW(ClassGuid, ClassNameW, MAX_CLASS_NAME_LEN,
-     NULL, MachineNameW, Reserved);
+     RequiredSize, MachineNameW, Reserved);
     if (ret)
     {
         int len = WideCharToMultiByte(CP_ACP, 0, ClassNameW, -1, ClassName,
@@ -1113,8 +1113,6 @@
             SetLastError(ERROR_INSUFFICIENT_BUFFER);
             ret = FALSE;
         }
-        else if (RequiredSize)
-            *RequiredSize = len;
     }
     MyFree(MachineNameW);
     return ret;
@@ -1135,73 +1133,98 @@
     DWORD dwLength;
     DWORD dwRegType;
     LONG rc;
+    PWSTR Buffer;
 
     TRACE("%s %p %lu %p %s %p\n", debugstr_guid(ClassGuid), ClassName,
         ClassNameSize, RequiredSize, debugstr_w(MachineName), Reserved);
 
-    hKey = SetupDiOpenClassRegKeyExW(ClassGuid,
-                                     KEY_QUERY_VALUE,
-                                     DIOCR_INSTALLER,
-                                     MachineName,
-                                     Reserved);
+    /* Make sure there's a GUID */
+    if (ClassGuid == NULL)
+    {
+        SetLastError(ERROR_INVALID_CLASS);  /* On Vista: ERROR_INVALID_USER_BUFFER */
+        return FALSE;
+    }
+
+    /* Make sure there's a real buffer when there's a size */
+    if ((ClassNameSize > 0) && (ClassName == NULL))
+    {
+        SetLastError(ERROR_INVALID_PARAMETER);  /* On Vista: ERROR_INVALID_USER_BUFFER
*/
+        return FALSE;
+    }
+
+    /* Open the key for the GUID */
+    hKey = SetupDiOpenClassRegKeyExW(ClassGuid, KEY_QUERY_VALUE, DIOCR_INSTALLER,
MachineName, Reserved);
+
     if (hKey == INVALID_HANDLE_VALUE)
-    {
-	return FALSE;
-    }
-
+        return FALSE;
+
+    /* Retrieve the class name data */
+    dwLength = ClassNameSize * sizeof(WCHAR);
+
+    do
+    {
+        /* Allocate a buffer to retrieve the class name data */
+        Buffer = HeapAlloc(GetProcessHeap(), 0, dwLength);
+
+        if (Buffer == NULL)
+        {
+            rc = GetLastError();
+            break;
+        }
+
+        /* Query for the class name data */
+        rc = RegQueryValueExW(hKey, Class, NULL, &dwRegType, (LPBYTE) Buffer,
&dwLength);
+
+        /* Clean up the buffer if needed */
+        if (rc != ERROR_SUCCESS)
+            HeapFree(GetProcessHeap(), 0, Buffer);
+    } while (rc == ERROR_MORE_DATA);
+
+    /* Close the key */
+    RegCloseKey(hKey);
+
+    /* Make sure we got the data */
+    if (rc != ERROR_SUCCESS)
+    {
+        SetLastError(rc);
+        return FALSE;
+    }
+
+    /* Make sure the data is a string */
+    if (dwRegType != REG_SZ)
+    {
+        HeapFree(GetProcessHeap(), 0, Buffer);
+        SetLastError(ERROR_GEN_FAILURE);
+        return FALSE;
+    }
+
+    /* Determine the length of the class name */
+    dwLength /= sizeof(WCHAR);
+
+    if ((dwLength == 0) || (Buffer[dwLength - 1] != UNICODE_NULL))
+        /* Count the null-terminator */
+        dwLength++;
+
+    /* Inform the caller about the class name */
+    if ((ClassName != NULL) && (dwLength <= ClassNameSize))
+    {
+        memcpy(ClassName, Buffer, (dwLength - 1) * sizeof(WCHAR));
+        ClassName[dwLength - 1] = UNICODE_NULL;
+    }
+
+    /* Inform the caller about the required size */
     if (RequiredSize != NULL)
-    {
-	dwLength = 0;
-	if (RegQueryValueExW(hKey,
-			     Class,
-			     NULL,
-			     NULL,
-			     NULL,
-			     &dwLength))
-	{
-	    RegCloseKey(hKey);
-	    return FALSE;
-	}
-
-	*RequiredSize = dwLength / sizeof(WCHAR) + 1;
-    }
-
-    if (!ClassGuid)
-    {
-        SetLastError(ERROR_INVALID_CLASS);
-        RegCloseKey(hKey);
-        return FALSE;
-    }
-    if (!ClassName && ClassNameSize > 0)
-    {
-        SetLastError(ERROR_INVALID_PARAMETER);
-        RegCloseKey(hKey);
-        return FALSE;
-    }
-
-    dwLength = ClassNameSize * sizeof(WCHAR) - sizeof(UNICODE_NULL);
-    rc = RegQueryValueExW(hKey,
-			 Class,
-			 NULL,
-			 &dwRegType,
-			 (LPBYTE)ClassName,
-			 &dwLength);
-    if (rc != ERROR_SUCCESS)
-    {
-	SetLastError(rc);
-	RegCloseKey(hKey);
-	return FALSE;
-    }
-    if (dwRegType != REG_SZ)
-    {
-        SetLastError(ERROR_GEN_FAILURE);
-        RegCloseKey(hKey);
-        return FALSE;
-    }
-    
-    if (ClassNameSize > 1)
-        ClassName[ClassNameSize] = UNICODE_NULL;
-    RegCloseKey(hKey);
+        *RequiredSize = dwLength;
+
+    /* Clean up the buffer */
+    HeapFree(GetProcessHeap(), 0, Buffer);
+
+    /* Make sure the buffer was large enough */
+    if ((ClassName == NULL) || (dwLength > ClassNameSize))
+    {
+        SetLastError(ERROR_INSUFFICIENT_BUFFER);
+        return FALSE;
+    }
 
     return TRUE;
 }