[PATCH 1/2] sechost: Reject string SIDs with too many characters.
Signed-off-by: Esme Povirk <esme(a)codeweavers.com> --- 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) -- 2.25.1
Signed-off-by: Esme Povirk <esme(a)codeweavers.com> --- dlls/advapi32/tests/security.c | 2 ++ dlls/sechost/security.c | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 9231c0243bf..6037ddb2165 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -265,6 +265,7 @@ static void test_ConvertStringSidToSid(void) str_to_sid_tests[] = { { "WD", "S-1-1-0" }, + { "wD", "S-1-1-0" }, { "CO", "S-1-3-0" }, { "CG", "S-1-3-1" }, { "OW", "S-1-3-4", 1 }, /* Vista+ */ @@ -304,6 +305,7 @@ static void test_ConvertStringSidToSid(void) { "PA", "", 1 }, { "RS", "", 1 }, { "SA", "", 1 }, + { "s-1-12-1", "S-1-12-1" }, }; const char noSubAuthStr[] = "S-1-5"; diff --git a/dlls/sechost/security.c b/dlls/sechost/security.c index 940561cce79..b4f00f5bdf5 100644 --- a/dlls/sechost/security.c +++ b/dlls/sechost/security.c @@ -596,7 +596,7 @@ static BOOL get_computer_sid( PSID sid ) static DWORD get_sid_size( const WCHAR *string, const WCHAR **end ) { - if (string[0] == 'S' && string[1] == '-') /* S-R-I(-S)+ */ + if ((string[0] == 'S' || string[0] == 's') && string[1] == '-') /* S-R-I(-S)+ */ { int token_count = 0; string++; @@ -622,13 +622,13 @@ static DWORD get_sid_size( const WCHAR *string, const WCHAR **end ) for (i = 0; i < ARRAY_SIZE(well_known_sids); i++) { - if (!wcsncmp( well_known_sids[i].str, string, 2 )) + if (!wcsnicmp( well_known_sids[i].str, string, 2 )) return GetSidLengthRequired( well_known_sids[i].sid.SubAuthorityCount ); } for (i = 0; i < ARRAY_SIZE(well_known_rids); i++) { - if (!wcsncmp( well_known_rids[i].str, string, 2 )) + if (!wcsnicmp( well_known_rids[i].str, string, 2 )) { struct max_sid local; get_computer_sid(&local); @@ -649,7 +649,7 @@ static BOOL parse_sid( const WCHAR *string, const WCHAR **end, SID *pisid, DWORD if (!pisid) /* Simply compute the size */ return TRUE; - if (string[0] == 'S' && string[1] == '-') /* S-R-I-S-S */ + if ((string[0] == 'S' || string[0] == 's') && string[1] == '-') /* S-R-I-S-S */ { DWORD i = 0, identAuth; DWORD csubauth = ((*size - GetSidLengthRequired(0)) / sizeof(DWORD)); @@ -717,7 +717,7 @@ static BOOL parse_sid( const WCHAR *string, const WCHAR **end, SID *pisid, DWORD for (i = 0; i < ARRAY_SIZE(well_known_sids); i++) { - if (!wcsncmp(well_known_sids[i].str, string, 2)) + if (!wcsnicmp(well_known_sids[i].str, string, 2)) { DWORD j; pisid->SubAuthorityCount = well_known_sids[i].sid.SubAuthorityCount; @@ -730,7 +730,7 @@ static BOOL parse_sid( const WCHAR *string, const WCHAR **end, SID *pisid, DWORD for (i = 0; i < ARRAY_SIZE(well_known_rids); i++) { - if (!wcsncmp(well_known_rids[i].str, string, 2)) + if (!wcsnicmp(well_known_rids[i].str, string, 2)) { get_computer_sid(pisid); pisid->SubAuthority[pisid->SubAuthorityCount] = well_known_rids[i].rid; -- 2.25.1
participants (1)
-
Esme Povirk