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

sandbox: Spit out some info when unsharing userns gets EPERM #3266

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

bjackman
Copy link
Contributor

@bjackman bjackman commented Dec 8, 2024

To try and minimise the pain of this issue
(#3265), dump some info that might help users resolve it.

I had a quick look around expecting to find a document from Red Hat discussing this topic much like the Ubuntu one I've linked here, but I didn't find it. Hopefully if it exists someone else can add it later.

mkosi/sandbox.py Outdated Show resolved Hide resolved
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

The check should be done in become_user(), not in unshare(). You can catch the exception there and handle it in the same way.

mkosi/sandbox.py Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated Show resolved Hide resolved
@bjackman
Copy link
Contributor Author

bjackman commented Dec 8, 2024

The check should be done in become_user(), not in unshare(). You can catch the exception there and handle it in the same way.

Ah sorry didn't see this - see my other comment thread: #3266 (comment)

I haven't bothered to comprehend the Ubuntu/AppArmor details here but the high level (which I am already appraised of due due to my day job) is: creating user namespaces itself is harmless. The issue is that then you have CAP_SYS_ADMIN and that gives you access to buggy kernel code in resources that are within other namespaces. So, we only actually need to prevent the next step where you access that buggy code. So I guess Ubuntu is doing something like "you can create the userns, but you can't create the mount/net ns inside it". Ideally it would go more like "you can create the mount ns too, but you can't mount a block filesystem/create a netfilter rule", but AFAIK that's just not really possible with the kernel's namespace/capability infra.

I think there's some risk that on some system this issue might not manifest in unshare at all, but I would say it's better to wait and address that issue if it actually arises.

but you can't mount a block filesystem

(Oh actually I think that specific case might already be true - block fs maintainers are under no illusion that their code is safe against arbitrary fs images!)

@DaanDeMeyer
Copy link
Contributor

What I'm saying is that unshare() is a generic function and shouldn't contain such specific error handling. So please move the error handling to the call of unshare() that actually fails. Whether that's the initial user namespace unsharing of something after that (or both).

@bjackman bjackman force-pushed the userns branch 2 times, most recently from 50937cb to 9524417 Compare December 8, 2024 12:57
@bjackman
Copy link
Contributor Author

bjackman commented Dec 8, 2024

So please move the error handling to the call of unshare() that actually fails.

Sure, done

mkosi/sandbox.py Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated
Comment on lines 820 to 832
try:
unshare(namespaces)
except OSError as e:
if e.errno == EPERM:
print(UNSHARE_EPERM_MSG, file=sys.stderr)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the only one that can fail due to permission issues? Or is this only when apparmor fails? What about if the sysctls are enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed my Ubuntu system has the kernel.unprivileged_userns_clone patch, so I tried that out. And indeed if I set that to 0 the failure happens in become_user. So I've just duplicated the logic there.

I think this could easily happen in a third place too, we probably can't make this foolproof, even if we just do it inside unshare. (E.g. I know of a certain Linux version that "solves" this problem in a third way that is neither kernel.unprivileged_userns_clone nor an LSM policy, I dunno where mkosi would fail on that system, I can't easily test it or share the details either).

mkosi/sandbox.py Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated Show resolved Hide resolved
mkosi/sandbox.py Outdated Show resolved Hide resolved
To try and minimise the pain of this issue
(systemd#3265), dump some info that might help
users resolve it.

I had a quick look around expecting to find a document from Red Hat discussing
this topic much like the Ubuntu one I've linked here, but I didn't find it.
Hopefully if it exists someone else can add it later.

I'm doing this via a direct write to stderr because of the comment at the top of
sandbox.py saying to avoid imports. If this is highly undesirable it looks like
log.log_notice would  be the right choice here (then you don't need the
annoying ANSI codes).
@bjackman
Copy link
Contributor Author

bjackman commented Dec 9, 2024

Been a long time since I did any real work in the GitHub UI, it's got fancy new features (they still haven't fixed isaacs/github#386 though??), LMK if I'm doing anything stupid here.

@behrmann behrmann merged commit b398597 into systemd:main Dec 9, 2024
24 of 36 checks passed
@bjackman bjackman deleted the userns branch December 15, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants