public inbox for cygwin@cygwin.com
 help / color / mirror / Atom feed
* Cygwin / MSYS2 runtime fails on Wine beause of accessing to (*ReferencedDomains)->Domains[-1]
@ 2015-04-01 10:16 Qian Hong
  2015-04-01 10:37 ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Qian Hong @ 2015-04-01 10:16 UTC (permalink / raw)
  To: cygwin

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

Hi folks,

When playing with Cygwin / MSYS2 on Wine, I found a crashing related
to LsaLookupSids.


In winsup/cygwin/uinfo.cc, we want to copy an Unicode string from
arg.full_acc->dom to dom:

1768     *wcpncpy (dom, arg.full_acc->dom->Buffer,
1769           arg.full_acc->dom->Length / sizeof (WCHAR)) = L'\0';

where arg.full_acc->dom->Buffer came from dlst->Domains[nlst[ncnt].DomainIndex]

winsup/cygwin/grp.cc:

650           fetch_acc_t full_acc =
651         {
652           .sid = sidp_buf[ncnt],
653           .name = &nlst[ncnt].Name,
654           .dom = &dlst->Domains[nlst[ncnt].DomainIndex].Name,
655           .acc_type = nlst[ncnt].Use
656         };

According to my test [1]. DomainIndex can be -1 sometimes, which seems
valid according to a similar MSDN entry [2]:

--- snip ---

Otherwise, the corresponding TranslatedNames entry MUST be updated with:

Use: SidTypeUnknown.

Name: Empty, unless LookupLevel is LsapLookupWksta. In that case, Name
MUST contain the textual representation of the corresponding SID, as
in step 2.

Flags: 0x00000000 (also see the following paragraph).

DomainIndex: -1.
--- snip ---

On windows, I never found crashing when accessing to Domains[-1]:
While it might be safe, but it might not be meaningful, here is an
example output of content of Domains[-1]:

lsa.c:431: haha names[8].DomainIndex -1
lsa.c:432: use 8 /* SidTypeUnknown */
lsa.c:433: name L"S-1-5-5-0-117053"
lsa.c:434: domain name L"\0000\0002\08c0" /* seems like garbage */
lsa.c:436: domain sid 00000020 /* not like a valid sid */

By comparing to a normal output, I strongly doubt Domains[-1] is meaningful.

lsa.c:431: names[7].DomainIndex 1
lsa.c:432: use 5
lsa.c:433: name L"This Organization"
lsa.c:434: domain name L"NT AUTHORITY"
lsa.c:436: domain sid 009808E8

Anyone know whether it is expected to access Domains[-1] in this case?

On Wine, accessing to Domains[-1] cause a crashing, I'll proposal a
patch to Wine to workaround this [as attachment], but it would be
great to see this issue also fixed at the Cygwin side if it is a
hidden bug.

Thanks for any comments and keep the great work!


[1] https://testbot.winehq.org/JobDetails.pl?Key=12577 (see attachment
for test case source code)
[2] https://msdn.microsoft.com/en-us/library/cc234496.aspx


-- 
Regards,
Qian Hong

-
http://www.winehq.org

