URGENT: RE: RFR(XS): 8057564: JVM hangs at getAgentProperties after attaching to VM with lower IntegrityLevel
Staffan Larsen
staffan.larsen at oracle.com
Mon Sep 22 09:56:47 UTC 2014
Looks good!
Thanks,
/Staffan
On 22 sep 2014, at 10:54, Sergey Gabdurakhmanov <sergey.gabdurakhmanov at oracle.com> wrote:
> Hi,
>
> I changed comment at line 265 and reduce line length.
> http://cr.openjdk.java.net/~sgabdura/8057564/webrev.04/
>
> BR,
> Sergey
>
> On 22.09.2014 12:43, Markus Grönlund wrote:
>> Looks good.
>>
>> Minor nit:
>>
>> 265: // That will allow to connect to VM with Medium Integrity Level from VM with High Integrity Level.
>>
>> It's actually the other way around - the server creates the NamedPipe (at High Integrity Level) and the clients running in Medium Integrity Level attempt to connect. Therefore the NamedPipe must be created at Medium Integrity Level (which is the default when passing an explicit SD to the CreateNamedPipe call). You could change this to:
>>
>> 265: // In order to allow Medium Integrity Level clients to open and use a NamedPipe created by an High Integrity Level process.
>>
>>
>> Thanks for fixing.
>>
>> You will also need a (R)eviewer for this as well, copying Staffan here as well.
>>
>> Thanks
>> Markus
>>
>>
>> -----Original Message-----
>> From: Sergey Gabdurakhmanov
>> Sent: den 22 september 2014 10:31
>> To: Markus Grönlund; Alexey Utkin; serviceability-dev at openjdk.java.net; SAMERSOFF,DMITRIY
>> Cc: Mattis Castegren; Christian Tornqvist
>> Subject: Re: URGENT: RE: RFR(XS): 8057564: JVM hangs at getAgentProperties after attaching to VM with lower IntegrityLevel
>>
>> Hi,
>>
>> This is new version of the patch. I hope last one.
>> http://cr.openjdk.java.net/~sgabdura/8057564/webrev.03/
>>
>> I also spent time on Sunday with that issue. I tried to find another solution, but did not succeed.
>> All my attempts of applying of modified Security Descriptor to existing Named Pipe failed with error "Access denied"
>>
>> BR,
>> Sergey
>>
>> On 22.09.2014 0:30, Markus Grönlund wrote:
>>> Hi again Sergey,
>>>
>>> I have spent some time in an attempt to figure out how we could accomplish this - I must say it has been quite frustrating.
>>>
>>> I was hoping we could only update the pSacl info with the SYSTEM_MANDATORY_LABEL_ACE field set to Medium. This is not straightforward. I have also tried to accomplish this by Windows impersonation, and updating the impersonation token - this works, but it requires quite a lot of infrastructure. In addition, if the thread is already impersonating, we need to restore the original token - and by setting the IntegrityLevel to Medium for the impersonation token one looses the SeImpersonatePrivilege which means I cannot do SetThreadToken() again for the original impersonation...messy.
>>>
>>> I then thought about using SDDL just as you have done, but only updating the Sacl info using SetSecurityInfo on the already created named pipe. This works, but it seems this triggered another issue (manifests as still hanging) - even though the integrity level is now set to Medium, the client have problem connecting (looks like this also requires easing up on the actual Discretionary ACL (just as you have done).
>>>
>>> So I ended up circling back to what you have in your second patch - if we are going to solve this proper, we need to spend quite a lot of time on it.
>>>
>>> I initially thought we should also state the Mandatory Label information in the SDDL, mainly for readability and rational purposes like:
>>>
>>> TCHAR *szSD = TEXT("D:") // Discretionary ACL
>>> TEXT("(A;OICI;GRGW;;;WD)") // Allow read/write/execute to everybody
>>> TEXT("(A;OICI;GA;;;SY)") // Allow full control to system
>>> TEXT("(A;OICI;GA;;;BA)") // Allow full control to administrators
>>> TEXT("S:") // System ACL
>>> TEXT("(ML;;NW;;;ME)"); // Mandatory Integrity Label, No-WriteUp Policy, Medium Mandatory Level
>>>
>>> However, after some thought I changed my mind on this:
>>>
>>> Only Vista and later support the Mandatory Integrity Label, and if we include it we need to check conditionally. Somehow, the default when creating a securable object with you own Security Descriptor (instead of inheriting either the primary process or impersonation token) is that you default to Medium Mandatory Level (I don’t know if this just luck, but it seems to be that way, even for High Level process tokens).
>>>
>>> So to avoid conditional checks I suggest not including this:
>>>
>>> ("S:") // System ACL
>>> TEXT("(ML;;NW;;;ME)"); // Mandatory Integrity Label, No-WriteUp Policy, Medium Mandatory Level
>>>
>>> Which is also what you already have in your patch.
>>>
>>> Maybe you could include a comment about the assumption that we expect to "get" Medium Mandatory Level by creating our own Security Descriptor? That way we can keep track of the rationale, and if MSFT changes this.
>>>
>>> I then just have one additional comment/change request:
>>>
>>> 291: LocalFree(&(sa.lpSecurityDescriptor));
>>>
>>> This should be:
>>>
>>> 291: LocalFree(sa.lpSecurityDescriptor);
>>>
>>>
>>> With that change and some comment to the Medium Mandatory Level I am ok with your suggestion.
>>>
>>> Thanks a lot for fixing this.
>>>
>>> Many thanks
>>> Markus
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Markus Grönlund
>>> Sent: den 19 september 2014 14:46
>>> To: Sergey Gabdurakhmanov
>>> Cc: Alexey Utkin; serviceability-dev at openjdk.java.net
>>> Subject: RE: URGENT: RE: RFR(XS): 8057564: JVM hangs at
>>> getAgentProperties after attaching to VM with lower IntegrityLevel
>>>
>>> Hi Sergey,
>>>
>>> This is not exactly what I had in mind - we have updated the DACL to provide some explicit security (from a NULL DACL which allows everyone everything), so that’s good. But..
>>>
>>> I was hoping we could just keep the default security for the DACL (no need to change this, pass a NULL to CreateNamedPipe() is fine (will inherit process token)).
>>>
>>> Then we should just focus on manipulating the SidStart field in the SYSTEM_MANDATORY_LABEL_ACE structure in the SACL (not the DACL).
>>>
>>> Maybe you tried this already?
>>>
>>> Cheers
>>> Markus
>>>
>>>
>>> -----Original Message-----
>>> From: Sergey Gabdurakhmanov
>>> Sent: den 19 september 2014 14:34
>>> To: Mattis Castegren; serviceability-dev at openjdk.java.net; Markus
>>> Grönlund; Staffan Larsen; Christian Törnqvist; Markus Grönlund; Alexey
>>> Utkin; Dmitry Samersoff
>>> Subject: Re: URGENT: RE: RFR(XS): 8057564: JVM hangs at
>>> getAgentProperties after attaching to VM with lower IntegrityLevel
>>>
>>> Hi,
>>>
>>> New version of the fix for review:
>>> http://cr.openjdk.java.net/~sgabdura/8057564/webrev.02/
>>>
>>> Now I add security descriptor with read/write permissions to everybody and full control to system and administrators.
>>>
>>> BR,
>>> Sergey
>>>
>>> On 17.09.2014 18:03, Mattis Castegren wrote:
>>>> Also adding Christian, who is both a reviewer AND knows windows.
>>>>
>>>> This is a very critical customer bug, and we have a hard deadline of next week.
>>>>
>>>> Kind Regards
>>>> /Mattis
>>>>
>>>> -----Original Message-----
>>>> From: Mattis Castegren
>>>> Sent: den 17 september 2014 07:08
>>>> To: Sergey Gabdurakhmanov; serviceability-dev at openjdk.java.net;
>>>> Markus Grönlund; Staffan Larsen
>>>> Cc: Mattis Castegren
>>>> Subject: RE: RFR(XS): 8057564: JVM hangs at getAgentProperties after
>>>> attaching to VM with lower IntegrityLevel
>>>>
>>>> Hi
>>>>
>>>> This is urgent for a customer case, so we would need the second review. Dmitry was ok with the fix. Sergey, you also got some additional review from someone who was not an official reviewer, right? Could you paste those comments?
>>>>
>>>> If no one on this alias feels comfortable with reviewing this fix, any ideas on someone else who can do it and who is has reviewer status? Maybe someone from another team with a lot of Windows experience?
>>>>
>>>> Kind Regards
>>>> /Mattis
>>>>
>>>> -----Original Message-----
>>>> From: Sergey Gabdurakhmanov
>>>> Sent: den 16 september 2014 12:56
>>>> To: serviceability-dev at openjdk.java.net
>>>> Subject: Re: RFR(XS): 8057564: JVM hangs at getAgentProperties after
>>>> attaching to VM with lower IntegrityLevel
>>>>
>>>> Hi,
>>>>
>>>> I need a second approval for the fix integration.
>>>> Can somebody else review the patch?
>>>>
>>>> BR,
>>>> Sergey
>>>>
>>>> On 12.09.2014 17:34, Dmitry Samersoff wrote:
>>>>> Sergey,
>>>>>
>>>>> Looks good for me.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2014-09-12 12:46, Sergey Gabdurakhmanov wrote:
>>>>>> Dmitry,
>>>>>>
>>>>>> New patch:
>>>>>> http://cr.openjdk.java.net/~sgabdura/8057564/webrev.01/
>>>>>>
>>>>>>
>>>>>> My answers:
>>>>>>
>>>>>> 1. You should not free lpSecurityDescriptor if it's null (ll.291)
>>>>>>
>>>>>> I checked MSDN
>>>>>> http://msdn.microsoft.com/en-us/library/windows/desktop/aa366730%28
>>>>>> v =vs.85%29.aspx "If the /hMem/ parameter is *NULL*, *LocalFree*
>>>>>> ignores the parameter and returns *NULL*."
>>>>>>
>>>>>> 2. It's better to re-arrange code a bit:
>>>>>>
>>>>>> if InitializeSecurityDescriptor or SetSecurityDescriptorDacl fails,
>>>>>> free lpSecurityDescriptor immediately and continue with
>>>>>> lpSecurityDescriptor == NULL
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>
>>>>>> 3. Make sure it works on all supported platforms: this code rise
>>>>>> minimal server version to windows 2003 server.
>>>>>>
>>>>>> In Windows 2003 server my fix will create a new security attributes.
>>>>>> If SetSecurityDescriptorDacl or InitializeSecurityDescriptor will
>>>>>> return false on Windows XP then my patch will pass NULL to
>>>>>> CreateNamedPipe and the code will use default security descriptor.
>>>>>>
>>>>>>
>>>>>> BR,
>>>>>> Sergey
>>>>>>
>>>>>> On 11.09.2014 16:16, Dmitry Samersoff wrote:
>>>>>>> Sergey,
>>>>>>>
>>>>>>> 1. You should not free lpSecurityDescriptor if it's null (ll.291)
>>>>>>>
>>>>>>> 2. It's better to re-arrange code a bit:
>>>>>>>
>>>>>>> if InitializeSecurityDescriptor or SetSecurityDescriptorDacl
>>>>>>> fails, free lpSecurityDescriptor immediately and continue with
>>>>>>> lpSecurityDescriptor == NULL
>>>>>>>
>>>>>>>
>>>>>>> 3. Make sure it works on all supported platforms: this code rise
>>>>>>> minimal server version to windows 2003 server.
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2014-09-11 15:49, Sergey Gabdurakhmanov wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Could I please have a review of this small fix.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~sgabdura/8057564/webrev.00/
>>>>>>>> bug: https://jbs.oracle.com/bugs/browse/JDK-8057564
>>>>>>>>
>>>>>>>> Problem description:
>>>>>>>> On Windows 7 with User Account Control (UAC) enabled, JVM hangs
>>>>>>>> at getAgentProperties or getSystemProperties after attaching from a "high"
>>>>>>>> IntegrityLevel JVM to a "medium" IntegrityLevel JVM, using Attach API:
>>>>>>>> attachedVM = com.sun.tools.attach.VirtualMachine.attach(pid);
>>>>>>>> final Properties systemProperties =
>>>>>>>> attachedVM.getSystemProperties();
>>>>>>>>
>>>>>>>> Root cause:
>>>>>>>> In WindowsVirtualMachine.attach is implemented with named pipes.
>>>>>>>> If named pipe was created with default security properties then
>>>>>>>> windows will not allow process with"medium" IntegrityLevel to be
>>>>>>>> attached to a processwith "high" IntegrityLevel.
>>>>>>>>
>>>>>>>> Solution:
>>>>>>>> Create security properties that allow requested connection.
>>>>>>>>
>>>>>>>> I'm going to push this fix into JDK9, 8 and 7.
>>>>>>>> BR,
>>>>>>>> Sergey
>>>>>>>>
>
More information about the serviceability-dev
mailing list