From 912a4e6fc797ca31e7d620ed1a220a3d49577d8c Mon Sep 17 00:00:00 2001 From: "Sean T. Allen" Date: Wed, 15 Sep 2021 12:43:27 -0400 Subject: [PATCH] Enforce security policy at unmount This is the first iteration of policy enforcement at unmount. There is an additional set of functionality that will come as part of a larger change in the near future. With this commit, we record that a device has been unmounted such that it isn't eligible to be used in any overlay after unmounting. In a future commit, I will be adding disallowing unmounting a device that is being used by a running container. Signed-off-by: Sean T. Allen --- internal/guest/runtime/hcsv2/uvm.go | 2 +- internal/guest/storage/pmem/pmem.go | 4 ++ internal/guest/storage/pmem/pmem_test.go | 45 +++++++++++++- internal/guest/storage/scsi/scsi.go | 6 +- internal/guest/storage/scsi/scsi_test.go | 62 ++++++++++++++++++- .../mountmonitoringsecuritypolicyenforcer.go | 10 ++- pkg/securitypolicy/securitypolicy_test.go | 53 +++++++++++++++- pkg/securitypolicy/securitypolicyenforcer.go | 28 ++++++++- .../securitypolicy/securitypolicyenforcer.go | 28 ++++++++- 9 files changed, 224 insertions(+), 14 deletions(-) diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 9d9af86ac9..4056e579ee 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -437,7 +437,7 @@ func modifyMappedVirtualDisk(ctx context.Context, rt prot.ModifyRequestType, mvd return nil case prot.MreqtRemove: if mvd.MountPath != "" { - if err := scsi.Unmount(ctx, mvd.Controller, mvd.Lun, mvd.MountPath, mvd.Encrypted); err != nil { + if err := scsi.Unmount(ctx, mvd.Controller, mvd.Lun, mvd.MountPath, mvd.Encrypted, mvd.VerityInfo, securityPolicy); err != nil { return err } } diff --git a/internal/guest/storage/pmem/pmem.go b/internal/guest/storage/pmem/pmem.go index b68018fa04..8af7207bad 100644 --- a/internal/guest/storage/pmem/pmem.go +++ b/internal/guest/storage/pmem/pmem.go @@ -128,6 +128,10 @@ func Unmount(ctx context.Context, devNumber uint32, target string, mappingInfo * trace.Int64Attribute("device", int64(devNumber)), trace.StringAttribute("target", target)) + if err := securityPolicy.EnforceDeviceUnmountPolicy(target); err != nil { + return errors.Wrapf(err, "unmounting pmem device from %s denied by policy", target) + } + if err := storage.UnmountPath(ctx, target, true); err != nil { return errors.Wrapf(err, "failed to unmount target: %s", target) } diff --git a/internal/guest/storage/pmem/pmem_test.go b/internal/guest/storage/pmem/pmem_test.go index 0dd0f63d18..d147ede5f2 100644 --- a/internal/guest/storage/pmem/pmem_test.go +++ b/internal/guest/storage/pmem/pmem_test.go @@ -227,7 +227,7 @@ func Test_Mount_Valid_Data(t *testing.T) { } } -func Test_Security_Policy_Enforcement(t *testing.T) { +func Test_Security_Policy_Enforcement_Mount_Calls(t *testing.T) { clearTestDependencies() osMkdirAll = func(path string, perm os.FileMode) error { @@ -249,6 +249,49 @@ func Test_Security_Policy_Enforcement(t *testing.T) { t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMountCalls, enforcer.DeviceMountCalls) } + expectedDeviceUnmountCalls := 0 + if enforcer.DeviceUnmountCalls != expectedDeviceUnmountCalls { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceUnmountCalls, enforcer.DeviceUnmountCalls) + } + + expectedOverlay := 0 + if enforcer.OverlayMountCalls != expectedOverlay { + t.Fatalf("expected %d attempts at overlay mount enforcement, got %d", expectedOverlay, enforcer.OverlayMountCalls) + } +} + +func Test_Security_Policy_Enforcement_Unmount_Calls(t *testing.T) { + clearTestDependencies() + + osMkdirAll = func(path string, perm os.FileMode) error { + return nil + } + + unixMount = func(source string, target string, fstype string, flags uintptr, data string) error { + return nil + } + + enforcer := mountMonitoringSecurityPolicyEnforcer() + err := Mount(context.Background(), 0, "/fake/path", nil, nil, enforcer) + if err != nil { + t.Fatalf("expected nil err, got: %v", err) + } + + err = Unmount(context.Background(), 0, "/fake/path", nil, nil, enforcer) + if err != nil { + t.Fatalf("expected nil err, got: %v", err) + } + + expectedDeviceMountCalls := 1 + if enforcer.DeviceMountCalls != expectedDeviceMountCalls { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMountCalls, enforcer.DeviceMountCalls) + } + + expectedDeviceUnmountCalls := 1 + if enforcer.DeviceUnmountCalls != expectedDeviceUnmountCalls { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceUnmountCalls, enforcer.DeviceUnmountCalls) + } + expectedOverlay := 0 if enforcer.OverlayMountCalls != expectedOverlay { t.Fatalf("expected %d attempts at overlay mount enforcement, got %d", expectedOverlay, enforcer.OverlayMountCalls) diff --git a/internal/guest/storage/scsi/scsi.go b/internal/guest/storage/scsi/scsi.go index ebb2bd1bf2..c95ab9d555 100644 --- a/internal/guest/storage/scsi/scsi.go +++ b/internal/guest/storage/scsi/scsi.go @@ -128,7 +128,7 @@ func Mount(ctx context.Context, controller, lun uint8, target string, readonly b // Unmount unmounts a SCSI device mounted at `target`. // // If `encrypted` is true, it removes all its associated dm-crypto state. -func Unmount(ctx context.Context, controller, lun uint8, target string, encrypted bool) (err error) { +func Unmount(ctx context.Context, controller, lun uint8, target string, encrypted bool, verityInfo *prot.DeviceVerityInfo, securityPolicy securitypolicy.SecurityPolicyEnforcer) (err error) { ctx, span := trace.StartSpan(ctx, "scsi::Unmount") defer span.End() defer func() { oc.SetSpanStatus(span, err) }() @@ -138,6 +138,10 @@ func Unmount(ctx context.Context, controller, lun uint8, target string, encrypte trace.Int64Attribute("lun", int64(lun)), trace.StringAttribute("target", target)) + if err = securityPolicy.EnforceDeviceUnmountPolicy(target); err != nil { + return errors.Wrapf(err, "unmounting scsi controller %d lun %d from %s denied by policy", controller, lun, target) + } + // Unmount unencrypted device if err := storage.UnmountPath(ctx, target, true); err != nil { return errors.Wrapf(err, "unmount failed: "+target) diff --git a/internal/guest/storage/scsi/scsi_test.go b/internal/guest/storage/scsi/scsi_test.go index 24a1e162c2..99cc77533d 100644 --- a/internal/guest/storage/scsi/scsi_test.go +++ b/internal/guest/storage/scsi/scsi_test.go @@ -388,7 +388,7 @@ func Test_Mount_Readonly_Valid_Data(t *testing.T) { } } -func Test_Read_Only_Security_Policy_Enforcement(t *testing.T) { +func Test_Read_Only_Security_Policy_Enforcement_Mount_Calls(t *testing.T) { clearTestDependencies() target := "/fake/path" @@ -420,13 +420,18 @@ func Test_Read_Only_Security_Policy_Enforcement(t *testing.T) { t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMounts, enforcer.DeviceMountCalls) } + expectedDeviceUnmounts := 0 + if enforcer.DeviceUnmountCalls != expectedDeviceUnmounts { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceUnmounts, enforcer.DeviceUnmountCalls) + } + expectedOverlay := 0 if enforcer.OverlayMountCalls != expectedOverlay { t.Fatalf("expected %d attempts at overlay mount enforcement, got %d", expectedOverlay, enforcer.OverlayMountCalls) } } -func Test_Read_Write_Security_Policy_Enforcement(t *testing.T) { +func Test_Read_Write_Security_Policy_Enforcement_Mount_Calls(t *testing.T) { clearTestDependencies() target := "/fake/path" @@ -458,6 +463,59 @@ func Test_Read_Write_Security_Policy_Enforcement(t *testing.T) { t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMounts, enforcer.DeviceMountCalls) } + expectedDeviceUnmounts := 0 + if enforcer.DeviceUnmountCalls != expectedDeviceUnmounts { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceUnmounts, enforcer.DeviceUnmountCalls) + } + + expectedOverlay := 0 + if enforcer.OverlayMountCalls != expectedOverlay { + t.Fatalf("expected %d attempts at overlay mount enforcement, got %d", expectedOverlay, enforcer.OverlayMountCalls) + } +} + +func Test_Security_Policy_Enforcement_Unmount_Calls(t *testing.T) { + clearTestDependencies() + + target := "/fake/path" + osMkdirAll = func(path string, perm os.FileMode) error { + if path != target { + t.Errorf("expected path: %v, got: %v", target, path) + return errors.New("unexpected path") + } + return nil + } + + controllerLunToName = func(ctx context.Context, controller, lun uint8) (string, error) { + return "", nil + } + + unixMount = func(source string, target string, fstype string, flags uintptr, data string) error { + // Fake the mount success + return nil + } + + enforcer := mountMonitoringSecurityPolicyEnforcer() + err := Mount(context.Background(), 0, 0, target, true, false, nil, nil, enforcer) + if err != nil { + t.Fatalf("expected nil err, got: %v", err) + } + + err = Unmount(context.Background(), 0, 0, target, false, nil, enforcer) + if err != nil { + t.Fatalf("expected nil err, got: %v", err) + } + + expectedDeviceMounts := 1 + if enforcer.DeviceMountCalls != expectedDeviceMounts { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMounts, enforcer.DeviceMountCalls) + } + + expectedDeviceUnmounts := 1 + if enforcer.DeviceUnmountCalls != expectedDeviceUnmounts { + t.Fatalf("expected %d attempt at pmem mount enforcement, got %d", expectedDeviceMounts, enforcer.DeviceUnmountCalls) + } + expectedOverlay := 0 if enforcer.OverlayMountCalls != expectedOverlay { t.Fatalf("expected %d attempts at overlay mount enforcement, got %d", expectedOverlay, enforcer.OverlayMountCalls) diff --git a/internal/guest/storage/test/policy/mountmonitoringsecuritypolicyenforcer.go b/internal/guest/storage/test/policy/mountmonitoringsecuritypolicyenforcer.go index d354e47275..0dea681746 100644 --- a/internal/guest/storage/test/policy/mountmonitoringsecuritypolicyenforcer.go +++ b/internal/guest/storage/test/policy/mountmonitoringsecuritypolicyenforcer.go @@ -7,8 +7,9 @@ import ( // For testing. Records the number of calls to each method so we can verify // the expected interactions took place. type MountMonitoringSecurityPolicyEnforcer struct { - DeviceMountCalls int - OverlayMountCalls int + DeviceMountCalls int + DeviceUnmountCalls int + OverlayMountCalls int } var _ securitypolicy.SecurityPolicyEnforcer = (*MountMonitoringSecurityPolicyEnforcer)(nil) @@ -18,6 +19,11 @@ func (p *MountMonitoringSecurityPolicyEnforcer) EnforceDeviceMountPolicy(target return nil } +func (p *MountMonitoringSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(target string) (err error) { + p.DeviceUnmountCalls++ + return nil +} + func (p *MountMonitoringSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { p.OverlayMountCalls++ return nil diff --git a/pkg/securitypolicy/securitypolicy_test.go b/pkg/securitypolicy/securitypolicy_test.go index f2a2a3f5e4..8f8d5d309a 100644 --- a/pkg/securitypolicy/securitypolicy_test.go +++ b/pkg/securitypolicy/securitypolicy_test.go @@ -122,9 +122,9 @@ func Test_StandardSecurityPolicyEnforcer_Devices_Initialization(t *testing.T) { } } -// Verify that StandardSecurityPolicyEnforcer.EnforcePmemMountPolicy will return -// an error when there's no matching root hash in the policy -func Test_EnforcePmemMountPolicy_No_Matches(t *testing.T) { +// Verify that StandardSecurityPolicyEnforcer.EnforceDeviceMountPolicy will +// return an error when there's no matching root hash in the policy +func Test_EnforceDeviceMountPolicy_No_Matches(t *testing.T) { f := func(p *generatedContainers) bool { policy := NewStandardSecurityPolicyEnforcer(p.containers, ignoredEncodedPolicyString) @@ -165,6 +165,53 @@ func Test_EnforceDeviceMountPolicy_Matches(t *testing.T) { } } +func Test_EnforceDeviceUmountPolicy_Removes_Device_Entries(t *testing.T) { + f := func(p *generatedContainers) bool { + policy := NewStandardSecurityPolicyEnforcer(p.containers, ignoredEncodedPolicyString) + + r := rand.New(rand.NewSource(time.Now().UnixNano())) + target := generateMountTarget(r) + rootHash := selectRootHashFromContainers(p, r) + + err := policy.EnforceDeviceMountPolicy(target, rootHash) + if err != nil { + return false + } + + // we set up an expected new data structure shape were + // the target has been removed, but everything else is + // the same + setupCorrectlyDone := false + expectedDevices := make([][]string, len(policy.Devices)) + for i, container := range policy.Devices { + expectedDevices[i] = make([]string, len(container)) + for j, storedTarget := range container { + if target == storedTarget { + setupCorrectlyDone = true + } else { + expectedDevices[i][j] = storedTarget + } + } + } + if !setupCorrectlyDone { + // somehow, setup failed. this should never happen without another test + // also failing + return false + } + + err = policy.EnforceDeviceUnmountPolicy(target) + if err != nil { + return false + } + + return cmp.Equal(policy.Devices, expectedDevices) + } + + if err := quick.Check(f, &quick.Config{MaxCount: 1000}); err != nil { + t.Errorf("Test_EnforceDeviceUmountPolicy_Removes_Device_Entries failed: %v", err) + } +} + // Verify that StandardSecurityPolicyEnforcer.EnforceOverlayMountPolicy will // return an error when there's no matching overlay targets. func Test_EnforceOverlayMountPolicy_No_Matches(t *testing.T) { diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 37622b6224..235b52f0ad 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -12,6 +12,7 @@ import ( type SecurityPolicyEnforcer interface { EnforceDeviceMountPolicy(target string, deviceHash string) (err error) + EnforceDeviceUnmountPolicy(unmountTarget string) (err error) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) EnforceStartContainerPolicy(containerID string, argList []string, envList []string) (err error) } @@ -38,7 +39,7 @@ type StandardSecurityPolicyEnforcer struct { // map them back to a container definition from the user supplied // SecurityPolicy // - // Devices is a listing of dm-verity root hashes seen when mounting a device + // Devices is a listing of targets seen when mounting a device // stored in a "per-container basis". As the UVM goes through its process of // bringing up containers, we have to piece together information about what // is going on. @@ -50,7 +51,7 @@ type StandardSecurityPolicyEnforcer struct { // in the supplied SecurityPolicy. Each "seen" layer is recorded in devices // as it is mounted. So for example, if a root hash mount is found for the // device being mounted and the first layer of the first container then we - // record the root hash in Devices[0][0]. + // record the device target in Devices[0][0]. // // Later, when overlay filesystems created, we verify that the ordered layers // for said overlay filesystem match one of the device orderings in Devices. @@ -214,6 +215,21 @@ func (policyState *StandardSecurityPolicyEnforcer) EnforceDeviceMountPolicy(targ return nil } +func (policyState *StandardSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(unmountTarget string) (err error) { + policyState.mutex.Lock() + defer policyState.mutex.Unlock() + + for _, container := range policyState.Devices { + for j, storedTarget := range container { + if unmountTarget == storedTarget { + container[j] = "" + } + } + } + + return nil +} + func (policyState *StandardSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { policyState.mutex.Lock() defer policyState.mutex.Unlock() @@ -409,6 +425,10 @@ func (p *OpenDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(target string, return nil } +func (p *OpenDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(target string) (err error) { + return nil +} + func (p *OpenDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { return nil } @@ -425,6 +445,10 @@ func (p *ClosedDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(target strin return errors.New("mounting is denied by policy") } +func (p *ClosedDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(target string) (err error) { + return errors.New("unmounting is denied by policy") +} + func (p *ClosedDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { return errors.New("creating an overlay fs is denied by policy") } diff --git a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go index 37622b6224..235b52f0ad 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go +++ b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go @@ -12,6 +12,7 @@ import ( type SecurityPolicyEnforcer interface { EnforceDeviceMountPolicy(target string, deviceHash string) (err error) + EnforceDeviceUnmountPolicy(unmountTarget string) (err error) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) EnforceStartContainerPolicy(containerID string, argList []string, envList []string) (err error) } @@ -38,7 +39,7 @@ type StandardSecurityPolicyEnforcer struct { // map them back to a container definition from the user supplied // SecurityPolicy // - // Devices is a listing of dm-verity root hashes seen when mounting a device + // Devices is a listing of targets seen when mounting a device // stored in a "per-container basis". As the UVM goes through its process of // bringing up containers, we have to piece together information about what // is going on. @@ -50,7 +51,7 @@ type StandardSecurityPolicyEnforcer struct { // in the supplied SecurityPolicy. Each "seen" layer is recorded in devices // as it is mounted. So for example, if a root hash mount is found for the // device being mounted and the first layer of the first container then we - // record the root hash in Devices[0][0]. + // record the device target in Devices[0][0]. // // Later, when overlay filesystems created, we verify that the ordered layers // for said overlay filesystem match one of the device orderings in Devices. @@ -214,6 +215,21 @@ func (policyState *StandardSecurityPolicyEnforcer) EnforceDeviceMountPolicy(targ return nil } +func (policyState *StandardSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(unmountTarget string) (err error) { + policyState.mutex.Lock() + defer policyState.mutex.Unlock() + + for _, container := range policyState.Devices { + for j, storedTarget := range container { + if unmountTarget == storedTarget { + container[j] = "" + } + } + } + + return nil +} + func (policyState *StandardSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { policyState.mutex.Lock() defer policyState.mutex.Unlock() @@ -409,6 +425,10 @@ func (p *OpenDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(target string, return nil } +func (p *OpenDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(target string) (err error) { + return nil +} + func (p *OpenDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { return nil } @@ -425,6 +445,10 @@ func (p *ClosedDoorSecurityPolicyEnforcer) EnforceDeviceMountPolicy(target strin return errors.New("mounting is denied by policy") } +func (p *ClosedDoorSecurityPolicyEnforcer) EnforceDeviceUnmountPolicy(target string) (err error) { + return errors.New("unmounting is denied by policy") +} + func (p *ClosedDoorSecurityPolicyEnforcer) EnforceOverlayMountPolicy(containerID string, layerPaths []string) (err error) { return errors.New("creating an overlay fs is denied by policy") }