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

Add support for symlinks for regular files #1701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BobbyAtFortanix
Copy link

@BobbyAtFortanix BobbyAtFortanix commented Jan 5, 2024

AI and bank confidential applications use symlinks for regular files.

  • added support for system calls: link, linkat, symlink & symlinkat
  • added set_link() operation to libos_d_ops
  • implemented set_link() for chroot, chroot/encrypted & tmp filesystems
  • implemented follow_link() for chroot, chroot/encrypted & tmp filesystems
  • added symlink support to the chroot/encrypted filesystem
  • added hardlink and symlink support to tmp filesystem
  • added lstat(), readlink() & link() operations to Pal's handle_ops
  • implemented lstat(), readlink() & link() for linux & linux-sgx
  • implemented corresponding lstat(), readlink() & link() outcalls for enclave and host
  • implemented PalGetLinkStats(), PalReadLink() & PalCreateLink()
  • added unit tests for the above changes

new changes can be tested by running the following:

cd libos/test/regression
gramine-test --sgx pytest -v -k "_link_ or _symlink_"

Signed-off-by: Bobby Marinov Bobby.Marinov@fortanix.com


This change is Reviewable

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 30 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @BobbyAtFortanix)

a discussion (no related file):
Quick cosmetic comments:

  • Some files don't have your copyright (cases where you modified a lot) or have wrong-person copyright (the new LibOS test)
  • There are no changes in our Documentation; surely we have some info in docs that conflicts with your symlinks implementation
  • You didn't enable symlink-related LTP tests
  • I think we have symlink-related LibOS regression and FS tests, but those parts are commented out (or something like this) -- they need to be re-enabled now

a discussion (no related file):
This symlink support is completely pass-through (except tmpfs files which are by design in-memory-only).

What is the threat model here? Can you provide a security analysis? Why did you choose this pass-through design?

How is this situation solved: #1176 ?

In this meta-issue, Pawel (ex-Gramine maintainer who wrote the whole FS subsystem) suggested the following:

Symlinks (will fix gramineproject/graphene#516): as in-memory files emulated by Graphene

Pawel's design suggestion contradicts yours: in-memory-only symlinks vs passthrough-symlinks. Why did you choose your design?


@BobbyAtFortanix
Copy link
Author

* Some files don't have your copyright (cases where you modified a lot) or have wrong-person copyright (the new LibOS test)

Fixed in the new commit

* There are no changes in our Documentation; surely we have some info in docs that conflicts with your symlinks implementation

Some additional information added. I hope it is helpful.

* You didn't enable symlink-related LTP tests

Enabled symlink-related tests in ltp.cfg. Some of them are still failing but it is not because of the lack of symlink support. For those, I have added a comment specifying what is missing.

* I think we have symlink-related LibOS regression and FS tests, but those parts are commented out (or something like this) -- they need to be re-enabled now

I could not figure out how to enable those nor how to run them.

a discussion (no related file): This symlink support is completely pass-through (except tmpfs files which are by design in-memory-only).

chroot/encrypted is also pass-through but the symlink there is protected by the encryption.

What is the threat model here? Can you provide a security analysis? Why did you choose this pass-through design?

Our use-case scenario always uses an application inside a docker container. The files accessed by the application are hashed during container signing and if the hash does not match at run-time, we bug check.
The chroot (unencrypted) filesystem implementation uses the existing files in the application's container, including the symlinks. So for example, if in a container, /usr/bin/sh is a symlink to /usr/bin/dash, then there's an actual symlink on-disk in the container image. That symlink will function exactly the same when the application is run in the container. If we want a symlink to function the same when it's created by that app running the symlink syscall, then the symlink syscall implementation inside needs to make an actual symlink on disk.
For read-only chroot mounts, the filesystem hash includes the hashes of the symlinks, so symlinks are protected against modification or removal. Of course on read-only mounts, you can't make symlinks, so it doesn't matter how the symlink system call is implemented.
For read-write chroot mounts, no security is provided. An attacker can freely read or modify any data, including symlinks. These mounts should only be used for parts of the filesystem that do not contain sensitive data, and cannot compromise the security of the application if modified. So for example application configuration files should not be in writable chroot mounts even if the contents of the configuration files are not themselves secret. i.e. this implementation relies that the user that creates the container will use the chroot/encrypted mounts for sensitive information.

How is this situation solved: #1176 ?

The manifest solution can be used only for read-only symlinks that are known before the application is signed/ran. The proposed solution affects the cases where the application itself is creating symlinks during runtime.

In this meta-issue, Pawel (ex-Gramine maintainer who wrote the whole FS subsystem) suggested the following:>
Symlinks (will fix gramineproject/graphene#516): as in-memory files emulated by Graphene
Pawel's design suggestion contradicts yours: in-memory-only symlinks vs passthrough-symlinks. Why did you choose your design?

Because some apps want to create symlinks at run time.

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

I could not figure out how to enable those nor how to run them.

Check item 6 here: https://gramine.readthedocs.io/en/stable/devel/onboarding.html#fixing-a-bug

Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @BobbyAtFortanix)

a discussion (no related file):
Thanks for this initial analysis!

I guess my main question lies here:

Because some apps want to create symlinks at run time.

What exactly do these apps want from symlinks? Can't these symlinks be created ephemerally, completely inside the enclave? If not, then why is it important to have these symlinks persistent across SGX enclaves (or separate runs of the same SGX enclave)?

For read-only chroot mounts, the filesystem hash includes the hashes of the symlinks, so symlinks are protected against modification or removal.

Hm, are you sure that's how our manifest-generating Python scripts work? IIRC, our Python scripts simply resolve symlinks, so the hashes are NOT the symlink contents (string with a destination path) but the symlinked-to file contents.


@BobbyAtFortanix BobbyAtFortanix force-pushed the links_and_symlinks branch 2 times, most recently from ab06f66 to a441f6a Compare February 15, 2024 02:38
- added support for system calls: link, linkat, symlink & symlinkat
- added set_link() operation to libos_d_ops
- implemented set_link() for chroot/encrypted & tmpfs filesystems
- implemented follow_link() for chroot/encrypted & tmpfs
- added symlink support to the chroot/encrypted filesystem
- added hardlink and symlink support to tmp filesystem
- added regression unit tests for link and symlinks
- updated the copyright in the modified source files
- added symlink information to the documentation

Signed-off-by: Bobby Marinov <Bobby.Marinov@fortanix.com>
@BobbyAtFortanix
Copy link
Author

I think I addressed all of the concerns from the previous review(s).
I've kept the sym-links support only for chroot/encrypted and tmpfs file systems. This way there could be no possible leak or interference with the enclave protected data.

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Hi @BobbyAtFortanix, since the PR is very big, the Gramine team will need to put a priority on it, discuss the design and then do a deep-dive into the implementation. Would you be able to join our Gramine weekly meetings at some point, so we can discuss how and when we can proceed with this PR?

Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @BobbyAtFortanix)

