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

nsenter: check cmdline for init argument #4342

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 10, 2024

Fixes the failure when _LIBCONTAINER_INITPIPE is present in runc environment.

Fixes: #4340.

Alternative to #4339.

@kolyshkin kolyshkin changed the title runc init: check cmdline for init argument libct/nsenter: check cmdline for init argument Jul 10, 2024
@kolyshkin kolyshkin changed the title libct/nsenter: check cmdline for init argument nsenter: check cmdline for init argument Jul 10, 2024
Fixes the failure when _LIBCONTAINER_INITPIPE is present in runc
environment.

Fixes: 4340.

Signed-off-by: Kir Kolyshkin <[email protected]>
@cyphar
Copy link
Member

cyphar commented Jul 10, 2024

Yeah, this is better than resurrecting the fetch_argv stuff.

@kolyshkin kolyshkin marked this pull request as ready for review July 10, 2024 22:43
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

Is there a reason we care about this? I'd like to know, as maybe some suggestions don't improve that particular use case.

The cmdline parsing, if we can expect runc init to have more params in some setups, can mean to re-implement the parsing in C (up to some point). So it doesn't seem ideal if more params can be expected. Can they?

Another option would be to get the parent pid and check it is runc. But parsing /proc/self/status, get the ppid, check if the parent is runc in C is not nice. And... how to check it is runc? a md5sum of /proc/ppid/exec is expensive, the comm name can be fake also, so in the end it seems like the same problem we have today.

Another option might be to have an open fd on the parent, that we inherit when forking and checking that fd is open?

EDIT: cyphar said it doesn't take any comand-line options. So this PR seems like a good fix for me IF we want to fix this. Sorry for the noise :)

libcontainer/nsenter/nsexec.c Show resolved Hide resolved
@cyphar
Copy link
Member

cyphar commented Jul 11, 2024

I'm also not really convinced this is worth fixing either. Ideally, we would only run the C code for runc init, so doing this is nicer from a correctness perspective but if it's too much effort to get right then it's not really a big enough issue imho.

That being said, runc init doesn't take arguments, so this patch is fine as-is IMHO.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jul 11, 2024

In theory, we can also switch to passing fds and other stuff via cmdline, not sure what are the pros and cons of doing that vs passing via environment. Probably not worth changing.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jul 11, 2024

I'm not sure either if we should fix this.

Arguments for:

  • it is a bug;
  • the fix does not have any noticeable performance impact AFAICS.

Arguments against:

  • no one ever hit this bug in practice; from what I understand @ningmingxiao found it by reading the source code;
  • the fix depends on /proc being mounted and accessible (but runc won't work without /proc anyway).

To me, both pros and contras are rather weak.

@cyphar
Copy link
Member

cyphar commented Jul 12, 2024

I think moving runc init config passing to argument doesn't make a lot of sense, there's no real downside to the current setup. Though I'm leaning towards "maybe we should merge this since it's not a big deal but it is 'more correct'".

the fix depends on /proc being mounted and accessible (but runc won't work without /proc anyway).

Somewhat related: We can fix this using fsopen in a way that will protect us against malicious /proc mounts (I just added some code to filepath-securejoin to do this). libpathrs will have library functions to make this easy, so we would be able to use this in runc soon-ish.

@kolyshkin kolyshkin closed this Aug 30, 2024
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.

runc fails if run with _LIBCONTAINER_INITPIPE env var set
3 participants