Skip to content

Fix policy enforcement to handle identical layers.#1441

Merged
anmaxvl merged 4 commits intomicrosoft:masterfrom
anmaxvl:policy-duplicate-layers
Jun 28, 2022
Merged

Fix policy enforcement to handle identical layers.#1441
anmaxvl merged 4 commits intomicrosoft:masterfrom
anmaxvl:policy-duplicate-layers

Conversation

@anmaxvl
Copy link
Copy Markdown
Contributor

@anmaxvl anmaxvl commented Jun 24, 2022

Read-only container layer enforcement currently tracks which layers
have been mounted for each container. The state is being tracked
by maintaining a 2D slice of device targets and each slice represents
a potential overlay FS that can be used for a given container.
Index i in the Devices slice corresponds to container at index i
in the policy. For example for containers with the following hashes:

[
  [hash1, hash2, hash3],
  [hash1, hash4],
]

The corresponding Devices slice will look something like:

[
  [/mnt/target1, /mnt/target2, /mnt/target3],
  [/mnt/target1, /mnt/target4],
]

Each individual slice then corresponds to a potential overlay fs:

  • overlay1: [/mnt/target1, /mnt/target2, /mnt/target3]
  • overlay2: [/mnt/target1, /mnt/target4]

The issue arises when container contains duplicate layers like:

[
  [hash1, hash2, hash2],
  [hash1, hash4],
]

The potential overlays will be computed as following:

[
  [/mnt/target1, /mnt/target2, /mnt/target2],
  [/mnt/target1, /mnt/target4],
]

Instead of:

[
  [/mnt/target1, /mnt/target2, /mnt/target3],
  [/mnt/target1, /mnt/target4],
]

The issue is reproducable by the following Dockerfile:

FROM ubuntu:latest
COPY script.sh .
RUN chmod +x script.sh

where script.sh already has executable permission flag set.

To address the issue, the logic to track currently mounted devices
and enforcing the overlay has been updated.
Instead of tracking potential overlays, we track the devices that
have been mounted and their hashes:

{
  /mnt/target1: hash1,
  /mnt/target2: hash2,
  /mnt/target3: hash2,
}

During overlay, we map the mount targets to the hashes and check
the resulting hash chain against the ones in the policy.

Signed-off-by: Maksim An [email protected]

@anmaxvl anmaxvl requested a review from a team as a code owner June 24, 2022 01:33
@anmaxvl
Copy link
Copy Markdown
Contributor Author

anmaxvl commented Jun 24, 2022

@SeanTAllen @KenGordon @svolos FYI

@SeanTAllen
Copy link
Copy Markdown
Contributor

For the record, I had an idea for how to address this and Maksim's was much better than mine. I was to stuck in thinking about changing the way it current worked rather than stepping back and going "wait, what is the best way to address this".

I heartily endorse this PR. Let's ship this!

@SeanTAllen
Copy link
Copy Markdown
Contributor

FYI @matajoh

Comment thread pkg/securitypolicy/securitypolicyenforcer.go Outdated
Copy link
Copy Markdown
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, one comment

@anmaxvl anmaxvl force-pushed the policy-duplicate-layers branch from 37d0904 to 364e112 Compare June 24, 2022 14:40
Copy link
Copy Markdown
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits, but LGTM overall

Comment thread pkg/securitypolicy/securitypolicyenforcer.go
Comment thread pkg/securitypolicy/securitypolicyenforcer.go Outdated
EncodedSecurityPolicy: encoded,
Containers: containers,
Devices: devices,
Devices: map[string]string{},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment on what is being stored in this map?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied the docs patch, let me know if this works.

@anmaxvl anmaxvl force-pushed the policy-duplicate-layers branch from 71e0016 to 6d853d5 Compare June 28, 2022 01:17
anmaxvl added 2 commits June 28, 2022 10:33
Read-only container layer enforcement currently tracks which layers
have been mounted for each container. The state is being tracked
by maintaining a 2D slice of device targets and each slice represents
a potential overlay FS that can be used for a given container.
Index `i` in the Devices slice corresponds to container at index `i`
in the policy. For example for containers with the following hashes:
```
[
  [hash1, hash2, hash3],
  [hash1, hash4],
]
```
The corresponding Devices slice will look something like:
```
[
  [/mnt/target1, /mnt/target2, /mnt/target3],
  [/mnt/target1, /mnt/target4],
]
```

Each individual slice then corresponds to a potential overlay fs:
  - overlay1: `[/mnt/target1, /mnt/target2, /mnt/target3]`
  - overlay2: `[/mnt/target1, /mnt/target4]`

The issue arises when container contains duplicate layers like:
```
[
  [hash1, hash2, hash2],
  [hash1, hash4],
]
```
The potential overlays will be computed as following:
```
[
  [/mnt/target1, /mnt/target2, /mnt/target2],
  [/mnt/target1, /mnt/target4],
]
```
Instead of:
```
[
  [/mnt/target1, /mnt/target2, /mnt/target3],
  [/mnt/target1, /mnt/target4],
]
```
The issue is reproducable by the following Dockerfile:
```
FROM ubuntu:latest
COPY script.sh .
RUN chmod +x script.sh
```
where `script.sh` already has executable permission flag set.

To address the issue, the logic to track currently mounted devices
and enforcing the overlay has been updated.
Instead of tracking potential overlays, we track the devices that
have been mounted and their hashes:
```
{
  /mnt/target1: hash1,
  /mnt/target2: hash2,
  /mnt/target3: hash2,
}
```
During overlay, we map the mount targets to the hashes and check
the resulting hash chain against the ones in the policy.

Signed-off-by: Maksim An <[email protected]>
@anmaxvl anmaxvl force-pushed the policy-duplicate-layers branch from 6d853d5 to f6b45cc Compare June 28, 2022 17:34
@anmaxvl anmaxvl merged commit aa40057 into microsoft:master Jun 28, 2022
@anmaxvl anmaxvl deleted the policy-duplicate-layers branch June 28, 2022 21:16
anmaxvl added a commit that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants