8000 fix_stdio_permissions: only change owner/permissions on ttys by terceiro · Pull Request #4486 · lxc/lxc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix_stdio_permissions: only change owner/permissions on ttys #4486

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix_stdio_permissions: only change owner/permissions on ttys #4486

wants to merge 1 commit into from

Conversation

terceiro
Copy link
Contributor

This avoids changing permissions on files passed in as shell redirections, e.g. lxc-attach [...] < FILE.

Fixes: #4484

@terceiro
fix_stdio_permissions: only change owner/permissions on ttys
3aec161 
This avoids changing permissions on files passed in as shell
redirections, e.g. `lxc-attach [...] < FILE`.

Fixes: lxc#4484
Signed-off-by: Antonio Terceiro <terceiro@debian.org>
@lxc-jenkins
Copy link

This pull request didn't trigger Jenkins as its author isn't in the allow list.

An organization member must perform one of the following:

  • To have this branch tested by Jenkins, use the "ok to test" command.
  • To have a one time test done, use the "t 8000 est this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

@mihalicyn
Copy link
Member

jenkins: ok to test

@lxc-jenkins
Copy link

Testsuite passed

@mihalicyn
Copy link
Member
mihalicyn commented Sep 27, 2024 •
edited
Loading

Hi @terceiro,

thanks for your patch!

I'm trying to figure out a story behind the original change #3344.
It seems to me that your patch is, effectively, disabling a behavior introduced in #3344,
because you add a isatty check which means that permissions/owner will be changed only for a terminal devices.

Unfortunately, there is no reproducer or any additional info in #3344 to figure out what was the use case. At the same time,
it seems a bit weird to me that we are changing an ownership/permission information on the file passed as a stdin/stdout/stderr,
because usually, Linux kernel does all the permission checks during open() syscall and if container's init process has an FD
then it should be fine to perform read()/write() on it. Which means that the original change is not really needed?..

Let's ask Christian's (@brauner) opinion on this story in general.

@brauner
Copy link
Member
brauner commented Sep 27, 2024

fix_stdio_permissions() is only called when the namespace uid/gid that is used to setup the container is different from the namespace uid/gid the container actually runs as. Usually both will be namespace uid/gid 0 mapped to some value on the host. But there are cases where the container workload switches to a non-zero namespace uid/gid. For that case we adjust the permissions on stdio/stdout/stderr to ensure that the non-zero namespace uid/gid can open them.

@brauner
Copy link
Member
brauner commented Sep 27, 2024

From the reproducer @terceiro pasted it looks like the lxc-attach uid/gid isn't different from the namesapce setuid uid/gid. So that would be a bug in attach.c somewhere.

@hallyn
Copy link
Member
hallyn commented Dec 31, 2024

From the reproducer @terceiro pasted it looks like the lxc-attach uid/gid isn't different from the namesapce setuid uid/gid. So that would be a bug in attach.c somewhere.

A bit of debug is giving me:

after alloc_attach_context ctx->setup_ns_uid = -1 ctx->target_ns_uid = -1
after get_attach_context ctx->setup_ns_uid = -1 ctx->target_ns_uid = 0

Doh! It just occurred to me I was wrong in my previous comment: my 4.0.12 test was with
a root owned container that is uid mapped. My 6.0.1 test was without a user namespace.

@hallyn
Copy link
Member
hallyn commented Dec 31, 2024

("previous comment" being the one in the related issue)

@hallyn
Copy link
Member
hallyn commented Dec 31, 2024

So on the one hand, if root in the container is root on the host, then we shouldn't need to chown for the container to be able to read/write the contents. But, the container might have sensitive info which through some convoluted manipulation of a system using lxc, allow an unprivileged user to arrange to exfiltrate data.

My biggest worry with what we're currently doing would be if it were possible for a container to create a root-owned executable file, but we prevent that by chmoding (unless the chmod can fail, in which case perhaps we should work harder to undo our chown?). So finally there could be the inconvenience for an unprivileged sudo-capable user who gets their file owned by root this way.

So right now my recommendation would be to not merge this pr, and close the issue as wontfix.

@mihalicyn mihalicyn mentioned this pull request Mar 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihalicyn mihalicyn Awaiting requested review from mihalicyn

@brauner brauner Awaiting requested review from brauner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lxc-attach changes permissions on file passed as standard input
5 participants
@terceiro @lxc-jenkins @mihalicyn @brauner @hallyn
0