[PATCH] server: Allow skipping debug handle retrieval in get_process_debug_info.

[Jinoh Kang]

On 12/6/21 22:51, Paul Gofman wrote:
> On 12/4/21 19:05, Jinoh Kang wrote:
>> Today, Wine uses NtQueryInformationProcess/ProcessDebugPort to detect
>> whether the current process is being debugged.  If it is, the process
>> issues a breakpoint to yield control to the debugger.
>>
>> Some debuggers (e.g. latest CDB) appear to create debug handles with
>> restricted DACL, which causes querying debug port to fail with
>> STATUS_ACCESS_DENIED.  This results in the debuggee erroneously
>> skipping the initial breakpoint.
>>
>> Fix this by making retrieval of debug port object handle optional.
>> Also, skip debug port object handle retrieval if serving requests that
>> don't need it (i.e. ProcessDebugPort and ProcessDebugFlags).
> 
> So it looks like ProcessDebugPort, ProcessDebugFlags are currently leaking debug object handle? Probably worth fixing as a separate patch.

No, the current code is correct with respect to resource management.
Thus:

> This also eliminates the extra round trip to the server for closing the
> unneeded debug port object handle.

> 
> Then, I suspect that maybe always querying the debug object handle with DEBUG_ALL_ACCESS is what is not quite right at the first place? Shouldn't it maybe return the debug object with available permissions in ProcessDebugObjectHandle?

Okay, looks like MAXIMUM_ALLOWED would be a better fit.

> That probably deserves a test,

> and if that is the case returning the debug object handle with the available permissions will probably fix both the concerned issue and ProcessDebugObjectHandle implementation.

Partly it should, but not completely.  ProcessDebugPort and ProcessDebugFlags
shall succeed even if the caller doesn't have access to the debug port object at
all.

In fact, it seems to be a known "anti-anti-debugging" trick to create
the debug port with highly restricted DACL, which makes querying for
ProcessDebugObjectHandle fail with STATUS_ACCESS_DENIED; however, this does not
apply to ProcessDebugPort and ProcessDebugFlags.

In order to replicate this behaviour, we need both fixes:

1. ProcessDebugObjectHandle shall open the object with MAXIMUM_ALLOWED
   (will probably fix the CDB case in particular)

2. ProcessDebugPort and ProcessDebugFlags shall not open the object at all
   (will replicate Windows behaviour and also avoid another round trip for
    NtClose)

> 
> Also, we do not include automatically generated changes (make_requests) in the patches, they are generated during upstream commit.
> 

Thanks!

-- 
Sincerely,
Jinoh Kang