Merge various fixes for C-LCOW since conf-aci/0.2.5 (part 1) #2581
+1,782
−179
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[Cherry-picked from ba849d8464ff019fe751872d458168bc9aec2256] While the entire PR contains several (perhaps unrelated) changes, the individual commits in this PR can be reviewed on their own, and there is some background in the commit messages. I've tested this with azcri-containerd as well as with the functional tests in hcsshim and some of the tests fail due to unrelated reasons (can't do VPMem, no GPU, WCOW) - I haven't noticed anything that could be caused by these changes. I also deployed these changes to a VM and can run some confidential workloads manually. Outstanding work that will not be done in this PR: - Deny unmounting of in-use layers (this should already be impossible due to directories being in use, but can result in rego metadata being inconsistent) Missing go tests that will not be part of this PR - I have local stash for these, but they are not fully working. Can do separately: - Functional test for checking that a bad OCISpec.Rootfs is rejected (tested manually) - Functional test for checking that a bad containerID is rejected (tested manually) Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Since the virtual pod ID is used to construct paths, it needs to be validated in the same way we validate container/sandbox IDs. This only applies to confidential. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
…il for confidential [cherry-picked from 73a7151ad8f15aad9f0e4eef6b86ed1a1a5a0953] This fixes a vulnerability where the host can provide arbitrary commands as hooks in the OCISpecification, which will get executed when passed to runc. Normally, ociSpec.Hooks is cleared explicitly by the host, and so we should never get passed hooks from a genuine host. The only case where OCI hooks would be set is if we are running a GPU container, in which case guest-side code in GCS will add hooks to set up the device - see addNvidiaDeviceHook. This restriction will also apply to the hook set this way by ourselves, however this is fine for now as we do not support Confidential GPUs. When we do support GPU confidential containers, we will need to pull this enforcement into Rego, and allow the policy to determine whether the hook should be allowed (along with enforcing which devices are allowed to be exposed to the container). Even after this fix, we still have unenforced fields in the OCISpecification that we receive from the host, that may allow the host to, for example, add device nodes, and we should harden this further in a future release. Tested using the following host-side reproduction patch: ```diff diff --git a/internal/hcsoci/hcsdoc_lcow.go b/internal/hcsoci/hcsdoc_lcow.go index db94d73..a270c13ac 100644 --- a/internal/hcsoci/hcsdoc_lcow.go +++ b/internal/hcsoci/hcsdoc_lcow.go @@ -94,6 +94,22 @@ func createLinuxContainerDocument(ctx context.Context, coi *createOptionsInterna return nil, err } + isSandbox := spec.Annotations[annotations.KubernetesContainerType] == "sandbox" + + if !isSandbox { + if spec.Hooks == nil { + spec.Hooks = &specs.Hooks{} + } + timeout := 5000000 + spec.Hooks.StartContainer = append(spec.Hooks.StartContainer, specs.Hook{ + Path: "/bin/sh", + Args: []string{"/bin/sh", "-c", "echo hacked by hooks > /hacked.txt"}, + Env: []string{"PATH=/usr/local/bin:/usr/bin:/bin:/sbin"}, + Timeout: &timeout, + }) + log.G(ctx).Info("Injected hook into OCISpec") + } + log.G(ctx).WithField("guestRoot", guestRoot).Debug("hcsshim::createLinuxContainerDoc") return &linuxHostedSystem{ SchemaVersion: schemaversion.SchemaV21(), ``` Non-confidential scenarios should not be affected. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
[cherry-picked from 5469e5c013e08dfca8aaf31dae8f617831fd5ac7]
For an arbitrary fragment offered by the host, currently we need to interpret the fragment code as a new Rego module in order for the framework to check it. However, we did not check that the namespace of the fragment, which is used as the namespace of the module, does not conflict with any of our own namespaces (e.g. framework, policy, or api). This means that as soon as we load it for policy checking, a malicious fragment could override enforcement points defined in the framework, including "load_fragment", and allow itself in, even if we would otherwise have denied it because of wrong issuer, etc.
This can be tested with the below fragment:
package framework
svn := "1"
framework_version := "0.3.1"
load_fragment := {"allowed": true, "add_module": true}
enforcement_point_info := {
"available": true,
"unknown": false,
"invalid": false,
"version_missing": false,
"default_results": {"allowed": true},
"use_framework": true
}
mount_device := {"allowed": true}
rw_mount_device := {"allowed": true}
mount_overlay := {"allowed": true}
create_container := {"allowed": true, "env_list": null, "allow_stdio_access": true}
unmount_device := {"allowed": true}
rw_unmount_device := {"allowed": true}
unmount_overlay := {"allowed": true}
exec_in_container := {"allowed": true, "env_list": null}
exec_external := {"allowed": true, "env_list": null, "allow_stdio_access": true}
shutdown_container := {"allowed": true}
signal_container_process := {"allowed": true}
plan9_mount := {"allowed": true}
plan9_unmount := {"allowed": true}
get_properties := {"allowed": true}
dump_stacks := {"allowed": true}
runtime_logging := {"allowed": true}
scratch_mount := {"allowed": true}
scratch_unmount := {"allowed": true}
However, our namespace parsing was also suspectable to attacks, and we might end up concluding that the fragment namespace is safe even if when the Rego engine parses it, it sees a package of "framework". This can be demonstrated with the following fragment:
package #aa
framework
svn := "0"
framework_version := "0.3.1"
load_fragment := {"allowed": true, "add_module": true}
And so we also ensure that the `package ...` line can't contain anything unusual.
Furthermore, since it can still be risky to inject arbitrary, untrusted Rego code into our execution context, we add another check prior to loading the fragment as a module, where we make sure that the fragment has to first pass the issuer and feed check, which do not require loading it.
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
[cherry-picked from 0ca40bb4f130b3508f4a130011463070328d40d0] - rego: Fix missing error reason when mounting a rw device to an existing mount point. This fixes a missing error message introduced in the last round of security fixes. It's not hugely important, but eases debugging if we get policy denials on mounting the scratch, for whatever reason. Also adds test for it. - Remove a no-op from rego Checked with @<Matthew Johnson (AR)> earlier that this basically does nothing and is just something left over. However I will not actually add a remove op for `metadata.started` for now. This PR is targeting the conf-aci branch on ADO because the commit being fixed is not on main yet. This should be backported to main together with the fixes from last month. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
anmaxvl
approved these changes
Jan 5, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first 5 commits of #2559