Closed Bug 1297794 Opened 3 years ago Closed 3 years ago

Fix SetEntriesInAcl failure on non-English Windows version.

Categories

(Core :: Disability Access APIs, defect)

50 Branch
Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + fixed
firefox51 --- fixed

People

(Reporter: kasper93, Assigned: kasper93)

References

(Blocks 1 open bug)

Details

Crash Data

Attachments

(1 file, 2 obsolete files)

Hi, 

I was tracking on regression and noticed that Debug build doesn't work at all here since this change https://hg.mozilla.org/mozilla-central/rev/c2d21e0b1856. It fails on assert during initialization. SetEntriesInAcl fails on non-English Windows. Basically it is not portable to use group name, because they might be localized. For example "ADMINISTRATORS" name is incorrect on my system. It is "ADMINISTRATORZY" on Polish Windows version. Because of that SetEntriesInAcl fails.

I'm attaching a patch that fixes the issue. I've used AllocateAndInitializeSid() to obtain proper sid for given group/user.

Regards,
Kacper
Attachment #8784499 - Attachment is obsolete: true
Weird 'hg bzexport' told me that there was an error yet send first attachment. Anyway they are the same. I've added Aaron Klotz as reviewer since it was his patch that I'm fixing here. I'm not familiar with review process so please correct me if I've done something wrong.
Attachment #8784501 - Attachment is obsolete: true
Attachment #8784501 - Flags: review?(aklotz)
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review74182

::: ipc/mscom/MainThreadRuntime.cpp:136
(Diff revision 1)
> +
> +  PSID adminSID = nullptr;
> +  if (!::AllocateAndInitializeSid(&SIDAuthNT, 2, SECURITY_BUILTIN_DOMAIN_RID,
> +                                  DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0,
> +                                  &adminSID)) {
> +    ::FreeSid(systemSID);

Everything else in this code uses smart pointers. Could you please stick with that convention here and declare a deleter struct for FreeSid?

You can see how I did it with LocalFreeDeleter and the PACL that was allocated by SetEntriesInAcl().
Attachment #8784913 - Flags: review?(aklotz) → review-
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review74182

> Everything else in this code uses smart pointers. Could you please stick with that convention here and declare a deleter struct for FreeSid?
> 
> You can see how I did it with LocalFreeDeleter and the PACL that was allocated by SetEntriesInAcl().

Of course that was my initial intention to use smart pointer. But if you take a look at PSID type, it is a typedef to PVOID (void*). Unlike the PACL which is typdef to ACL*. UniquePtr has one operator which returns underlying object and dereferencing void* is obviously not correct and will not compile. So it cannot be used with void type. And since AllocateAndInitializeSid() takes PSID* which is void** I cannot use SID** instead. So yeah it is a lot clearer not to use smart pointers here to avoid totally useless casts. We don't need anything from SID type anyway.

That's say I personally think it is better not to use smart pointer for PSID, but of course I will do it if you insist on it.
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review74232

OK. I'll r+.
Attachment #8784913 - Flags: review- → review+
Actually, I think I'd prefer that you use CreateWellKnownSid instead of AllocateAndInitializeSid; that would allow us to control the memory allocation as well. Would you mind doing that instead?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kasper93)
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review74234

Sorry, changed my mind on this. I'd prefer that we use CreateWellKnownSid here.
Attachment #8784913 - Flags: review+ → review-
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review74234

Done. Please take a look.
(In reply to Aaron Klotz [:aklotz] from comment #9)
> Sorry, changed my mind on this. I'd prefer that we use CreateWellKnownSid here.

Sure, new patch is uploaded.
Flags: needinfo?(kasper93)
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review75664

r=me once these minor issues are fixed. Thanks!

::: ipc/mscom/MainThreadRuntime.cpp:124
(Diff revision 2)
>    SECURITY_DESCRIPTOR sd;
>    if (!::InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) {
>      return HRESULT_FROM_WIN32(::GetLastError());
>    }
>  
> +  BYTE systemSid[SECURITY_SID_SIZE(1)];

SECURITY_SID_SIZE is specific to the Windows 10 SDK. Since we support older SDKs, you won't be able to use this.

How about you just use SECURITY_MAX_SID_SIZE here instead?

::: ipc/mscom/MainThreadRuntime.cpp:131
(Diff revision 2)
> +  if (!::CreateWellKnownSid(WinLocalSystemSid, nullptr, systemSid,
> +                            &systemSidSize)) {
> +    return HRESULT_FROM_WIN32(::GetLastError());
> +  }
> +
> +  BYTE adminSid[SECURITY_SID_SIZE(2)];

Same here. s/SECURITY_SID_SIZE/SECURITY_MAX_SID_SIZE/.

::: ipc/mscom/MainThreadRuntime.cpp:144
(Diff revision 2)
>    EXPLICIT_ACCESS entries[] = {
>      {COM_RIGHTS_EXECUTE, GRANT_ACCESS, NO_INHERITANCE,
> -      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_USER,
> -       L"SYSTEM"}},
> +      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_SID, TRUSTEE_IS_USER,
> +       reinterpret_cast<LPWSTR>(systemSid)}},
>      {COM_RIGHTS_EXECUTE, GRANT_ACCESS, NO_INHERITANCE,
> -      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_NAME, TRUSTEE_IS_WELL_KNOWN_GROUP,
> +      {nullptr, NO_MULTIPLE_TRUSTEE, TRUSTEE_IS_SID, TRUSTEE_IS_GROUP,

I think you can still leave this as TRUSTEE_IS_WELL_KNOWN_GROUP
Attachment #8784913 - Flags: review?(aklotz) → review+
Assignee: nobody → kasper93
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

https://reviewboard.mozilla.org/r/74222/#review75664

> SECURITY_SID_SIZE is specific to the Windows 10 SDK. Since we support older SDKs, you won't be able to use this.
> 
> How about you just use SECURITY_MAX_SID_SIZE here instead?

Sure. I tried to be clever and save few bytes, but forgot to check availability of this macro.
Thanks for fixing this!
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2f2b784ab70
Fix SetEntriesInAcl failure on non-English Windows version. r=aklotz
https://hg.mozilla.org/mozilla-central/rev/e2f2b784ab70
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

Approval Request Comment
[Feature/regressing bug #]: Bug 1277075
[User impact if declined]: (Windows builds only) Microsoft COM might not initialize properly in Firefox, impacting any browser functionality that calls into COM-based APIs.
[Describe test coverage new/current, TreeHerder]: Landed green on m-c.
[Risks and why]: Low, just changes the existing code to work correctly on non-English locales.
[String/UUID change made/needed]: None
Attachment #8784913 - Flags: approval-mozilla-aurora?
I think this is currently the #3 crash on Aurora: https://crash-stats.mozilla.com/signature/?product=Firefox&signature=TpAllocTimer
Crash Signature: [@ TpAllocTimer]
Comment on attachment 8784913 [details]
Bug 1297794 - Fix SetEntriesInAcl failure on non-English Windows version.

Let's hope this fixes the top crasher on Aurora50.
Attachment #8784913 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Looking at stack trace it is definitely related. But it really shouldn't crash like that. This change will likely "fix" that because it will no longer fall into InitializeByName() path. But all I wanted to say is that the crash is weird. All 292 reports are from one system done 2016-09-07 from 21:11:31 to 21:12:59 and silent except this period of time. Am I missing something or one guy just spammed crash reporter there?
You need to log in before you can comment on or make changes to this bug.