Skip to content

Commit c599b53

Browse files
author
John Howard
authored
Merge pull request #117 from Microsoft/jjh/gvmga-take2
Fix GrantVmGroupAccess
2 parents cf4178e + 8df5952 commit c599b53

2 files changed

Lines changed: 31 additions & 95 deletions

File tree

‎pkg/security/grantvmgroupaccess.go‎

Lines changed: 8 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ type (
3636
)
3737

3838
const (
39-
accessMaskDesiredPermission accessMask = 0x12019f
39+
accessMaskDesiredPermission accessMask = 1 << 31 // GENERIC_READ
4040

4141
accessModeGrant accessMode = 1
4242

@@ -55,18 +55,17 @@ const (
5555
shareModeRead shareMode = 0x1
5656
shareModeWrite shareMode = 0x2
5757

58-
sidVmGroup = "S-1-5-83-1-3166535780-1122986932-343720105-43916321"
59-
sidVmWorkerProcessCapability = "S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704"
58+
sidVmGroup = "S-1-5-83-0"
6059

6160
trusteeFormIsSid trusteeForm = 0
6261

6362
trusteeTypeWellKnownGroup trusteeType = 5
6463
)
6564

6665
// GrantVMGroupAccess sets the DACL for a specified file or directory to
67-
// include Grant ACE entries for both the VM Group SID, and the VM Worker Process
68-
// Capability SID. This is a golang re-implementation of the same function in
69-
// vmcompute, just not exported in RS5. Which kind of sucks. Sucks a lot :/
66+
// include Grant ACE entries for the VM Group SID. This is a golang re-
67+
// implementation of the same function in vmcompute, just not exported in
68+
// RS5. Which kind of sucks. Sucks a lot :/
7069
func GrantVmGroupAccess(name string) error {
7170
// Stat (to determine if `name` is a directory).
7271
s, err := os.Stat(name)
@@ -91,56 +90,6 @@ func GrantVmGroupAccess(name string) error {
9190
}
9291
defer syscall.LocalFree((syscall.Handle)(unsafe.Pointer(sd)))
9392

94-
// Just a very large comment in case you ever want to debug this... Shows how
95-
// to use a winio library function to examine the security descriptor, and
96-
// how to decode obtained SD. This example if from a VHD which is attached to a
97-
// B in Hyper-V. The only real thing of note is the ACE with the VM WP Capability SID
98-
// The other ACEs aren't particularly interesting. Except to note that the second
99-
// SID is the SID of the worker process. We don't do that in this code, rather
100-
// add the VM Group SID. To debug would need to add `fmt` to imports. Will also
101-
// need `//sys getSecurityDescriptorLength(sd uintptr) (len uint32) = advapi32.GetSecurityDescriptorLength`
102-
// defined and regenerated.
103-
//
104-
// >> sdByteArray := make([]byte, getSecurityDescriptorLength(sd))
105-
// >> copy(sdByteArray, (*[0xffff]byte)(unsafe.Pointer(sd))[:len(sdByteArray)])
106-
// >> sddl, err := winio.SecurityDescriptorToSddl(sdByteArray)
107-
// >> if err != nil {
108-
// >> return err
109-
// >> }
110-
// >> fmt.Println("SDDL:", sddl)
111-
//
112-
// Example output (pretty-printed) - first one (or two potentially) are the interesting ones:
113-
// D:AI
114-
// (A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
115-
// (A;;0x12019f;;;S-1-5-83-1-3166535780-1122986932-343720105-43916321)
116-
// (A;ID;FA;;;BA)
117-
// (A;ID;FA;;;SY)
118-
// (A;ID;0x1301bf;;;AU)
119-
// (A;ID;0x1200a9;;;BU)
120-
//
121-
// And what ICACLS on that file shows
122-
// S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(R,W)
123-
// NT VIRTUAL MACHINE\BCBD8064-6BB4-42EF-A9C0-7C14211C9E02:(R,W)
124-
// BUILTIN\Administrators:(I)(F)
125-
// NT AUTHORITY\SYSTEM:(I)(F)
126-
// NT AUTHORITY\Authenticated Users:(I)(M)
127-
// BUILTIN\Users:(I)(RX)
128-
//
129-
// Translating D:AI(A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
130-
// https://docs.microsoft.com/en-us/windows/desktop/secauthz/security-descriptor-string-format
131-
// - D:AI DACL:AutoInherited followed by ACEs, of which the first one is:
132-
// - (A;;0x12019f;;;S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704)
133-
// Format is ace_type;ace_flags;rights;object_guid;inherit_object_guid;account_sid;(resource_attribute)
134-
// ace_type = Allowed
135-
// ace_flags = 0
136-
// rights = 0x12019f (exercise for reader to decode)
137-
// object_guid=blank
138-
// inherit_object_guid=blank
139-
// account_sid=VM Worker Process Capability SID
140-
//
141-
// Translating (A;;0x12019f;;;S-1-5-83-1-3166535780-1122986932-343720105-43916321)
142-
// is simples. It's the SID of the specific VMWP.
143-
14493
// Generate a new DACL which is the current DACL with the required ACEs added.
14594
// Must defer LocalFree on success.
14695
newDACL, err := generateDACLWithAcesAdded(name, s.IsDir(), origDACL)
@@ -178,14 +127,10 @@ func createFile(name string, isDir bool) (syscall.Handle, error) {
178127
// The caller is responsible for LocalFree of the returned DACL on success.
179128
func generateDACLWithAcesAdded(name string, isDir bool, origDACL uintptr) (uintptr, error) {
180129
// Generate pointers to the SIDs based on the string SIDs
181-
sid1, err := syscall.StringToSid(sidVmGroup)
130+
sid, err := syscall.StringToSid(sidVmGroup)
182131
if err != nil {
183132
return 0, errors.Wrapf(err, "%s syscall.StringToSid %s %s", gvmga, name, sidVmGroup)
184133
}
185-
sid2, err := syscall.StringToSid(sidVmWorkerProcessCapability)
186-
if err != nil {
187-
return 0, errors.Wrapf(err, "%s syscall.StringToSid %s %s", gvmga, name, sidVmWorkerProcessCapability)
188-
}
189134

190135
inheritance := inheritModeNoInheritance
191136
if isDir {
@@ -200,23 +145,13 @@ func generateDACLWithAcesAdded(name string, isDir bool, origDACL uintptr) (uintp
200145
trustee: trustee{
201146
trusteeForm: trusteeFormIsSid,
202147
trusteeType: trusteeTypeWellKnownGroup,
203-
name: uintptr(unsafe.Pointer(sid1)),
204-
},
205-
},
206-
explicitAccess{
207-
accessPermissions: accessMaskDesiredPermission,
208-
accessMode: accessModeGrant,
209-
inheritance: inheritance,
210-
trustee: trustee{
211-
trusteeForm: trusteeFormIsSid,
212-
trusteeType: trusteeTypeWellKnownGroup,
213-
name: uintptr(unsafe.Pointer(sid2)),
148+
name: uintptr(unsafe.Pointer(sid)),
214149
},
215150
},
216151
}
217152

218153
modifiedDACL := uintptr(0)
219-
if err := setEntriesInAcl(uintptr(uint32(2)), uintptr(unsafe.Pointer(&eaArray[0])), origDACL, &modifiedDACL); err != nil {
154+
if err := setEntriesInAcl(uintptr(uint32(1)), uintptr(unsafe.Pointer(&eaArray[0])), origDACL, &modifiedDACL); err != nil {
220155
return 0, errors.Wrapf(err, "%s SetEntriesInAcl %s", gvmga, name)
221156
}
222157

‎pkg/security/grantvmgroupaccess_test.go‎

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -58,47 +58,48 @@ func TestGrantVmGroupAccess(t *testing.T) {
5858

5959
verifyicacls(t,
6060
f.Name(),
61-
"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(R,W)",
62-
"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(R,W)",
61+
[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(R)`},
6362
)
6463

64+
// Two items here:
65+
// - One explicit read only.
66+
// - Other applies to this folder, subfolders and files
67+
// (OI): object inherit
68+
// (CI): container inherit
69+
// (IO): inherit only
70+
// (GR): generic read
71+
//
72+
// In properties for the directory, advanced security settings, this will
73+
// show as a single line "Allow/Virtual Machines/Read/Inherited from none/This folder, subfolder and files
6574
verifyicacls(t,
6675
d,
67-
"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(OI)(CI)(R,W)",
68-
"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(OI)(CI)(R,W)",
76+
[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(R)`, `NT VIRTUAL MACHINE\\Virtual Machines:(OI)(CI)(IO)(GR)`},
6977
)
7078

7179
verifyicacls(t,
7280
find.Name(),
73-
"S-1-15-3-1024-2268835264-3721307629-241982045-173645152-1490879176-104643441-2915960892-1612460704:(I)(R,W)",
74-
"S-1-5-83-1-3166535780-1122986932-343720105-43916321:(I)(R,W)",
81+
[]string{`NT VIRTUAL MACHINE\\Virtual Machines:(I)(R)`},
7582
)
7683

7784
}
7885

79-
func verifyicacls(t *testing.T, name string, ace1 string, ace2 string) {
86+
func verifyicacls(t *testing.T, name string, aces []string) {
8087
cmd := exec.Command("icacls", name)
8188
outb, err := cmd.CombinedOutput()
8289
if err != nil {
8390
t.Fatal(err)
8491
}
8592
out := string(outb)
8693

87-
// Avoid () being part of match groups
88-
ace1 = strings.Replace(ace1, "(", "\\(", -1)
89-
ace1 = strings.Replace(ace1, ")", "\\)", -1)
90-
ace2 = strings.Replace(ace2, "(", "\\(", -1)
91-
ace2 = strings.Replace(ace2, ")", "\\)", -1)
94+
for _, ace := range aces {
95+
// Avoid '(' and ')' being part of match groups
96+
ace = strings.Replace(ace, "(", "\\(", -1)
97+
ace = strings.Replace(ace, ")", "\\)", -1)
9298

93-
rx1 := regexp.MustCompile(ace1)
94-
matches1 := rx1.FindAllStringIndex(out, -1)
95-
if len(matches1) != 1 {
96-
t.Fatalf("expected one match for %s got %d", ace1, len(matches1))
97-
}
98-
99-
rx2 := regexp.MustCompile(ace1)
100-
matches2 := rx2.FindAllStringIndex(out, -1)
101-
if len(matches2) != 1 {
102-
t.Fatalf("expected one match for %s got %d", ace2, len(matches2))
99+
rx := regexp.MustCompile(ace)
100+
matches := rx.FindAllStringIndex(out, -1)
101+
if len(matches) != 1 {
102+
t.Fatalf("expected one match for %s got %d\n%s", ace, len(matches), out)
103+
}
103104
}
104105
}

0 commit comments

Comments
 (0)