Make event and mutexes created by SetupSingleton accessible to everyone.

Make event and mutexes created by SetupSingleton accessible to everyone.

Before this CL, a non-elevated installer could get an
ERROR_ACCESS_DENIED when calling CreateEvent/CreateMutex to obtain a
handle to an event/mutex that had already been created by an elevated
installer.

With this CL, SetupSingleton creates events and mutexes with
SECURITY_ATTRIBUTES that allow access to everyone. Calls to
CreateEvent/CreateMutex from SetupSingleton should no longer cause
an ERROR_ACCESS_DENIED, even if the event/mutex has already been
created by a installer running with a different access token.

BUG=650981

[fdoray, 2016-09-30 15:41:46]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-09-30 15:41:46]

PTAL

[grt (UTC plus 2), 2016-10-03 07:49:00]

please ask security (maybe wfh) what they think about this. it opens the
system-level installer up to a DoS attack. maybe this is an okay compromise, but
it should be discussed before being made a thing. thanks.

https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_singleton.cc (right):

https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_singleton.cc:59: // A nullptr DACL allows access to
everyone.
i don't think it's a good idea to allow any process access. authenticated users
is a bit better (SECURITY_NT_AUTHORITY + SECURITY_AUTHENTICATED_USER_RID).

[fdoray, 2016-10-03 15:23:46]

On 2016/10/03 07:49:00, grt (UTC plus 2) wrote:
> please ask security (maybe wfh) what they think about this. it opens the
> system-level installer up to a DoS attack. maybe this is an okay compromise,
but
> it should be discussed before being made a thing. thanks.
> 
>
https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
> File chrome/installer/setup/setup_singleton.cc (right):
> 
>
https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
> chrome/installer/setup/setup_singleton.cc:59: // A nullptr DACL allows access
to
> everyone.
> i don't think it's a good idea to allow any process access. authenticated
users
> is a bit better (SECURITY_NT_AUTHORITY + SECURITY_AUTHENTICATED_USER_RID).

I don't think we should change the SECURITY_ATTRIBUTES of the event and mutexes
managed by SetupSingleton.

SetupSingleton is acquired by install, update, uninstall, --rename-chrome-exe
and --delete-old-versions processes. None of these can do its job properly on a
system-level install without being elevated. Therefore, I don't think it's
useful to use permissive SECURITY_ATTRIBUTES to avoid ERROR_ACCESS_DENIED
errors. It might be better to fail during SetupSingleton acquisition with
ERROR_ACCESS_DENIED than to acquire the SetupSingleton successfully and fail
later.

[grt (UTC plus 2), 2016-10-04 06:41:36]

On 2016/10/03 15:23:46, fdoray wrote:
> On 2016/10/03 07:49:00, grt (UTC plus 2) wrote:
> > please ask security (maybe wfh) what they think about this. it opens the
> > system-level installer up to a DoS attack. maybe this is an okay compromise,
> but
> > it should be discussed before being made a thing. thanks.
> > 
> >
>
https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
> > File chrome/installer/setup/setup_singleton.cc (right):
> > 
> >
>
https://codereview.chromium.org/2385673003/diff/1/chrome/installer/setup/setu...
> > chrome/installer/setup/setup_singleton.cc:59: // A nullptr DACL allows
access
> to
> > everyone.
> > i don't think it's a good idea to allow any process access. authenticated
> users
> > is a bit better (SECURITY_NT_AUTHORITY + SECURITY_AUTHENTICATED_USER_RID).
> 
> I don't think we should change the SECURITY_ATTRIBUTES of the event and
mutexes
> managed by SetupSingleton.
> 
> SetupSingleton is acquired by install, update, uninstall, --rename-chrome-exe
> and --delete-old-versions processes. None of these can do its job properly on
a
> system-level install without being elevated. Therefore, I don't think it's
> useful to use permissive SECURITY_ATTRIBUTES to avoid ERROR_ACCESS_DENIED
> errors. It might be better to fail during SetupSingleton acquisition with
> ERROR_ACCESS_DENIED than to acquire the SetupSingleton successfully and fail
> later.

True dat. SGTM.

[fdoray, 2016-10-04 12:32:02]

Closing this CL

[fdoray, 2016-10-04 12:32:06]

Description was changed from

==========
Make event and mutexes created by SetupSingleton accessible to everyone.

Before this CL, a non-elevated installer could get an
ERROR_ACCESS_DENIED when calling CreateEvent/CreateMutex to obtain a
handle to an event/mutex that had already been created by an elevated
installer.

With this CL, SetupSingleton creates events and mutexes with
SECURITY_ATTRIBUTES that allow access to everyone. Calls to
CreateEvent/CreateMutex from SetupSingleton should no longer cause
an ERROR_ACCESS_DENIED, even if the event/mutex has already been
created by a installer running with a different access token.

BUG=650981
==========

to

==========
Make event and mutexes created by SetupSingleton accessible to everyone.

Before this CL, a non-elevated installer could get an
ERROR_ACCESS_DENIED when calling CreateEvent/CreateMutex to obtain a
handle to an event/mutex that had already been created by an elevated
installer.

With this CL, SetupSingleton creates events and mutexes with
SECURITY_ATTRIBUTES that allow access to everyone. Calls to
CreateEvent/CreateMutex from SetupSingleton should no longer cause
an ERROR_ACCESS_DENIED, even if the event/mutex has already been
created by a installer running with a different access token.

BUG=650981
==========