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

Kernel+Userland: Add auto-jailing symlink of dynamic loader, introduce the new set-elf-jailed utility #24764

Conversation

supercomputer7
Copy link
Member

Relies on #22968. So this is a draft for now. The idea is to enable auto-jailing on programs by changing their dynamic loader to another which is almost identical, but enters jail-mode just before jumping to the program main function.

@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from 22b6e04 to 900598d Compare July 21, 2024 13:57
@supercomputer7 supercomputer7 marked this pull request as ready for review July 21, 2024 13:57
@supercomputer7 supercomputer7 changed the title Userland: Add set-elf-jailed utility Userland: Add auto-jailing version of dynamic loader, introduce the new set-elf-jailed utility Jul 21, 2024
@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from 900598d to b869faf Compare July 21, 2024 14:04
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jul 21, 2024
Copy link
Contributor

@DanShaders DanShaders left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

Userland/Utilities/set-elf-jailed.cpp Show resolved Hide resolved
@supercomputer7 supercomputer7 added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jul 22, 2024
@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from b869faf to 0880707 Compare July 23, 2024 17:49
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author labels Jul 23, 2024
@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from 0880707 to b37060c Compare July 23, 2024 18:33
@timschumi
Copy link
Member

I see the idea, but I can't help the feeling that there are better ways to signal this to the kernel (either temporarily or permanently) than building an entirely different DynamicLoader and patchelfing the files. Things like xattrs and environment variables come to mind.

@supercomputer7
Copy link
Member Author

supercomputer7 commented Aug 7, 2024

I see the idea, but I can't help the feeling that there are better ways to signal this to the kernel (either temporarily or permanently) than building an entirely different DynamicLoader and patchelfing the files. Things like xattrs and environment variables come to mind.

What about filesystems that don't support xattrs? Patching ELFs is a permanent solution and will work across all shells, filesystems, etc. Environment variables are also useful, but frankly it's more a temporary solution than a permanent one.

I don't mind having multiple ways to set a program into jail mode, and I think this is a completely valid way as well.

@supercomputer7
Copy link
Member Author

If adding a second dynamic loader is considered a bad idea (which I don't really think it is, because it's simple and makes it easy to check if an ELF file is auto-jailed by just running file), we could add an option to do this in the regular dynamic loader - we will add an option in the args parsing, and an environment variable as well. We could then add a special argument in the aux vector that will signal this (secure mode come to mind) or using a special section in an ELF file for this (so patchelf is still needed, or some replacement of our own).

Adding support for xattrs is also not-a-bad idea, just not something I'd want to mess around with for now (surely not for this).

@supercomputer7
Copy link
Member Author

supercomputer7 commented Aug 8, 2024

Another good idea - make /usr/lib/ldjail.so a symlink to the original loader, so we don’t have 2 almost identical loaders on disk. If you run a program through the symlink, the loader can detect this (we could add a passing of the basename of the loader in the aux vector, I think this should work fine) and then set the program into jail mode accordingly.

@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from b37060c to 457fdab Compare August 9, 2024 10:15
@supercomputer7 supercomputer7 marked this pull request as draft August 9, 2024 10:15
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Aug 9, 2024
@supercomputer7 supercomputer7 changed the title Userland: Add auto-jailing version of dynamic loader, introduce the new set-elf-jailed utility Kernel+Userland: Add auto-jailing symlink of dynamic loader, introduce the new set-elf-jailed utility Aug 9, 2024
@supercomputer7
Copy link
Member Author

Another good idea - make /usr/lib/ldjail.so a symlink to the original loader, so we don’t have 2 almost identical loaders on disk. If you run a program through the symlink, the loader can detect this (we could add a passing of the basename of the loader in the aux vector, I think this should work fine) and then set the program into jail mode accordingly.

I implemented this idea and it looks really OK now.
This PR now relies on #24791 to be merged first.

@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from 457fdab to 9f78bf4 Compare August 9, 2024 10:19
@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch 2 times, most recently from 9167430 to 7d2fb8d Compare August 9, 2024 11:34
Copy link

stale bot commented Aug 31, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Aug 31, 2024
@timschumi timschumi added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased and removed stale labels Sep 1, 2024
In particular, try to make it obvious as possible that we don't want to
fail after a certain point (when committing to the new ELF image).

To implement this, there's a new method called commit_exec that can't
fail, which is called after we disable the scope guard of reverting back
to the old memory space.

All allocations are done beforehand, on the do_exec method, so in case
of failure reverting back is easier.
This will be useful later on when we add a symlink to the ELF program
interpreter, thus enabling it to auto-jail the actual program.
Instead of manually running `runc` to set jail-mode on running programs,
we could just create another version of the DynamicLoader which enters
jail-mode just before running the loaded program.
In the previous commit, we added a another version of the DynamicLoader
which enters jail-mode just before running the loaded program.

Then, using a special utility, we can enforce using this dynamic loader
on specific programs on which we desire to run in jail-mode.
@supercomputer7 supercomputer7 force-pushed the containers-add-set-elf-jailed-utility branch from 7d2fb8d to 6973406 Compare September 3, 2024 05:42
@supercomputer7 supercomputer7 added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased labels Sep 3, 2024
@supercomputer7 supercomputer7 marked this pull request as ready for review September 3, 2024 05:43
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Sep 3, 2024
Copy link

stale bot commented Sep 28, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Sep 28, 2024
@supercomputer7
Copy link
Member Author

This is stale and sadly I don't have the time right now to keep it open. I might re-open it sometime in the future, if it seems like an interesting feature to someone in the project :)

@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 2, 2024
@DanShaders
Copy link
Contributor

Off: clearly, you haven't mastered art of removing "stale" label without making any changes for months (ref 24567) :^)

@timschumi
Copy link
Member

timschumi commented Oct 3, 2024

Off: clearly, you haven't mastered art of removing "stale" label without making any changes for months (ref 24567) :^)

Just saying, but that is the textbook definition of stale. Maybe not necessarily 100% for the Coroutine PR, since we also had off-GitHub discussions with a somewhat unclear outcome, but it definitely would be when applied to certain PRs that have been subject to repeated bumps (e.g. 24774).

Removing/invalidating stale labels should be reserved for cases where the PR isn't blocking on anything else but maintainer/reviewer attention. The permission to label issues and PRs isn't a free pass for keeping PRs in the queue when they shouldn't be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants