win: Lower integrity level of connection pipe

win: Lower integrity level of connection pipe

This is necessary to be able to connect to crashpad_handler from a
Chrome renderer.

R=jschuh@chromium.org, mark@chromium.org
BUG=chromium:546288
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/e75e8c800f3f2f9ca1b131b2a0599a2fb5ca80f9

[scottmg, 2015-11-05 19:58:22]

Description was changed from

==========
win: Lower integrity level of connection pipe

R=mark@chromium.org,jschuh@chromium.org
BUG=chromium:546288
==========

to

==========
win: Lower integrity level of connection pipe

This is necessary to be able to connect to crashpad_handler
from a Chrome renderer.

R=mark@chromium.org,jschuh@chromium.org
BUG=chromium:546288
==========

[jschuh, 2015-11-05 20:04:54]

lgtm on the windows pipe creation and label, with a doc nit.

[jschuh, 2015-11-05 20:05:13]

and here's the nit

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:53: // low integrity processes.
Should be "connected to by processes at any integrity level".

[Mark Mentovai, 2015-11-05 20:08:01]

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:70: // We only need to set the integrity
level on the first instance of the pipe.
Shouldn’t we still do this if we run crashpad_handler in persistent --pipe-name
mode without having its client start it?

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:85: PSECURITY_DESCRIPTOR sec_desc =
nullptr;
SECURITY_DESCRIPTOR*, ACL*.

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:90: PLOG(ERROR) <<
"ConvertStringSecurityDescriptorToSecurityDescriptorW";
No W on the end.

[scottmg, 2015-11-05 20:15:31]

Thanks

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:53: // low integrity processes.
On 2015/11/05 20:05:13, jschuh (very slow) wrote:
> Should be "connected to by processes at any integrity level".

Done.

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:70: // We only need to set the integrity
level on the first instance of the pipe.
On 2015/11/05 20:08:01, Mark Mentovai wrote:
> Shouldn’t we still do this if we run crashpad_handler in persistent
--pipe-name
> mode without having its client start it?

