Add support for mount policy enforcement. #1311

anmaxvl · 2022-03-02T16:25:35Z

It is possible that a malicious mount can be used to attack an LCOW pod.
If the attacker has knowleldge of the workload running, they could
possibly change the environment for a container and alter its execution.

This PR adds support for describing and enforcing mount policy for a
given container. The mount policy closely follows OCI spec to be as
explicit as possible to the user. The policy can be made less explicit
in the future if needed.

The dev tool has been updated to support mount configurations and the
configuration spec is similar to CRI config with the exception of
unsupported features (e.g., selinux config).
The tool translates CRI config to appropriate mount type and options in
mount policy. Initial implementation doesn't support any wildcards and
mount source and destinations are compared directly.

CRI adds some default mounts for all Linux containers and they had to be
hardcoded in this codebase as well. Extra caution is needed in the
future, in case the list expands.

Additional changes have been made to how sandbox and hugepages mounts
are generated to make sure that the same utility functions are used to
generate appropriate mount specs.

TODO:

Testing/Validation

Signed-off-by: Maksim An maksiman@microsoft.com

anmaxvl · 2022-03-16T00:02:56Z

@SeanTAllen @KenGordon FYI

pkg/securitypolicy/securitypolicyenforcer.go

SeanTAllen · 2022-03-16T12:58:26Z

I worry about the handcrafted nature of the test coverage for the functionality. There's a ton of options and a large number of interleavings that probably aren't covered. I'd personally feel more confident if there were generative tests to cover all the edges. That said, I can imagine that writing them would be a pain as would understanding them without copious comments.

Do any of the existing unit tests cover this functionality or is it only the new functional test(s) that provide coverage?

anmaxvl · 2022-03-16T20:50:00Z

@SeanTAllen , I haven't added any unit tests, only the e2e functional ones. I'll get to the unit tests later.

internal/spec/spec.go

pkg/securitypolicy/config.go

internal/guestpath/paths.go

internal/guest/runtime/hcsv2/spec.go

pkg/securitypolicy/securitypolicyenforcer.go

kevpar · 2022-04-08T18:00:33Z

You have a merge conflict FYI

anmaxvl · 2022-04-08T19:12:12Z

You have a merge conflict FYI

done resolving

katiewasnothere

A small comment or so, but otherwise lgtm

internal/guest/runtime/hcsv2/uvm.go

kevpar · 2022-04-12T17:47:15Z

pkg/securitypolicy/securitypolicyenforcer.go

