[PATCH 1/2] sechost: Reject string SIDs with too many characters.

[Esme Povirk]

Signed-off-by: Esme Povirk 
---
 dlls/advapi32/tests/security.c | 17 +++++++++-
 dlls/sechost/security.c        | 58 ++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 3f1fffda273..9231c0243bf 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -338,6 +338,20 @@ static void test_ConvertStringSidToSid(void)
      "expected GetLastError() is ERROR_INVALID_SID, got %d\n",
      GetLastError() );

+    r = ConvertStringSidToSidA( "WDandmorecharacters", &psid );
+    ok( !r,
+     "expected failure with too many characters\n" );
+    ok( GetLastError() == ERROR_INVALID_SID,
+     "expected GetLastError() is ERROR_INVALID_SID, got %d\n",
+     GetLastError() );
+
+    r = ConvertStringSidToSidA( "WD)", &psid );
+    ok( !r,
+     "expected failure with too many characters\n" );
+    ok( GetLastError() == ERROR_INVALID_SID,
+     "expected GetLastError() is ERROR_INVALID_SID, got %d\n",
+     GetLastError() );
+
     ok(ConvertStringSidToSidA("S-1-5-21-93476-23408-4576", &psid), "ConvertStringSidToSidA failed\n");
     pisid = psid;
     ok(pisid->SubAuthorityCount == 4, "Invalid sub authority count - expected 4, got %d\n", pisid->SubAuthorityCount);
@@ -4237,7 +4251,8 @@ static void test_ConvertStringSecurityDescriptor(void)
         { "",                                SDDL_REVISION_1, TRUE },
         /* test ACE string SID */
         { "D:(D;;GA;;;S-1-0-0)",             SDDL_REVISION_1, TRUE },
-        { "D:(D;;GA;;;Nonexistent account)", SDDL_REVISION_1, FALSE, ERROR_INVALID_ACL, ERROR_INVALID_SID } /* W2K */
+        { "D:(D;;GA;;;WDANDSUCH)",           SDDL_REVISION_1, FALSE, ERROR_INVALID_ACL },
+        { "D:(D;;GA;;;Nonexistent account)", SDDL_REVISION_1, FALSE, ERROR_INVALID_ACL, ERROR_INVALID_SID }, /* W2K */
     };

for (i = 0; i < ARRAY_SIZE(cssd); i++)
diff --git a/dlls/sechost/security.c b/dlls/sechost/security.c
index b6731b1fe1c..940561cce79 100644
--- a/dlls/sechost/security.c
+++ b/dlls/sechost/security.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
  */

+#include <assert.h>
 #include <stdarg.h>
 #include "windef.h"
 #include "winbase.h"
@@ -593,18 +594,22 @@ static BOOL get_computer_sid( PSID sid )
     return TRUE;
 }

