[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: URGENT: RE: RFR(XS): 8057564: JVM hangs at getAgentProperties after attaching to VM with lower I
From: Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date: 2014-09-22 11:28:59
Message-ID: 542007FB.9020403 () oracle ! com
[Download RAW message or body]
Looks good for me.
-Dmitry
On 2014-09-22 12:54, Sergey Gabdurakhmanov 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@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@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@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@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@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
>>>>>>>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic