Skip to content

Commit

Permalink
Set root dir permissions on Windows with SET instead of DENY, and ove…
Browse files Browse the repository at this point in the history 
…rwrite existing DACL (#1965)

Co-authored-by: Zack Olson <[email protected]>
  • Loading branch information
@RebeccaMahany @zackattack01
RebeccaMahany and zackattack01 authored Nov 21, 2024
1 parent 7d72304 commit 84f7e1a
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 61 deletions.
108 changes: 57 additions & 51 deletions cmd/launcher/svc_config_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"log/slog"
"strings"
"time"
"unsafe"

"github.com/kolide/kit/version"
"github.com/kolide/launcher/pkg/launcher"
Expand All @@ -36,10 +35,6 @@ const (
// we add or update the currentVersionKeyName alongside the existing keys from installation
currentVersionRegistryKeyFmt = `Software\Kolide\Launcher\%s\%s`
currentVersionKeyName = `CurrentVersionNum`

// these are the flag values for the actual "write" ACLs that we see through Get-Acl in powershell. they are not exposed as external constants
// github.com/Microsoft/go-winio/internal/fs.FILE_WRITE_DATA|github.com/Microsoft/go-winio/internal/fs.FILE_CREATE_PIPE_INSTANCE|github.com/Microsoft/go-winio/internal/fs.FILE_WRITE_PROPERTIES|github.com/Microsoft/go-winio/internal/fs.FILE_WRITE_ATTRIBUTES (278) = 0x116
accessPermissionsAllWrites = 0x116
)

func checkServiceConfiguration(logger *slog.Logger, opts *launcher.Options) {
Expand Down Expand Up @@ -328,11 +323,13 @@ func checkCurrentVersionMetadata(logger *slog.Logger, identifier string) {
)
}

// checkRootDirACLs verifies that there is an explicit denial for builtin/users write permissions
// set on the root directory. If none exists, a new one is created and added to the existing
// security configuration for the directory. errors are logged but not retried, as we will attempt this
// checkRootDirACLs sets a security policy on the root directory to ensure that
// SYSTEM, administrators, and the directory owner have full access, but that regular
// users only have read/execute permission. errors are logged but not retried, as we will attempt this
// on every launcher startup
func checkRootDirACLs(logger *slog.Logger, rootDirectory string) {
logger = logger.With("component", "checkRootDirACLs")

if strings.TrimSpace(rootDirectory) == "" {
logger.Log(context.TODO(), slog.LevelError,
"unable to check directory permissions without root dir set, skipping",
Expand All @@ -342,85 +339,94 @@ func checkRootDirACLs(logger *slog.Logger, rootDirectory string) {
return
}

// Get the current security descriptor for the directory
sd, err := windows.GetNamedSecurityInfo(
rootDirectory,
windows.SE_FILE_OBJECT,
windows.DACL_SECURITY_INFORMATION,
)

// Get all the SIDs we need for our permissions
usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid)
if err != nil {
logger.Log(context.TODO(), slog.LevelError,
"gathering existing ACL from named sec info",
"failed getting builtin users SID",
"err", err,
)

return
}

existingDACL, _, err := sd.DACL()
adminsSID, err := windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid)
if err != nil {
logger.Log(context.TODO(), slog.LevelError,
"getting DACL from security descriptor",
"failed getting builtin admins SID",
"err", err,
)

return
}

usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid)
creatorOwnerSID, err := windows.CreateWellKnownSid(windows.WinCreatorOwnerSid)
if err != nil {
logger.Log(context.TODO(), slog.LevelError,
"failed getting builtin users SID",
"failed getting creator/owner SID",
"err", err,
)

return
}

// first iterate the existing ACEs for the directory, we're checking to see
// if there is already a DENY entry set for user's group to avoid recreating every time
for i := 0; i < int(existingDACL.AceCount); i++ {
var ace *windows.ACCESS_ALLOWED_ACE
if aceErr := windows.GetAce(existingDACL, uint32(i), &ace); aceErr != nil {
logger.Log(context.TODO(), slog.LevelWarn,
"encountered error parsing ACE from existing DACL",
"err", aceErr,
)

return
}

// do the easy checks first and continue if this isn't the ACE we're looking for
if ace.Mask != accessPermissionsAllWrites || ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE {
continue
}

sid := (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(ace)) + unsafe.Offsetof(ace.SidStart)))
if sid.Equals(usersSID) {
logger.Log(context.TODO(), slog.LevelDebug,
"root directory already had proper DACL permissions set, skipping",
)
systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid)
if err != nil {
logger.Log(context.TODO(), slog.LevelError,
"failed getting SYSTEM SID",
"err", err,
)

return
}
return
}

// We want to mirror the permissions set in Program Files:
// SYSTEM, admin, and creator/owner have full control; users are allowed only read and execute.
explicitAccessPolicies := []windows.EXPLICIT_ACCESS{
{
AccessPermissions: accessPermissionsAllWrites, // deny writes
AccessMode: windows.DENY_ACCESS,
Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, // ensure denial is inherited by sub folders
AccessPermissions: windows.GENERIC_ALL,
AccessMode: windows.SET_ACCESS,
Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, // ensure access is inherited by sub folders
Trustee: windows.TRUSTEE{
TrusteeForm: windows.TRUSTEE_IS_SID,
TrusteeType: windows.TRUSTEE_IS_GROUP,
TrusteeValue: windows.TrusteeValueFromSID(systemSID),
},
},
{
AccessPermissions: windows.GENERIC_ALL,
AccessMode: windows.SET_ACCESS,
Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, // ensure access is inherited by sub folders
Trustee: windows.TRUSTEE{
TrusteeForm: windows.TRUSTEE_IS_SID,
TrusteeType: windows.TRUSTEE_IS_GROUP,
TrusteeValue: windows.TrusteeValueFromSID(adminsSID),
},
},
{
AccessPermissions: windows.GENERIC_READ | windows.GENERIC_EXECUTE,
AccessMode: windows.SET_ACCESS,
Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, // ensure access is inherited by sub folders
Trustee: windows.TRUSTEE{
TrusteeForm: windows.TRUSTEE_IS_SID,
TrusteeType: windows.TRUSTEE_IS_GROUP,
TrusteeValue: windows.TrusteeValueFromSID(usersSID),
},
},
{
AccessPermissions: windows.GENERIC_ALL,
AccessMode: windows.SET_ACCESS,
Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, // ensure access is inherited by sub folders
Trustee: windows.TRUSTEE{
TrusteeForm: windows.TRUSTEE_IS_SID,
TrusteeType: windows.TRUSTEE_IS_GROUP,
TrusteeValue: windows.TrusteeValueFromSID(creatorOwnerSID),
},
},
}

// merge our existing DACL with our new explicit denial entry
newDACL, err := windows.ACLFromEntries(explicitAccessPolicies, existingDACL)
// Overwrite the existing DACL
newDACL, err := windows.ACLFromEntries(explicitAccessPolicies, nil)
if err != nil {
logger.Log(context.TODO(), slog.LevelError,
"generating new DACL from access entries",
Expand All @@ -430,7 +436,7 @@ func checkRootDirACLs(logger *slog.Logger, rootDirectory string) {
return
}

// apply the merged DACL to the root directory
// apply the new DACL to the root directory
err = windows.SetNamedSecurityInfo(
rootDirectory,
windows.SE_FILE_OBJECT,
Expand Down
62 changes: 52 additions & 10 deletions cmd/launcher/svc_config_windows_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,76 @@
package main

import (
"io"
"log/slog"
"testing"
"unsafe"

"github.com/kolide/launcher/pkg/threadsafebuffer"
"github.com/stretchr/testify/require"
"golang.org/x/sys/windows"
)

func Test_checkRootDirACLs(t *testing.T) {
t.Parallel()

rootDir := t.TempDir()
var logBytes threadsafebuffer.ThreadSafeBuffer

// Get info about our starting permissions
initialRootDirInfo, err := windows.GetNamedSecurityInfo(rootDir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
require.NoError(t, err, "getting named security info")
initialRootDirDacl, _, err := initialRootDirInfo.DACL()
require.NoError(t, err, "getting DACL")
require.NotNil(t, initialRootDirDacl)

var logBytes threadsafebuffer.ThreadSafeBuffer
slogger := slog.New(slog.NewTextHandler(&logBytes, &slog.HandlerOptions{
Level: slog.LevelDebug,
}))

// run the check once, expecting that we will correctly work all the way through
// and log that we've updated the ACLs for our new directory
// Check the root dir ACLs -- expect that we update the permissions
checkRootDirACLs(slogger, rootDir)
require.Contains(t, logBytes.String(), "updated ACLs for root directory")

// now clear the log, and rerun. if the previous run did what it was supposed to,
// and our check-before-write logic works correctly, we should detect the ACL we
// just added and exit early
io.Copy(io.Discard, &logBytes)
// Get our updated permissions
rootDirInfo, err := windows.GetNamedSecurityInfo(rootDir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
require.NoError(t, err, "getting named security info")
rootDirDacl, _, err := rootDirInfo.DACL()
require.NoError(t, err, "getting DACL")
require.NotNil(t, rootDirDacl)

// Confirm permissions have updated
require.NotEqual(t, initialRootDirInfo.String(), rootDirInfo.String(), "permissions did not change")

// Confirm that users only have access to read+execute
usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid)
require.NoError(t, err, "getting users SID")
userAceFound := false
for i := 0; i < int(rootDirDacl.AceCount); i++ {
var ace *windows.ACCESS_ALLOWED_ACE
require.NoError(t, windows.GetAce(rootDirDacl, uint32(i), &ace), "getting ACE")

if ace.Mask != windows.GENERIC_READ|windows.GENERIC_EXECUTE {
continue
}

sid := (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(ace)) + unsafe.Offsetof(ace.SidStart)))
if sid.Equals(usersSID) {
userAceFound = true
break
}
}
require.True(t, userAceFound, "ACE not found for WinBuiltinUsersSid with permissions GENERIC_READ|GENERIC_EXECUTE")

// Run checkRootDirACLs and confirm that the permissions do not change
checkRootDirACLs(slogger, rootDir)
require.NotContains(t, logBytes.String(), "updated ACLs for root directory")
require.Contains(t, logBytes.String(), "root directory already had proper DACL permissions set, skipping")

// Get permissions again
rootDirInfoUpdated, err := windows.GetNamedSecurityInfo(rootDir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
require.NoError(t, err, "getting named security info")
rootDirDaclUpdated, _, err := rootDirInfoUpdated.DACL()
require.NoError(t, err, "getting DACL")
require.NotNil(t, rootDirDaclUpdated)

// Confirm permissions have not updated
require.Equal(t, rootDirInfo.String(), rootDirInfoUpdated.String(), "permissions should not have changed")
}

0 comments on commit 84f7e1a

Please sign in to comment.