-static DWORD get_sid_size( const WCHAR *string )
+static DWORD get_sid_size( const WCHAR *string, const WCHAR **end )
 {
     if (string[0] == 'S' && string[1] == '-') /* S-R-I(-S)+ */
     {
         int token_count = 0;
-        while (*string)
+        string++;
+        while (*string == '-' || iswdigit(*string))
         {
             if (*string == '-')
                 token_count++;
             string++;
         }

+        if (end)
+            *end = string;
+
         if (token_count >= 3)
             return GetSidLengthRequired( token_count - 2 );
     }
@@ -612,6 +617,9 @@ static DWORD get_sid_size( const WCHAR *string )
     {
         unsigned int i;

+        if (end)
+            *end = string + 2;
+
         for (i = 0; i < ARRAY_SIZE(well_known_sids); i++)
         {
             if (!wcsncmp( well_known_sids[i].str, string, 2 ))
@@ -632,12 +640,12 @@ static DWORD get_sid_size( const WCHAR *string )
     return GetSidLengthRequired( 0 );
 }

-static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
+static BOOL parse_sid( const WCHAR *string, const WCHAR **end, SID *pisid, DWORD *size )
 {
     while (*string == ' ')
         string++;

-    *size = get_sid_size( string );
+    *size = get_sid_size( string, end );
     if (!pisid) /* Simply compute the size */
         return TRUE;

@@ -647,7 +655,7 @@ static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
         DWORD csubauth = ((*size - GetSidLengthRequired(0)) / sizeof(DWORD));

string += 2; /* Advance to Revision */
-        pisid->Revision = wcstoul( string, NULL, 10 );
+        pisid->Revision = wcstoul( string, (WCHAR**)&string, 10 );

if (pisid->Revision != SDDL_REVISION)
         {
@@ -665,8 +673,6 @@ static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
         pisid->SubAuthorityCount = csubauth;

/* Advance to identifier authority */
-        while (*string && *string != '-')
-            string++;
         if (*string == '-')
             string++;

@@ -675,24 +681,20 @@ static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
          */
         pisid->IdentifierAuthority.Value[0] = 0;
         pisid->IdentifierAuthority.Value[1] = 0;
-        identAuth = wcstoul( string, NULL, 10 );
+        identAuth = wcstoul( string, (WCHAR**)&string, 10 );
         pisid->IdentifierAuthority.Value[5] = identAuth & 0xff;
         pisid->IdentifierAuthority.Value[4] = (identAuth & 0xff00) >> 8;
         pisid->IdentifierAuthority.Value[3] = (identAuth & 0xff0000) >> 16;
         pisid->IdentifierAuthority.Value[2] = (identAuth & 0xff000000) >> 24;

/* Advance to first sub authority */
-        while (*string && *string != '-')
-            string++;
         if (*string == '-')
             string++;

-        while (*string)
+        while (iswdigit(*string) || *string == '-')
         {
-            pisid->SubAuthority[i++] = wcstoul( string, NULL, 10 );
+            pisid->SubAuthority[i++] = wcstoul( string, (WCHAR**)&string, 10 );

-            while (*string && *string != '-')
-                string++;
             if (*string == '-')
                 string++;
         }
@@ -703,6 +705,9 @@ static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
             return FALSE;
         }

+        if (end)
+            assert(*end == string);
+
         return TRUE;
     }
     else /* String constant format  - Only available in winxp and above */
@@ -746,6 +751,7 @@ static BOOL parse_sid( const WCHAR *string, SID *pisid, DWORD *size )
 BOOL WINAPI DECLSPEC_HOTPATCH ConvertStringSidToSidW( const WCHAR *string, PSID *sid )
 {
     DWORD size;
+    const WCHAR *string_end;

TRACE("%s, %p\n", debugstr_w(string), sid);

@@ -761,12 +767,18 @@ BOOL WINAPI DECLSPEC_HOTPATCH ConvertStringSidToSidW( const WCHAR *string, PSID
         return FALSE;
     }

-    if (!parse_sid( string, NULL, &size ))
+    if (!parse_sid( string, &string_end, NULL, &size ))
         return FALSE;

+    if (*string_end)
+    {
+        SetLastError(ERROR_INVALID_SID);
+        return FALSE;
+    }
+
     *sid = LocalAlloc( 0, size );

-    if (!parse_sid( string, *sid, &size ))
+    if (!parse_sid( string, NULL, *sid, &size ))
     {
         LocalFree( *sid );
         return FALSE;
@@ -996,11 +1008,11 @@ static BOOL parse_acl( const WCHAR *string, DWORD *flags, ACL *acl, DWORD *ret_s
         string++;

/* Parse ACE account sid */
-        if (parse_sid( string, ace ? (SID *)&ace->SidStart : NULL, &sidlen ))
-        {
-            while (*string && *string != ')')
-                string++;
-        }
+        if (!parse_sid( string, &string, ace ? (SID *)&ace->SidStart : NULL, &sidlen ))
+            goto err;
+
+        while (*string == ' ')
+            string++;

if (*string != ')')
             goto err;
@@ -1095,7 +1107,7 @@ static BOOL parse_sd( const WCHAR *string, SECURITY_DESCRIPTOR_RELATIVE *sd, DWO
             {
                 DWORD bytes;

-                if (!parse_sid( tok, (SID *)next, &bytes ))
+                if (!parse_sid( tok, NULL, (SID *)next, &bytes ))
                     goto out;

if (sd)
@@ -1113,7 +1125,7 @@ static BOOL parse_sd( const WCHAR *string, SECURITY_DESCRIPTOR_RELATIVE *sd, DWO
             {
                 DWORD bytes;

-                if (!parse_sid( tok, (SID *)next, &bytes ))
+                if (!parse_sid( tok, NULL, (SID *)next, &bytes ))
                     goto out;

if (sd)