Yes, we should, but don't we still in ExceptionHandlerServer::Run, regardless of
whether we created the pipe name or someone else did? (I'm probably
misunderstanding what you're saying.)

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:85: PSECURITY_DESCRIPTOR sec_desc =
nullptr;
On 2015/11/05 20:08:01, Mark Mentovai wrote:
> SECURITY_DESCRIPTOR*, ACL*.

Done.

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:90: PLOG(ERROR) <<
"ConvertStringSecurityDescriptorToSecurityDescriptorW";
On 2015/11/05 20:08:01, Mark Mentovai wrote:
> No W on the end.

Done.

[Mark Mentovai, 2015-11-05 21:01:13]

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
util/win/exception_handler_server.cc:70: // We only need to set the integrity
level on the first instance of the pipe.
scottmg wrote:
> On 2015/11/05 20:08:01, Mark Mentovai wrote:
> > Shouldn’t we still do this if we run crashpad_handler in persistent
> --pipe-name
> > mode without having its client start it?
> 
> Yes, we should, but don't we still in ExceptionHandlerServer::Run, regardless
of
> whether we created the pipe name or someone else did? (I'm probably
> misunderstanding what you're saying.)

first_instance only controls whether FILE_FLAG_FIRST_PIPE_INSTANCE is set. It’s
set when called by CreatePipe() to establish a pipe name before Run(), but it’s
not set if SetPipeName() establishes the name but no pipe. Run() doesn’t set it.

You can change this up appropriately, but as written now, none of this new stuff
will happen in persistent mode with a well-known --pipe-name.

[scottmg, 2015-11-05 21:20:21]

On 2015/11/05 21:01:13, Mark Mentovai wrote:
>
https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
> File util/win/exception_handler_server.cc (right):
> 
>
https://codereview.chromium.org/1405093013/diff/1/util/win/exception_handler_...
> util/win/exception_handler_server.cc:70: // We only need to set the integrity
> level on the first instance of the pipe.
> scottmg wrote:
> > On 2015/11/05 20:08:01, Mark Mentovai wrote:
> > > Shouldn’t we still do this if we run crashpad_handler in persistent
> > --pipe-name
> > > mode without having its client start it?
> > 
> > Yes, we should, but don't we still in ExceptionHandlerServer::Run,
regardless
> of
> > whether we created the pipe name or someone else did? (I'm probably
> > misunderstanding what you're saying.)
> 
> first_instance only controls whether FILE_FLAG_FIRST_PIPE_INSTANCE is set.
It’s
> set when called by CreatePipe() to establish a pipe name before Run(), but
it’s
> not set if SetPipeName() establishes the name but no pipe. Run() doesn’t set
it.

Oh right, thanks for re-explaining.

> 
> You can change this up appropriately, but as written now, none of this new
stuff
> will happen in persistent mode with a well-known --pipe-name.

I set first_instance for i == 0. It needs to be done only on the first instance
otherwise subsequent sets will fail with ACCESS_DENIED as the first set
conflicts with the second.

[Mark Mentovai, 2015-11-05 21:36:15]

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:87: PACL* sacl = nullptr;
ACL**?

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:89: kSddl, SDDL_REVISION, &sec_desc,
nullptr)) {
Be explicit about the format of the string you’re providing, SDDL_REVISION_1?

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:100: sec_desc, &sacl_present, &sacl,
&sacl_defaulted)) {
sacl points to something in sec_desc, so no need to free it separately, right?

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:352:
first_pipe_instance_.reset(CreateNamedPipeInstance(pipe_name, true));
The logging needs to be rationalized a bit. Now you have
CreateNamedPipeInstance() always logging on failure, where previously, it never
did. Since this is retried in a loop and does its own logging only after a few
retries, having CreateNamedPipeInstance() log on its own on each iteration is
undesirable.

[Mark Mentovai, 2015-11-05 21:39:11]

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:385: 
Maybe this works out better from that perspective if you set the integrity level
from here if i == 0.

[scottmg, 2015-11-05 22:29:59]

Alright, around in circles here a little, but I think this is better. Please
take another look.

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:87: PACL* sacl = nullptr;
On 2015/11/05 21:36:15, Mark Mentovai wrote:
> ACL**?

Oops.

Sadly, now that I try, um, compiling, ACL* is fine, but the definitions are dumb
for SECURITY_DESCRIPTOR, so it has to be PSECURITY_DESCRIPTOR.

typedef PVOID PSECURITY_DESCRIPTOR;

typedef struct _SECURITY_DESCRIPTOR {
   BYTE  Revision;
   BYTE  Sbz1;
   SECURITY_DESCRIPTOR_CONTROL Control;
   PSID Owner;
   PSID Group;
   PACL Sacl;
   PACL Dacl;

   } SECURITY_DESCRIPTOR, *PISECURITY_DESCRIPTOR;

And the prototype is 

BOOL
WINAPI
ConvertStringSecurityDescriptorToSecurityDescriptorW(
    _In_ LPCWSTR StringSecurityDescriptor,
    _In_ DWORD StringSDRevision,
    _Outptr_ PSECURITY_DESCRIPTOR * SecurityDescriptor,
    _Out_opt_ PULONG SecurityDescriptorSize
    );

9_9

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:89: kSddl, SDDL_REVISION, &sec_desc,
nullptr)) {
On 2015/11/05 21:36:15, Mark Mentovai wrote:
> Be explicit about the format of the string you’re providing, SDDL_REVISION_1?

Done.

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:100: sec_desc, &sacl_present, &sacl,
&sacl_defaulted)) {
On 2015/11/05 21:36:15, Mark Mentovai wrote:
> sacl points to something in sec_desc, so no need to free it separately, right?

Yes, that's my understanding from the docs. Maybe Justin can confirm.

... and now removed.

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:352:
first_pipe_instance_.reset(CreateNamedPipeInstance(pipe_name, true));
On 2015/11/05 21:36:15, Mark Mentovai wrote:
> The logging needs to be rationalized a bit. Now you have
> CreateNamedPipeInstance() always logging on failure, where previously, it
never
> did. Since this is retried in a loop and does its own logging only after a few
> retries, having CreateNamedPipeInstance() log on its own on each iteration is
> undesirable.

I think we're back to previous logging behaviour now, with the addition that the
lowering function will log on failure.

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:352:
first_pipe_instance_.reset(CreateNamedPipeInstance(pipe_name, true));
On 2015/11/05 21:36:15, Mark Mentovai wrote:
> The logging needs to be rationalized a bit. Now you have
> CreateNamedPipeInstance() always logging on failure, where previously, it
never
> did. Since this is retried in a loop and does its own logging only after a few
> retries, having CreateNamedPipeInstance() log on its own on each iteration is
> undesirable.

(no lowering function now)

https://codereview.chromium.org/1405093013/diff/40001/util/win/exception_hand...
util/win/exception_handler_server.cc:385: 
On 2015/11/05 21:39:11, Mark Mentovai wrote:
> Maybe this works out better from that perspective if you set the integrity
level
> from here if i == 0.

Yeah, that seems better. Done.

...

Hmm, it would seem that SetSecurityInfo always fails if
FILE_FLAG_FIRST_PIPE_INSTANCE isn't set. So I need to set that flag all the same
on the first trip through the loop.

...

After a bit more discussion, I switched to passing in a SECURITY_ATTRIBUTES to
CreateNamedPipe instead. This still has to be done only on the first instance.

[Mark Mentovai, 2015-11-06 14:55:48]

LGTM. This structure is much better, and less code, too.

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:54: SECURITY_ATTRIBUTES security_attributes
= {0};
Can you defer the = {} until you’re sure we’re going to use it, below on line
76?

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:68: PSECURITY_DESCRIPTOR sec_desc =
nullptr;
No need to initialize.

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:69:
PCHECK(ConvertStringSecurityDescriptorToSecurityDescriptor(
Do we need to add -ladvapi32.lib or do we get that by default?

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
File util/win/scoped_local_free.h (right):

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:21: static HLOCAL InvalidValue() { return nullptr;
}
#include <windows.h>

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:24: PLOG(ERROR) << "LocalFree";
Logging stuff compiles to more than anyone ever expects. Provide a .cc and
#include base/logging.h. This can also optionally use PLOG_IF.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:28: using ScopedLocalFree =
base::ScopedGeneric<HLOCAL, LocalFreeTraits>;
#include base/scoped_generic.h.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:28: using ScopedLocalFree =
base::ScopedGeneric<HLOCAL, LocalFreeTraits>;
We also have another user of this that I added a couple of days ago in
util/win/command_line_test.cc. That one is called ScopedLocalAlloc, but it’s the
same thing. (ScopedLocalAlloc may be a better name, because we usually name the
scopers after the thing they own, not the thing they do when they’re destroyed,
but it’s your call.)

[scottmg, 2015-11-06 18:03:30]

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
File util/win/exception_handler_server.cc (right):

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:54: SECURITY_ATTRIBUTES security_attributes
= {0};
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> Can you defer the = {} until you’re sure we’re going to use it, below on line
> 76?

Done.

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:68: PSECURITY_DESCRIPTOR sec_desc =
nullptr;
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> No need to initialize.

Done.

https://codereview.chromium.org/1405093013/diff/100001/util/win/exception_han...
util/win/exception_handler_server.cc:69:
PCHECK(ConvertStringSecurityDescriptorToSecurityDescriptor(
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> Do we need to add -ladvapi32.lib or do we get that by default?

Done.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
File util/win/scoped_local_free.h (right):

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:21: static HLOCAL InvalidValue() { return nullptr;
}
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> #include <windows.h>

Done.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:24: PLOG(ERROR) << "LocalFree";
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> Logging stuff compiles to more than anyone ever expects. Provide a .cc and
> #include base/logging.h. This can also optionally use PLOG_IF.

Done.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:28: using ScopedLocalFree =
base::ScopedGeneric<HLOCAL, LocalFreeTraits>;
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> We also have another user of this that I added a couple of days ago in
> util/win/command_line_test.cc. That one is called ScopedLocalAlloc, but it’s
the
> same thing. (ScopedLocalAlloc may be a better name, because we usually name
the
> scopers after the thing they own, not the thing they do when they’re
destroyed,
> but it’s your call.)

Done, switched to ScopedLocalAlloc.

https://codereview.chromium.org/1405093013/diff/100001/util/win/scoped_local_...
util/win/scoped_local_free.h:28: using ScopedLocalFree =
base::ScopedGeneric<HLOCAL, LocalFreeTraits>;
On 2015/11/06 14:55:48, Mark Mentovai wrote:
> #include base/scoped_generic.h.

Done.

[Mark Mentovai, 2015-11-06 18:18:42]

LGTM

[scottmg, 2015-11-06 18:43:44]

Committed patchset #7 (id:120001) manually as
e75e8c800f3f2f9ca1b131b2a0599a2fb5ca80f9 (presubmit successful).

[scottmg, 2015-11-06 20:26:55]

Description was changed from

==========
win: Lower integrity level of connection pipe

This is necessary to be able to connect to crashpad_handler from a
Chrome renderer.

R=jschuh@chromium.org, mark@chromium.org
BUG=chromium:546288

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/e75e8c800f3f2f9ca1b131b...
==========

to

==========
win: Lower integrity level of connection pipe

This is necessary to be able to connect to crashpad_handler from a
Chrome renderer.

R=jschuh@chromium.org, mark@chromium.org
BUG=chromium:546288

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/e75e8c800f3f2f9ca1b131b...
==========