Fix GrantVmGroupAccess

Signed-off-by: John Howard <jhoward@microsoft.com>

pkg/security/grantvmgroupaccess.go

@@ -36,7 +36,7 @@ type (
 )
 
 const (
-	accessMaskDesiredPermission accessMask = 0x12019f
+	accessMaskDesiredPermission accessMask = 1 << 31 // GENERIC_READ
 
 	accessModeGrant accessMode = 1
 
@@ -55,8 +55,7 @@ const (
 	shareModeRead  shareMode = 0x1
 	shareModeWrite shareMode = 0x2
 
-	sidVmGroup                   = "S-1-5-83-1-3166535780-1122986932-343720105-43916321"
+	sidVmGroup = "S-1-5-83-0"
-	sidVmWorkerProcessCapability = "S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704"
 
 	trusteeFormIsSid trusteeForm = 0
 
@@ -64,9 +63,9 @@ const (
 )
 
 // GrantVMGroupAccess sets the DACL for a specified file or directory to
-// include Grant ACE entries for both the VM Group SID, and the VM Worker Process
+// include Grant ACE entries for the VM Group SID. This is a golang re-
-// Capability SID. This is a golang re-implementation of the same function in
+// implementation of the same function in vmcompute, just not exported in
-// vmcompute, just not exported in RS5. Which kind of sucks. Sucks a lot :/
+// RS5. Which kind of sucks. Sucks a lot :/
 func GrantVmGroupAccess(name string) error {
 	// Stat (to determine if `name` is a directory).
 	s, err := os.Stat(name)
@@ -91,56 +90,6 @@ func GrantVmGroupAccess(name string) error {
 	}
 	defer syscall.LocalFree((syscall.Handle)(unsafe.Pointer(sd)))
 
-	// Just a very large comment in case you ever want to debug this... Shows how
-	// to use a winio library function to examine the security descriptor, and
-	// how to decode obtained SD. This example if from a VHD which is attached to a
-	// B in Hyper-V. The only real thing of note is the ACE with the VM WP Capability SID
-	// The other ACEs aren't particularly interesting. Except to note that the second
-	// SID is the SID of the worker process. We don't do that in this code, rather
-	// add the VM Group SID. To debug would need to add `fmt` to imports. Will also
-	// need `//sys getSecurityDescriptorLength(sd uintptr) (len uint32) = advapi32.GetSecurityDescriptorLength`
-	// defined and regenerated.
-	//
-	// >> sdByteArray := make([]byte, getSecurityDescriptorLength(sd))
-	// >> copy(sdByteArray, (*[0xffff]byte)(unsafe.Pointer(sd))[:len(sdByteArray)])
-	// >> sddl, err := winio.SecurityDescriptorToSddl(sdByteArray)
-	// >> if err != nil {
-	// >>     return err
-	// >> }
-	// >> fmt.Println("SDDL:", sddl)
-	//
-	// Example output (pretty-printed) - first one (or two potentially) are the interesting ones:
-	// D:AI
-	//  (A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
-	//  (A;;0x12019f;;;S-1-5-83-1-3166535780-1122986932-343720105-43916321)
-	//  (A;ID;FA;;;BA)
-	//  (A;ID;FA;;;SY)
-	//  (A;ID;0x1301bf;;;AU)
-	//  (A;ID;0x1200a9;;;BU)
-	//
-	// And what ICACLS on that file shows
-	// S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(R,W)
-	// NT VIRTUAL MACHINE\BCBD8064-6BB4-42EF-A9C0-7C14211C9E02:(R,W)
-	// BUILTIN\Administrators:(I)(F)
-	// NT AUTHORITY\SYSTEM:(I)(F)
-	// NT AUTHORITY\Authenticated Users:(I)(M)
-	// BUILTIN\Users:(I)(RX)
-	//
-	// Translating D:AI(A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
-	// https://docs.microsoft.com/en-us/windows/desktop/secauthz/security-descriptor-string-format
-	//  - D:AI DACL:AutoInherited followed by ACEs, of which the first one is:
-	//  - (A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
-	// Format is ace_type;ace_flags;rights;object_guid;inherit_object_guid;account_sid;(resource_attribute)
-	// ace_type = Allowed
-	// ace_flags = 0
-	// rights = 0x12019f (exercise for reader to decode)
-	// object_guid=blank
-	// inherit_object_guid=blank
-	// account_sid=VM Worker Process Capability SID
-	//
-	// Translating (A;;0x12019f;;;S-1-5-83-1-3166535780-1122986932-343720105-43916321)
-	// is simples. It's the SID of the specific VMWP.
-
 	// Generate a new DACL which is the current DACL with the required ACEs added.
 	// Must defer LocalFree on success.
 	newDACL, err := generateDACLWithAcesAdded(name, s.IsDir(), origDACL)
@@ -178,14 +127,10 @@ func createFile(name string, isDir bool) (syscall.Handle, error) {
 // The caller is responsible for LocalFree of the returned DACL on success.
 func generateDACLWithAcesAdded(name string, isDir bool, origDACL uintptr) (uintptr, error) {
 	// Generate pointers to the SIDs based on the string SIDs
-	sid1, err := syscall.StringToSid(sidVmGroup)
+	sid, err := syscall.StringToSid(sidVmGroup)
 	if err != nil {
 		return 0, errors.Wrapf(err, "%s syscall.StringToSid %s %s", gvmga, name, sidVmGroup)
 	}
-	sid2, err := syscall.StringToSid(sidVmWorkerProcessCapability)
-	if err != nil {
-		return 0, errors.Wrapf(err, "%s syscall.StringToSid %s %s", gvmga, name, sidVmWorkerProcessCapability)
-	}
 
 	inheritance := inheritModeNoInheritance
 	if isDir {
@@ -200,23 +145,13 @@ func generateDACLWithAcesAdded(name string, isDir bool, origDACL uintptr) (uintp
 			trustee: trustee{
 				trusteeForm: trusteeFormIsSid,
 				trusteeType: trusteeTypeWellKnownGroup,
-				name:        uintptr(unsafe.Pointer(sid1)),
+				name:        uintptr(unsafe.Pointer(sid)),
-			},
-		},
-		explicitAccess{
-			accessPermissions: accessMaskDesiredPermission,
-			accessMode:        accessModeGrant,
-			inheritance:       inheritance,
-			trustee: trustee{
-				trusteeForm: trusteeFormIsSid,
-				trusteeType: trusteeTypeWellKnownGroup,
-				name:        uintptr(unsafe.Pointer(sid2)),
 			},
 		},
 	}
 
 	modifiedDACL := uintptr(0)
-	if err := setEntriesInAcl(uintptr(uint32(2)), uintptr(unsafe.Pointer(&eaArray[0])), origDACL, &modifiedDACL); err != nil {
+	if err := setEntriesInAcl(uintptr(uint32(1)), uintptr(unsafe.Pointer(&eaArray[0])), origDACL, &modifiedDACL); err != nil {
 		return 0, errors.Wrapf(err, "%s SetEntriesInAcl %s", gvmga, name)
 	}
 

pkg/security/grantvmgroupaccess_test.go

@@ -58,25 +58,32 @@ func TestGrantVmGroupAccess(t *testing.T) {
 
 	verifyicacls(t,
 		f.Name(),
-		"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(R,W)",
+		[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(R)`},
-		"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(R,W)",
 	)
 
+	// Two items here:
+	//  - One explicit read only.
+	//  - Other applies to this folder, subfolders and files
+	//      (OI): object inherit
+	//      (CI): container inherit
+	//      (IO): inherit only
+	//      (GR): generic read
+	//
+	// In properties for the directory, advanced security settings, this will
+	// show as a single line "Allow/Virtual Machines/Read/Inherited from none/This folder, subfolder and files
 	verifyicacls(t,
 		d,
-		"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(OI)(CI)(R,W)",
+		[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(R)`, `NT VIRTUAL MACHINE\\Virtual Machines:(OI)(CI)(IO)(GR)`},
-		"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(OI)(CI)(R,W)",
 	)
 
 	verifyicacls(t,
 		find.Name(),
-		"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(I)(R,W)",
+		[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(I)(R)`},
-		"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(I)(R,W)",
 	)
 
 }
 
-func verifyicacls(t *testing.T, name string, ace1 string, ace2 string) {
+func verifyicacls(t *testing.T, name string, aces []string) {
 	cmd := exec.Command("icacls", name)
 	outb, err := cmd.CombinedOutput()
 	if err != nil {
@@ -84,21 +91,15 @@ func verifyicacls(t *testing.T, name string, ace1 string, ace2 string) {
 	}
 	out := string(outb)
 
-	// Avoid () being part of match groups
+	for _, ace := range aces {
-	ace1 = strings.Replace(ace1, "(", "\\(", -1)
+		// Avoid '(' and ')' being part of match groups
-	ace1 = strings.Replace(ace1, ")", "\\)", -1)
+		ace = strings.Replace(ace, "(", "\\(", -1)
-	ace2 = strings.Replace(ace2, "(", "\\(", -1)
+		ace = strings.Replace(ace, ")", "\\)", -1)
-	ace2 = strings.Replace(ace2, ")", "\\)", -1)
 
-	rx1 := regexp.MustCompile(ace1)
+		rx := regexp.MustCompile(ace)
-	matches1 := rx1.FindAllStringIndex(out, -1)
+		matches := rx.FindAllStringIndex(out, -1)
-	if len(matches1) != 1 {
+		if len(matches) != 1 {
-		t.Fatalf("expected one match for %s got %d", ace1, len(matches1))
+			t.Fatalf("expected one match for %s got %d\n%s", ace, len(matches), out)
-	}
+		}
-
-	rx2 := regexp.MustCompile(ace1)
-	matches2 := rx2.FindAllStringIndex(out, -1)
-	if len(matches2) != 1 {
-		t.Fatalf("expected one match for %s got %d", ace2, len(matches2))
 	}
 }