Reject blockdev SCSI mounts when security policy is active - #5
micromaomao with Copilot wants to merge 76 commits into
Conversation
A helper to gate changes behind confidential containers only. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
It seems to me that for 9p mounts from the host into a UVM, we are only supposed
to mount to exactly
^/run/gcs/c/<containerID>/mounts/m[0-9]+$
i.e. /run/gcs/c/.../m? is not just a prefix check. The only place which uses
this prefix is allocateLinuxResources, and it doesn't try to mount to anything
under the m<number> directory. Hence we should make the regex match there be
full string match too.
Combined with the fact that 9p mountpoints are already checked for duplicate,
this prevents any “mounting on top of symlinks” tricks from the host.
Another example, for mount source, the policy usually has:
"mounts": [
{
"destination": "/etc/resolv.conf",
"options": [
"rbind",
"rshared",
"rw"
],
"source": "sandbox:///tmp/atlas/resolvconf/.+",
"type": "bind"
}
],
and is intended to enforce that when starting the container, the source for the
/etc/resolv.conf mount must come from /tmp/atlas/resolvconf/ within the
sandboxMounts. Similar policies will be generated for file mounts:
{
"destination": "/mnt/volume",
"options": [
"rbind",
"rshared",
"rw"
],
"source": "sandbox:///tmp/atlas/azureFileVolume/.+",
"type": "bind"
},
This commit changes these cases to so that we use the anchored pattern,
effectively enforcing a full match.
Fixes: https://msazure.visualstudio.com/One/_workitems/edit/33064760
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Since these IDs are used to construct various paths (mount dir, path for resolv.conf, scratch path (containerScratchPathInUVM in lcow.go)), there is potential for path traversal attack here. We check that it can't be something weird for confidential containers (while still allowing anything the host passes if we're not in confidential mode, to not accidentally break other dependencies). Aside from CreateContainer, we also check for this in modify*Settings in case the host crafts a request with a malformed ID later on. Since the functional tests uses names like TestContainerExecLCOW-1df570f2-container, we can't enforce that this must strictly be a hex or a UUID. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
The main purpose of this is to prevent mounting host-controlled, non-encrypted
filesystems. Combined with the ability to mount to anywhere, this results in
code execution from the host into the guest. Unencrypted and
non-integrity-checked disks on its own also runs the risk of kernel filesystem
bugs.
While this commit does not yet prevent mounting to arbitrary paths, it makes
exploiting this much more difficult now as all the host can do is mount an empty
directory on top of things.
Using the reproducer in the bug report, we get the following deny message:
{"decision":"deny","input":{"encrypted":false,"ensureFilesystem":false,"filesystem":"","readonly":false,"rule":"mount_device","target":"/bin/"},"reason":{"errors":["ensureFilesystem must be set on rw device mounts","rw device mounts uses a filesystem that is not allowed","unencrypted scratch not allowed, device to be mounted must be encrypted"]}}
Fixes: https://msazure.visualstudio.com/One/_workitems/edit/33144273
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
If the host can control the rootfs path, by using otherwise legitimate 9p mounts, this lets it take over the container. Similar exploits might be possible by controlling ScratchDirPath or OCIBundlePath in this request as well, so we enforce that those paths are as expected too. Since in a previous commit we already ensure that 9pfs can only be mounted to /run/gcs/c/.../mounts, this means that it is no longer possible to use 9p mounts to exploit this, and nor should it be possible to use e.g. something in sandbox://. A later commit should also ensure that disks cannot be mounted to arbitrary paths too. Error message example: time="2025-06-09T15:34:38Z" level=fatal msg="starting the container \"fdb12ddbbfdeb1ee990abe892a06ac80304e5298c48d7eae2845c201e72efd5b\": rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to create container fdb12ddbbfdeb1ee990abe892a06ac80304e5298c48d7eae2845c201e72efd5b: guest RPC failure: OCISpecification.Root.Path \"/run/gcs/c/f3c2e64041edd6aa1c7f20c15c2bed0d2afffabb3b22aceac391d8f43d9fc567/mounts/m0\" must equal expected \"/run/gcs/c/fdb12ddbbfdeb1ee990abe892a06ac80304e5298c48d7eae2845c201e72efd5b/rootfs\": unknown" Fixes: https://msazure.visualstudio.com/One/_workitems/edit/33205622 Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Need to fix tests Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This replaces the currently unused* LCOWGlobalMountPrefixFmt and WCOWGlobalScsiMountPrefixFmt, and allows these format strings to be reused in a later commit for policy enforcement. *: I searched in hcsshim and azcri with no results. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This commit makes sure that we only accept mount requests with mountpoints that
we expect:
- Read-only scsi disks: These are container layers, and can only be mounted
under /run/mounts/scsi/m[0-9]+
- Read-write scsi disks: These are scratch disks, and should appear only at
/run/gcs/c/<container-id>, where <container-id> might also be the sandbox ID if
shared scratch is used.
- Overlay mounts (LCOWCombinedLayers): They should be at exactly
/run/gcs/c/<container-id>/rootfs, and we make sure that the container ID matches
with the one passed in the request, as this container ID is passed to rego.
We check the overlay mountpoints in Go code as we're checking rootfs in Go
already, unconditionally regardless of the policy. We also check that the
scratch dir passed in is correct.
Error message examples:
{"decision":"deny","input":{"deviceHash":"16b514057a06ad665f92c02863aca074fd5976c755d26bff16365299169e8415","mountPathRegex":"/run/mounts/scsi/m[0-9]+","readonly":true,"rule":"mount_device","target":"/tmp/scsi_m0"},"reason":{"errors":["mountpoint invalid"]}}
{"decision":"deny","input":{"encrypted":true,"ensureFilesystem":true,"filesystem":"xfs","mountPathRegex":"/run/gcs/c/[0-9a-fA-F]{64}","readonly":false,"rule":"mount_device","target":"/tmp/weird_root/8609646aeeafa903d6f15bb2d220e25c71d3b0596c6cfa0645230482975e4fe5"},"reason":{"errors":["mountpoint invalid"]}}
time="2025-06-10T17:36:55Z" level=fatal msg="run pod sandbox: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount container storage: guest modify: guest RPC failure: scratch path \"/tmp/weird_scratch/76250e7bb19ce9b0b3476451efe67ac4a3bd4ffe8dd51e639a203ec8fc813599\" must match regex \"^/run/gcs/c/[0-9a-fA-F]{64}/scratch/76250e7bb19ce9b0b3476451efe67ac4a3bd4ffe8dd51e639a203ec8fc813599$\": unknown"
time="2025-06-10T17:39:02Z" level=fatal msg="run pod sandbox: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount container storage: guest modify: guest RPC failure: combined layers target \"/\" does not match expected path \"/run/gcs/c/808fe5a536a33faa6c8a66a8af7b952ffd208bc7d2e96b85fb8ae353a575bc48/rootfs\": unknown"
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This matters the most for for device_mount. Some of this is not strictly necessary (such as using the correct overlay target) as we do the rootfs checks in Go code rather than rego, but we still try to make the test correct here. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
- Added test for path traversal fix. - Updated some error checking to require specific messages. - Update assertDecisionJSONContains to print out the actual error message if no match. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Check that invalid targets gets denied, and for rw mounts, ensureFileSystem and
encrypted must be set correctly. Make sure we can both mount and umount the
layers and the scratch disk, in any order. Also check that unmount is denied
for targets that has not been mounted, for both ro and rw mounts.
Also rename Test_Rego_EnforceDeviceUmountPolicy_Removes_Device_Entries to
Test_Rego_EnforceDeviceUnmountPolicy_Removes_Device_Entries for consistency
("unmount" instead of "umount").
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This test is currently broken already, due to VPMem multimapping not being disabled, and also due to missing required environment variables. In addition to fixing that, this commit also make it work with the latest changes, by using proper containerID, and disabling VPMem altogether (as it will fail the mount target check). Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…dential At least on confidential mode, read only disks are only supposed to be ext4, and in fact if it isn't, we will fail to read the verity info, and error out earlier. However, currently the host can specify a filesystem even when we're mounting a dm-verify protected volume. This presents a risk for exploits (but is not currently exploitable), for example the host could specify virtiofs, and prepare the correct vhost socket with tags like /dev/mapper/dm-verify-..., and be able to override a container layer. This is not currently exploitable thanks to the fact that we try to mount with the noload option first, and mounting would not continue if that fails. This option will prevent mounting virtiofs, 9p, overlay, etc. Note that the host can also specify its own options, but those options are only applied if we get through the first "mount with noload" stage. This commit prevents the host from specifying anything other than ext4, thus eliminating this risk. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Suggested by Ken in PR review Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Since that function creates the overlay mount, it is reasonable for it to also try to mount the read-write scratch disk first, for realism. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Mahati suggested below to add a new enforcement point rw_mount_device, instead of adding an input.readonly to the existing mount_device. This commit does that, and also bump the API version and set introducedVersion for the new rule, so that old policies will fallback to the "default" for this enforcement point, which in this commit is defined to allow. This also has the benefit that handwritten policies that does slightly different things in mount_device would not break. Since previously written policies would not have handled the scratch mount in mount_device, using a separate rule for read-write mounts is less likely to break those. However, doing it this way means that existing policies does not get this rw mountpoint protection. To remedy that, the next commit will add a "use_framework" mechanism to pass through new enforcement points to the framework by default, and use it for rw_mount_device instead of defaulting to allow. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…ment points This allows us to introduce the new rw_mount_device enforcement point while also letting it work even if we have an old policy. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Since we will use this string in more places, we make it a constant to ensure consistency. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
We will later need to copy the metadata in order to implement revertable
sections.
This extra function is necessary due to the fact that
map[string]map[string]interface{} is not convertable to map[string]interface{}
(or back), so the existing copy function cannot be used.
An alternative is to turn the deep copy functions into one generic function, but
reviewer was against it.
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This will be used in the policy enforcer in a later commit. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
We do this since the read-only unmount and read-write unmount may also use different logic (even though they don't do so right now), and if a customer has overridden unmount_device, using it also for the scratch disk would not be backwards compatible. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Currently this doesn't change anything, but in a later commit the caller will be changed to revert the state on errors that happens while carrying out an action after enforcing the security policy. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Suggested-by: Matthew Johnson <matjoh@microsoft.com> Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…anics in new code Per discussion with Ken, we concluded that the behaviour of panics is unpredictable / undesirable, and gcs should go into a hang state instead. The intention is to eventually replace all panic / log.Fatal with this. Suggested-by: Ken Gordon <kegordo@microsoft.com> Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Suggested-by: Matthew Johnson <matjoh@microsoft.com> Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Currently the host can pass in a share name with injected option in it, e.g.
"123,cache=loose". While this is currently probably harmless, it's still an
risk and so we should block these kind of mount option injections.
In Linux, this is parsed by v9fs_parse_options. It basically scans until the
next ',', and it doesn't matter whether we add quotes. In hcsshim, all plan9
mounts go through AddPlan9 on the host side, and that function uses a number as
the share name. Therefore, we will simply restrict the share name to be digits
only.
Test:
+++ b/internal/uvm/plan9.go
@@ -90,7 +90,7 @@ func (uvm *UtilityVM) AddPlan9(ctx context.Context, hostPath string, uvmPath str
RequestType: guestrequest.RequestTypeAdd,
Settings: guestresource.LCOWMappedDirectory{
MountPath: uvmPath,
- ShareName: name,
+ ShareName: name + ",cache=loose",
Port: plan9Port,
ReadOnly: readOnly,
},
Output:
failed to share directory C:\lcow_info\ into UVM: rpc error: code = Unknown desc = guest modify: guest RPC failure: invalid plan9 share name "1,cache=loose": must match regex "^[0-9]+$"
Closes: https://msazure.visualstudio.com/One/_workitems/edit/34370380
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Since the expected usage of this struct expects caller to undo operations in case of failure, it makes more sense to expect that the caller hold the lock throughout, until it has either committed the operation or undone it. This prevents accidental misuse, although in practice this struct is unlikely to be called from different threads anyway due to sequential bridge message processing in confidential containers. Locking hostMounts this way effectively means that mount/unmount operations are always single-threaded. However, this is the case in confidential containers anyway due to the sequential message processing, and on non-confidential containers hostMounts isn't used for now. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
In C-LCOW, we do not want to host to be able to arbitrarily control mount options. Currently there are two possible ways mount options might be specified by the host: 1. For read-only mounts (image layers), option "ro" is specified (see addLCOWLayer). 2. If the OCI spec passed by containerd contains physical/virtual disk mounts, it might contain mount options, and hcsshim would pass this through to GCS (see allocateLinuxResources). We can allow 1 (and in fact, require it to be consistent with the readOnly field in the request), and today C-LCOW does not support external disk mounts, and so we can reject any other mount options passed via route 2. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…nts, and prevent unmounting or deleting in-use things [cherry-picked from d0334883cd43eecbb401a6ded3e0317179a3e54b] This set of changes adds some checks (when running with a confidential policy) to prevent the host from trying to clean up mounts, overlays, or the container states dir when the container is running (or when the overlay has not been unmounted yet). This is through enhancing the existing `hostMounts` utility, as well as adding a `terminated` flag to the Container struct. The correct order of operations should always be: - mount read-only layers and scratch (in any order, and individual containers (not the sandbox) might not have their own scratch) - mount the overlay - start the container - container terminates - unmount overlay - unmount read-only layers and scratch The starting up order is implied, and we now explicitly deny e.g. unmounting layer/scratch before unmounting overlay, or unmounting the overlay while container has not terminated. We also deny deleteContainerState requests when the container is running or when the overlay is mounted. Doing so when a container is running can result in unexpectedly deleting its files, which breaks it in unpredictable ways and is bad. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
[cherry-picked from 1dd0b7ea0b0f91d3698f6008fb0bd5b0de777da6] Blocks mount option passing for 9p (which is accidental) and SCSI disks. - guest: Restrict plan9 share names to digits only on Confidential mode - hcsv2/uvm: Restrict SCSI mount options in confidential mode (The only one we allow is `ro`) Related work items: #34370380 Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This is to prepare for adding one additional parameter. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Modify the create_container enforcement point to take in an extra "devices"
input for Rego to enforce on.
Even though Rego gets the devices array as input, for now we will just not allow
any devices in framework.rego, and there is no way for a normal policy to allow
them, but this may be changed in the future, where the container definition in
the policy may contains a "devices" array defaulting to [].
We should let the policy enforcer ignore any dynamically added devices to
privileged containers, since if the policy allows privileged, adding devices to
/dev should be expected. We should also allow /dev/sev-guest unconditionally,
without the policy needing to have a rule for it. In order to implement this,
we simply take a copy of the devices array before we populate it with GCS-found
devices and sev-guest, and pass that copy to the enforcer.
Testing:
- Non-privileged and privileged containers work as expected, with the correct
devices.
- Modifying the host to inject malicious devices to workload container,
privileged containers gets policy denial ("devices not supported"), whereas
non-privileged containers gets "guest RPC failure: no such file or directory:
unknown" due to GCS trying to resolve the device's major/minor number for
cgroup.
- If the malicious device is injected to the sandbox container instead, we get
policy denials.
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…n any LinuxDevices [cherry-picked from 9f69e49..72b338a] [forward-ported from original PR 13653011 (e631aa42c6039ab0d228b746b280c26809433f00)] See https://msazure.visualstudio.com/ContainerPlatform/_git/Microsoft.hcsshim/pullrequest/13653011 Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Co-authored-by: micromaomao <6070533+micromaomao@users.noreply.github.com>
|
@copilot Also review these two things:
Both are mechanisms introduced after these fixes were written |
Co-authored-by: micromaomao <6070533+micromaomao@users.noreply.github.com>
Co-authored-by: micromaomao <6070533+micromaomao@users.noreply.github.com>
Added sections 5 and 6 to Virtual pod: Overall good. Container ID validation is applied to virtual pod IDs; path generation has defense-in-depth sanitization; cgroup control, CRI type override, and cleanup are correct. One finding (5.3): the BlockDev: No security issues found. Policy enforcement, verity integration, and hostMounts state tracking are all correctly applied to blockdev mounts. The symlink-based mount (instead of filesystem mount) correctly points to the verity device when verity is enabled. Device cgroup integration in |
Block device mounts (BlockDev) create symlinks to raw SCSI devices instead of mounting filesystems. There is currently no way to express block device mounts in the security policy, and the device cgroup rules added by updateBlockDeviceMounts are not checked by the policy enforcer. Add two defense-in-depth checks: 1. In modifyMappedVirtualDisk: reject BlockDev=true early when HasSecurityPolicy() is true, preventing symlink creation. 2. In CreateContainer: reject any OCI spec mounts with blockdev:// destination prefix when HasSecurityPolicy() is true, preventing device cgroup rule addition even if a symlink somehow existed. Co-authored-by: micromaomao <6070533+micromaomao@users.noreply.github.com>
8dcfae6 to
41711a3
Compare
3689bad to
3675c53
Compare
BlockDev mounts create symlinks to raw SCSI devices instead of mounting filesystems. The policy framework cannot express block device permissions, and the device cgroup rules added by
updateBlockDeviceMountsbypassEnforceCreateContainerPolicyV2entirely (they go intospec.Linux.Resources.Devices, notspec.Linux.Devices). A malicious host could use this to expose arbitrary block devices to confidential containers.Changes
modifyMappedVirtualDisk: Early-rejectBlockDev=truewhenHasSecurityPolicy(), preventing symlink creationCreateContainer: Reject OCI spec mounts withblockdev://destination prefix whenHasSecurityPolicy(), as defense-in-depth against device cgroup bypassSECURITY_REVIEW.md: Updated Finding 6.2 with root cause analysis and fix documentationBoth checks are no-ops without a security policy (non-confidential case where host is trusted).
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.