Copy link
Author

@BobbyAtFortanix BobbyAtFortanix left a comment

Choose a reason for hiding this comment

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

I'll join the Gramine's 2024/02/27 (tomorrow) 7am PST meeting.

Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: )

Copy link
Author

@BobbyAtFortanix BobbyAtFortanix left a comment

Choose a reason for hiding this comment

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

I hope this will help during the code review.

Pal

  • added a flag to PalStreamOpen() specifying if the handle created is only going to be used for deleting the file. In that case there is no need to go and open the file (ocall) before deleting it via handle-less path based ocall. It also allows us to delete link files that fail open with ELOOP error.

LibOS

libos/test

  • added the basic symbolic & hard link tests for chroot(disabled), chroot/encrypted and tmpfs

libos/src/sys/libos_file.c

  • this is where the symlink/hardlink are created for all file systems

libos/src/fs

  • changes related to symlink & hardlink for tmpfs and chroot/encrypted

Other notes

  • Running non-file related LTP tests with tmpfs instead of chroot is failing with error. e.g. setrlimit05 test. this can be verified by modifying the LTP manifest file and running the test.

Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: )

Copy link
Author

@BobbyAtFortanix BobbyAtFortanix left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Thanks for this initial analysis!

I guess my main question lies here:

Because some apps want to create symlinks at run time.

What exactly do these apps want from symlinks? Can't these symlinks be created ephemerally, completely inside the enclave? If not, then why is it important to have these symlinks persistent across SGX enclaves (or separate runs of the same SGX enclave)?

For read-only chroot mounts, the filesystem hash includes the hashes of the symlinks, so symlinks are protected against modification or removal.

Hm, are you sure that's how our manifest-generating Python scripts work? IIRC, our Python scripts simply resolve symlinks, so the hashes are NOT the symlink contents (string with a destination path) but the symlinked-to file contents.

we don't know how the applications use symlinks. We have a batch file that runs the client application and collects all syscalls and pseudo-file opens and then creates a summary report. For security reasons the client send us that summary report.
In our Graphene(before it was renamed to Gramine) fork we have modified chroot to have read-only mounts. For those we generate a complete hash for everything under that mount so we can catch any attempt of modifying it without our knowledge. When a directory is enumerated we make sure that no files/dirs are added nor removed.
We also maintain a symlink directory entry instead of opening the symlink target.


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.

2 participants