@@ -133,19 +190,28 @@ func NewStandardSecurityPolicyEnforcer(containers []securityPolicyContainer, enc
 func (c Containers) toInternal() ([]securityPolicyContainer, error) {
 	containerMapLength := len(c.Elements)
 	if c.Length != containerMapLength {
-		return nil, fmt.Errorf("container numbers don't match in policy. expected: %d, actual: %d", c.Length, containerMapLength)


As a general comment, if you have two sets of changes, things that are really important, and some general cleanup that is not important, it can be helpful to put them in separate PRs. That way it's easier for reviewers to focus on the really important part of the changes. This can be especially helpful with GitHub since it's so bad at just showing you the new pieces of a PR that you've looked at already.

You don't necessarily need to do that here, since it's been around for a while already, but good to consider in the future.

It is possible that a malicious mount can be used to attack an LCOW pod. If the attacker has knowleldge of the workload running, they could possibly change the environment for a container and alter its execution. This PR adds support for describing and enforcing mount policy for a given container. The mount policy closely follows OCI spec to be as explicit as possible to the user. The policy can be made less explicit in the future if needed. The dev tool has been updated to support mount configurations and the configuration spec is similar to CRI config with the exception of unsupported features (e.g., selinux config). The tool translates CRI config to appropriate mount type and options in mount policy. Initial implementation doesn't support any wildcards for the Destination, but supports REGEX for the Source. CRI adds some default mounts for all Linux containers and they had to be hardcoded in this codebase as well. Extra caution is needed in the future, in case the list expands. Additional changes have been made to how sandbox and hugepages mounts are generated to make sure that the same utility functions are used to generate appropriate mount specs. Add positive and negative tests for security policy mount constraints Hide mount enforcement behind a LCOWIntegrity feature flag Update securitypolicy tool docs Signed-off-by: Maksim An <maksiman@microsoft.com>

Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl · 2022-04-14T16:09:19Z

@kevpar moved ExtendDefaultMounts to SecurityPolicyEnforcer interface and removed unrelated code.

kevpar

LGTM

…)" This reverts commit 2028de8. During local testing a gcs with an older version of security policy was used when doing the fix: microsoft#1322. As we can see, the quotations weren't there. However, later a PR was merged, which added them: microsoft#1311 Signed-off-by: Maksim An <maksiman@microsoft.com>

This reverts commit 2028de8. During local testing a gcs with an older version of security policy was used when doing the fix: #1322. As we can see, the quotations weren't there. However, later a PR was merged, which added them: #1311 Signed-off-by: Maksim An <maksiman@microsoft.com>

Sync ADO with upstream to enable including test GCS binaries as part of dev-pipeline Related work items: #1311, #1322, #1341, #1343, #1345, #1347, #1348, #1350, #1353, #1354, #1355, #1358, #1361, #1365, #1368, #1369, #1370

* Add support for mount policy enforcement. It is possible that a malicious mount can be used to attack an LCOW pod. If the attacker has knowleldge of the workload running, they could possibly change the environment for a container and alter its execution. This PR adds support for describing and enforcing mount policy for a given container. The mount policy closely follows OCI spec to be as explicit as possible to the user. The policy can be made less explicit in the future if needed. The dev tool has been updated to support mount configurations and the configuration spec is similar to CRI config with the exception of unsupported features (e.g., selinux config). The tool translates CRI config to appropriate mount type and options in mount policy. Initial implementation doesn't support any wildcards for the Destination, but supports REGEX for the Source. CRI adds some default mounts for all Linux containers and they had to be hardcoded in this codebase as well. Extra caution is needed in the future, in case the list expands. Additional changes have been made to how sandbox and hugepages mounts are generated to make sure that the same utility functions are used to generate appropriate mount specs. Add positive and negative tests for security policy mount constraints Hide mount enforcement behind a LCOWIntegrity feature flag Update securitypolicy tool docs Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl force-pushed the mount-policy-enforcement branch 7 times, most recently from 54491be to caa48aa Compare March 11, 2022 22:36

anmaxvl marked this pull request as ready for review March 12, 2022 02:47

anmaxvl requested a review from a team as a code owner March 12, 2022 02:47

SeanTAllen reviewed Mar 16, 2022

View reviewed changes

pkg/securitypolicy/securitypolicyenforcer.go Outdated Show resolved Hide resolved

SeanTAllen reviewed Mar 16, 2022

View reviewed changes

pkg/securitypolicy/securitypolicyenforcer.go Outdated Show resolved Hide resolved

SeanTAllen reviewed Mar 16, 2022

View reviewed changes

pkg/securitypolicy/securitypolicyenforcer.go Outdated Show resolved Hide resolved

msscotb assigned katiewasnothere and msscotb Mar 17, 2022

anmaxvl force-pushed the mount-policy-enforcement branch from 3294385 to e6966f5 Compare March 21, 2022 18:26

kevpar reviewed Mar 21, 2022

View reviewed changes

internal/spec/spec.go Outdated Show resolved Hide resolved

katiewasnothere reviewed Mar 21, 2022

View reviewed changes

internal/spec/spec.go Outdated Show resolved Hide resolved

anmaxvl commented Mar 21, 2022

View reviewed changes

pkg/securitypolicy/config.go Outdated Show resolved Hide resolved

kevpar reviewed Mar 22, 2022

View reviewed changes

internal/guestpath/paths.go Show resolved Hide resolved

kevpar reviewed Mar 22, 2022

View reviewed changes

internal/guestpath/paths.go Outdated Show resolved Hide resolved

kevpar reviewed Mar 22, 2022

View reviewed changes

internal/guestpath/paths.go Show resolved Hide resolved

anmaxvl force-pushed the mount-policy-enforcement branch 2 times, most recently from 69beb99 to 2dbf0a0 Compare March 23, 2022 06:46

anmaxvl assigned kevpar Mar 23, 2022

anmaxvl force-pushed the mount-policy-enforcement branch 3 times, most recently from ed88272 to ead70b4 Compare March 23, 2022 06:56

kevpar reviewed Mar 28, 2022

View reviewed changes

internal/guest/runtime/hcsv2/spec.go Outdated Show resolved Hide resolved

anmaxvl force-pushed the mount-policy-enforcement branch 5 times, most recently from 0b942d6 to 8c92072 Compare March 29, 2022 23:59

katiewasnothere reviewed Apr 1, 2022

View reviewed changes

pkg/securitypolicy/securitypolicyenforcer.go Show resolved Hide resolved

anmaxvl force-pushed the mount-policy-enforcement branch 3 times, most recently from a0536ce to 15afe54 Compare April 7, 2022 17:16

anmaxvl force-pushed the mount-policy-enforcement branch from 15afe54 to 0ed9bd0 Compare April 8, 2022 19:11

katiewasnothere approved these changes Apr 9, 2022

View reviewed changes

anmaxvl force-pushed the mount-policy-enforcement branch from 0ed9bd0 to 679bf8c Compare April 11, 2022 04:44

kevpar reviewed Apr 12, 2022

View reviewed changes

internal/guest/runtime/hcsv2/uvm.go Outdated Show resolved Hide resolved

kevpar reviewed Apr 12, 2022

View reviewed changes

anmaxvl force-pushed the mount-policy-enforcement branch from 6a15f78 to 682b056 Compare April 13, 2022 20:00

Move ExtendDefaultMounts to SecurityPolicyEnforcer interface

2af6ba2

Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl force-pushed the mount-policy-enforcement branch from 682b056 to 2af6ba2 Compare April 13, 2022 20:04

kevpar approved these changes Apr 14, 2022

View reviewed changes

anmaxvl merged commit 655b7e1 into microsoft:master Apr 15, 2022

anmaxvl deleted the mount-policy-enforcement branch April 15, 2022 11:03

anmaxvl mentioned this pull request Apr 22, 2022

Revert "Fix working_dir negative test error expectation (#1348)" #1368

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for mount policy enforcement. #1311

Add support for mount policy enforcement. #1311

anmaxvl commented Mar 2, 2022 •

edited

Loading

anmaxvl commented Mar 16, 2022

SeanTAllen commented Mar 16, 2022

anmaxvl commented Mar 16, 2022

kevpar commented Apr 8, 2022

anmaxvl commented Apr 8, 2022

katiewasnothere left a comment

kevpar Apr 12, 2022

kevpar Apr 12, 2022

anmaxvl commented Apr 14, 2022

kevpar left a comment

Add support for mount policy enforcement. #1311

Add support for mount policy enforcement. #1311

Conversation

anmaxvl commented Mar 2, 2022 • edited Loading

anmaxvl commented Mar 16, 2022

SeanTAllen commented Mar 16, 2022

anmaxvl commented Mar 16, 2022

kevpar commented Apr 8, 2022

anmaxvl commented Apr 8, 2022

katiewasnothere left a comment

Choose a reason for hiding this comment

kevpar Apr 12, 2022

Choose a reason for hiding this comment

kevpar Apr 12, 2022

Choose a reason for hiding this comment

anmaxvl commented Apr 14, 2022

kevpar left a comment

Choose a reason for hiding this comment

anmaxvl commented Mar 2, 2022 •

edited

Loading