Looks good! Thanks, /Staffan
On 22 sep 2014, at 10:54, Sergey Gabdurakhmanov <sergey.gabdurakhma...@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@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 >>>>>>>> >
