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
From 9ade3cce58a26560920535496832e796f2fc0d90 Mon Sep 17 00:00:00 2001
From: Qian Hong <qh...@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
--
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
