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

Private /tmp directory for scriptlets #2617

Closed

Conversation

jsegitz
Copy link

@jsegitz jsegitz commented Aug 16, 2023

Ensure a private /tmp directory is used for each scriptlet that is run during installation. Sometimes packagers place vulnerable code in these snippets that operates naively on /tmp and allows for escalation of privileges for local users.

This is intended as POC/RFC. We monitor the %post etc. scriptlets added by our maintainers and regularly identify issues with those. Not all will be fixed with this (everything that operates out of /tmp can still cause issues), but the hope is to at least prevent issues in /tmp once and for all.

We applied this patch to openSUSE Tumbleweed, build the distribution with it and ran several openQA test suites on this (e.g. https://openqa.opensuse.org/tests/3507153) without seeing any issues. But I assume that getting this upstream might require changes due to reasons outlines in the contributing guidelines, but I hope we can enable this by default

I assume that there will need to be a way to opt out of this via a cmd argument, but didn't implement this yet.

Ensure a private /tmp directory is used for each scriptlet that is
run during installation. Sometimes packagers place vulnerable code
in these snippets that operates naively on /tmp and allows for
escalation of privileges for local users
@jsegitz
Copy link
Author

jsegitz commented Aug 16, 2023

I'll have a look at the CI error, didn't see that locally

@pmatilai
Copy link
Member

pmatilai commented Aug 25, 2023

I'm not at all opposed to adding Linux-specific enhancements where they make sense, but we can't rely on them unconditionally. So at the very least this will need conditionalizing on the availability of the relevant unshare() etc facilities, but maybe also a way to disable at runtime even when built in. The need for one may not be obvious now, but pretty much every time I've added I've added such a feature without an explicit disabler, I've regretted it later upon encountering some unexpected situation (be it bug or something else) 😅

Another, stylistic nit is that the rpm codebase doesn't use bool and doing so in this one place only looks just odd, just use a plain old int instead.

@pmatilai
Copy link
Member

pmatilai commented Aug 25, 2023

As for the disabler, just remembered there's already --nouserns which you can use for this too (look for _rpm_nouserns in the code)

pmatilai

This comment was marked as duplicate.

Copy link
Member

@pmatilai pmatilai left a comment

Choose a reason for hiding this comment

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

See above comments.

@pmatilai
Copy link
Member

On a related note - kinda inspired by this: #2632

@pmatilai
Copy link
Member

pmatilai commented Aug 29, 2023

Looked at this a little closer. It's doing the unshare() in the main rpm process, which may not effect the transaction-in-progress too much, but it'll potentially break any future transactions done by the same process by detaching the mount table. That's a no-go.

This needs to be done post fork() in the scriptlet code, but the caveat is that it'll then miss out on Lua scriptlets. I could swear we have a ticket on making Lua scriptlets forked too because the current in-process thing is problematic in an increasing number of ways, and here's one more.

@pmatilai
Copy link
Member

Superseded by #2666 which makes this configurable and adds more functionality.

Thanks for the initiative and the patch!

@pmatilai pmatilai closed this Sep 15, 2023
@jsegitz
Copy link
Author

jsegitz commented Sep 18, 2023

thank you very much! I'll follow the new issue

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