[-- Attachment #2: 0001-advapi32-prepend-a-hidden-Domain-1-to-prevent-applicat.txt --]
[-- Type: text/plain, Size: 5142 bytes --]

From 9ade3cce58a26560920535496832e796f2fc0d90 Mon Sep 17 00:00:00 2001
From: Qian Hong <qhong@codeweavers.com>
Date: Wed, 1 Apr 2015 18:05:42 +0800
Subject: [PATCH] advapi32: prepend a hidden Domain[-1] to prevent application
 crashing when access to Domain[-1] by accident.

---
 dlls/advapi32/lsa.c       |  9 ++++++---
 dlls/advapi32/tests/lsa.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/dlls/advapi32/lsa.c b/dlls/advapi32/lsa.c
index 2a8b791..8320d58 100644
--- a/dlls/advapi32/lsa.c
+++ b/dlls/advapi32/lsa.c
@@ -488,14 +488,16 @@ NTSTATUS WINAPI LsaLookupSids(
     if (!(*Names = heap_alloc(name_fullsize))) return STATUS_NO_MEMORY;
     /* maximum count of stored domain infos is Count, allocate it like that cause really needed
        count could only be computed after sid data is retrieved */
-    domain_fullsize = sizeof(LSA_REFERENCED_DOMAIN_LIST) + sizeof(LSA_TRUST_INFORMATION)*Count;
+    domain_fullsize = sizeof(LSA_REFERENCED_DOMAIN_LIST) + sizeof(LSA_TRUST_INFORMATION)*(Count+1);
     if (!(*ReferencedDomains = heap_alloc(domain_fullsize)))
     {
         heap_free(*Names);
         return STATUS_NO_MEMORY;
     }
     (*ReferencedDomains)->Entries = 0;
-    (*ReferencedDomains)->Domains = (LSA_TRUST_INFORMATION*)((char*)*ReferencedDomains + sizeof(LSA_REFERENCED_DOMAIN_LIST));
+    (*ReferencedDomains)->Domains = (LSA_TRUST_INFORMATION*)((char*)*ReferencedDomains + sizeof(LSA_REFERENCED_DOMAIN_LIST) + sizeof(LSA_TRUST_INFORMATION));
+    (*ReferencedDomains)->Domains[-1].Name.Buffer = NULL;
+    (*ReferencedDomains)->Domains[-1].Name.Length = 0;
 
     /* Get full names data length and full length needed to store domain name and SID */
     for (i = 0; i < Count; i++)
@@ -503,6 +505,7 @@ NTSTATUS WINAPI LsaLookupSids(
         (*Names)[i].Use = SidTypeUnknown;
         (*Names)[i].DomainIndex = -1;
         (*Names)[i].Name.Buffer = NULL;
+        (*Names)[i].Name.Length = 0;
 
         memset(&(*ReferencedDomains)->Domains[i], 0, sizeof(LSA_TRUST_INFORMATION));
 
@@ -555,7 +558,7 @@ NTSTATUS WINAPI LsaLookupSids(
 
     *ReferencedDomains = heap_realloc(*ReferencedDomains, domain_fullsize);
     /* fix pointer after reallocation */
-    (*ReferencedDomains)->Domains = (LSA_TRUST_INFORMATION*)((char*)*ReferencedDomains + sizeof(LSA_REFERENCED_DOMAIN_LIST));
+    (*ReferencedDomains)->Domains = (LSA_TRUST_INFORMATION*)((char*)*ReferencedDomains + sizeof(LSA_REFERENCED_DOMAIN_LIST) + sizeof(LSA_TRUST_INFORMATION));
     domain_data = (char*)(*ReferencedDomains)->Domains + sizeof(LSA_TRUST_INFORMATION)*Count;
 
     mapped = 0;
diff --git a/dlls/advapi32/tests/lsa.c b/dlls/advapi32/tests/lsa.c
index 1a0d211..38fee45 100644
--- a/dlls/advapi32/tests/lsa.c
+++ b/dlls/advapi32/tests/lsa.c
@@ -361,7 +361,10 @@ static void test_LsaLookupSids(void)
     LSA_TRANSLATED_NAME *names;
     LSA_HANDLE policy;
     TOKEN_USER *user;
+    TOKEN_GROUPS *groups;
+    int group_id;
     NTSTATUS status;
+    PSID sids[257];
     HANDLE token;
     DWORD size;
     BOOL ret;
@@ -392,6 +395,7 @@ static void test_LsaLookupSids(void)
        ok((char*)list->Domains[0].Sid - (char*)list->Domains > 0, "%p, %p\n", list->Domains, list->Domains[0].Sid);
        ok(list->Domains[0].Name.MaximumLength > list->Domains[0].Name.Length, "got %d, %d\n", list->Domains[0].Name.MaximumLength,
            list->Domains[0].Name.Length);
+       trace("haha names[0].DomainIndex %d\n", names[0].DomainIndex);
     }
 
     pLsaFreeMemory(names);
@@ -399,6 +403,39 @@ static void test_LsaLookupSids(void)
 
     HeapFree(GetProcessHeap(), 0, user);
 
+    /* Test Enum TokenGroups */
+    ret = GetTokenInformation(token, TokenGroups, NULL, 0, &size);
+    ok(!ret, "got %d\n", ret);
+
+    groups = HeapAlloc(GetProcessHeap(), 0, size);
+    ret = GetTokenInformation(token, TokenGroups, groups, size, &size);
+    ok(ret, "got %d\n", ret);
+
+    for (group_id = 0; group_id < groups->GroupCount; group_id++)
+        sids[group_id] = groups->Groups[group_id].Sid;
+
+    status = pLsaLookupSids(policy, groups->GroupCount, sids, &list, &names);
+    ok(status == STATUS_SUCCESS, "got 0x%08x\n", status);
+
+    ok(list->Entries > 0, "got %d\n", list->Entries);
+    for (group_id = 0; group_id < groups->GroupCount; group_id++)
+    {
+        trace("entries %d\n", list->Entries);
+        if (list->Entries)
+        {
+           trace("names[%d].DomainIndex %d\n", group_id, names[group_id].DomainIndex);
+           trace("use %d\n", names[group_id].Use);
+           trace("name %s\n", wine_dbgstr_wn(names[group_id].Name.Buffer, names[group_id].Name.Length/sizeof(WCHAR)));
+           trace("domain name %s\n", wine_dbgstr_wn(list->Domains[names[group_id].DomainIndex].Name.Buffer, list->Domains[names[group_id].DomainIndex].Name.Length/sizeof(WCHAR)));
+        }
+           trace("domain sid %p\n", list->Domains[names[group_id].DomainIndex].Sid);
+    }
+
+    pLsaFreeMemory(names);
+    pLsaFreeMemory(list);
+
+    HeapFree(GetProcessHeap(), 0, groups);
+
     CloseHandle(token);
 
     status = pLsaClose(policy);
-- 
2.1.0


[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-04-01 14:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-01 10:16 Cygwin / MSYS2 runtime fails on Wine beause of accessing to (*ReferencedDomains)->Domains[-1] Qian Hong
2015-04-01 10:37 ` Corinna Vinschen
2015-04-01 10:42   ` Qian Hong
2015-04-01 11:36     ` Corinna Vinschen
2015-04-01 12:22       ` Corinna Vinschen
2015-04-01 13:31         ` Qian Hong
2015-04-01 13:45           ` Corinna Vinschen
2015-04-01 14:33             ` Qian Hong
2015-04-01 10:47   ` Qian Hong
2015-04-01 11:02     ` Corinna Vinschen
2015-04-01 13:15       ` Qian Hong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).