Add missing AllowElevated policy check when creating a container#1624
Merged
anmaxvl merged 2 commits intomicrosoft:mainfrom Jan 27, 2023
SeanTAllen:missing-priv-check
Merged
Add missing AllowElevated policy check when creating a container#1624anmaxvl merged 2 commits intomicrosoft:mainfrom SeanTAllen:missing-priv-check
anmaxvl merged 2 commits intomicrosoft:mainfrom
SeanTAllen:missing-priv-check
Conversation
When we added AllowElevated and checked it was working correctly, we got it slightly wrong. When a container is started, we were adding in expected mounts that only happen for privileged containers and using those are mounts that are allowed. During testing, if AllowElevated was left off, a privileged container would fail to start seemingly indicating that all was good. However, all was not good. A malicious orchestrator with control of the API could create a container privileged that didn't contain any extra "privileged mounts" and the container would start as privileged with everything else that being privileged entails except for the mounts. This commit adds an explicit check as part of crete container to verify that is the container is attempting to be started as privileged that it has AllowElevated. Maksim and I both thought that this had been implemented. I remember it being implemented. Apparently that memory is incorrect. Either way, it was noticed last Thursday and here's the fix. Signed-off-by: Sean T. Allen <[email protected]>
Signed-off-by: Maksim An <[email protected]>
anmaxvl
approved these changes
Jan 25, 2023
helsaawy
approved these changes
Jan 25, 2023
Contributor
helsaawy
left a comment
helsaawy
left a comment
There was a problem hiding this comment.
minor feedback, LGTM overall
anmaxvl
pushed a commit
that referenced
this pull request
Jan 27, 2023
A change to one of these two checks was requested by Hamza as part of #1624. It was decided to get both instances in their own PR as the change was unrelated to the work in 1624. Signed-off-by: Sean T. Allen <[email protected]>
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
…rosoft#1624) * Add missing AllowElevated policy check when creating a container When we added AllowElevated and checked it was working correctly, we got it slightly wrong. When a container is started, we were adding in expected mounts that only happen for privileged containers and using those are mounts that are allowed. During testing, if AllowElevated was left off, a privileged container would fail to start seemingly indicating that all was good. However, all was not good. A malicious orchestrator with control of the API could create a container privileged that didn't contain any extra "privileged mounts" and the container would start as privileged with everything else that being privileged entails except for the mounts. This commit adds an explicit check as part of crete container to verify that is the container is attempting to be started as privileged that it has AllowElevated. Maksim and I both thought that this had been implemented. I remember it being implemented. Apparently that memory is incorrect. Either way, it was noticed last Thursday and here's the fix. Signed-off-by: Sean T. Allen <[email protected]>
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
A change to one of these two checks was requested by Hamza as part of microsoft#1624. It was decided to get both instances in their own PR as the change was unrelated to the work in 1624. Signed-off-by: Sean T. Allen <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
When we added AllowElevated and checked it was working correctly, we got it slightly wrong. When a container is started, we were adding in expected mounts that only happen for privileged containers and using those are mounts that are allowed.
During testing, if AllowElevated was left off, a privileged container would fail to start seemingly indicating that all was good. However, all was not good.
A malicious orchestrator with control of the API could create a container privileged that didn't contain any extra "privileged mounts" and the container would start as privileged with everything else that being privileged entails except for the mounts.
This commit adds an explicit check as part of crete container to verify that is the container is attempting to be started as privileged that it has AllowElevated.
Maksim and I both thought that this had been implemented. I remember it being implemented. Apparently that memory is incorrect. Either way, it was noticed last Thursday and here's the fix.
Signed-off-by: Sean T. Allen [email protected]