Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boot-instance: allow more images to run #109

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Feb 16, 2023

Setting rootfs_entry to /bin allowed to run binaries from that directory only. / works too and allows binaries from any path to run.

At the same time, Occlum did not get all the dynamically linked libraries loaded due to missing lib search paths. Add trusted LD_LIBRARY_PATH setting for the most common paths.

Setting rootfs_entry to /bin allowed to run binaries from
that directory only. / works too and allows binaries from
any path to run.

At the same time, Occlum did not get all the dynamically
linked libraries loaded due to missing lib search paths.
Add trusted LD_LIBRARY_PATH setting for the most common
paths.

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi mythi requested a review from a team as a code owner February 16, 2023 13:48
@mythi
Copy link
Contributor Author

mythi commented Feb 16, 2023

@qzheng527 please help review

@@ -22,7 +22,7 @@ fn main() -> Result<(), Box<dyn Error>> {
let rootfs_key = b"c7-32-b3-ed-44-df-ec-7b-25-2d-9a-32-38-8d-58-61";
let rootfs_upper_layer = "/sefs/upper";
let rootfs_lower_layer = "/sefs/lower";
let rootfs_entry = "/bin";
let rootfs_entry = "/";

Choose a reason for hiding this comment

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

The reason Occlum limit the entry path is for security. If changing to "/", every binaries in the image could be executed. For example, a workload " /opt/app/app_bin" saves some sensitve data in TEE which is encrypted. If no entry path limit, hacks may get the chance to run "/bin/cat" to dump the sensitve data out.
I understand that is the "trade-off" to support more images with different entry paths.
If we do that, the enclave-cc flow should take care of the security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so this is the same as entry_points in Occlum.json?

For example, a workload " /opt/app/app_bin" saves some sensitve data in TEE which is encrypted. If no entry path limit, hacks may get the chance to run "/bin/cat" to dump the sensitve data out.

Right, it's still open for CoCo in general so I would be OK to go with this PR. Is it possible to do this mount as read-only?

Choose a reason for hiding this comment

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

OK so this is the same as entry_points in Occlum.json?

For example, a workload " /opt/app/app_bin" saves some sensitve data in TEE which is encrypted. If no entry path limit, hacks may get the chance to run "/bin/cat" to dump the sensitve data out.

Right, it's still open for CoCo in general so I would be OK to go with this PR. Is it possible to do this mount as read-only?

Yes, it is the same as entry_points in Occlum.json.
It has nothing to do this the mount way. No limit in entry path just gives a "hole" for malicious user to do what they want.
But I am OK with this PR in current stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has nothing to do this the mount way. No limit in entry path just gives a "hole" for malicious user to do what they want.

Yes the question about mount was not related to entry_point directly. I was just thinking can we make the rootfs read-only to ensure nothing sensitive is written there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qzheng527 could you also "Approve" via github review, thanks

Copy link
Member

Choose a reason for hiding this comment

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

If we do that, the enclave-cc flow should take care of the security.

@qzheng527 could you say a little more about what would be expected of the enclave-cc flow to enforce security?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do that, the enclave-cc flow should take care of the security.

@qzheng527 could you say a little more about what would be expected of the enclave-cc flow to enforce security?

@qzheng527 did you see this question?

Choose a reason for hiding this comment

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

If we do that, the enclave-cc flow should take care of the security.

@qzheng527 could you say a little more about what would be expected of the enclave-cc flow to enforce security?

@dcmiddle @mythi It is a open question, I don't have solid solution for this. But perhaps, the enclave-cc flow can handle the parse of the container image, then pass the entrypoint plus other information (key, envs, ...) to the app enclave to boot.

Copy link
Member

Choose a reason for hiding this comment

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

@qzheng527 thanks for the clarification. I opened an issue so we can collect thoughts.
#119

@mythi
Copy link
Contributor Author

mythi commented Feb 23, 2023

I would like this to be in v0.4.0

@fidencio fidencio merged commit 554e115 into confidential-containers:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants