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.