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

Allow tmpfiles to create files in a root under an unprivileged directory #11820

Merged
merged 4 commits into from Mar 4, 2019
Merged

Conversation

dm0-
Copy link
Contributor

@dm0- dm0- commented Feb 25, 2019

I hit a problem with building packages that run tmpfiles to generate the initial package contents in Gentoo, where parent directories are created but not the files themselves. The issue started for copying files at 16ba55a, creating directories at 4c39d89, etc. It turns out to be due to symlink safety checks that prevent running in a root-owned directory under a directory owned by an unprivileged user, which happens to be the case here:

  • /var/tmp/portage/$PACKAGE = user-owned working directory for packaging
  • /var/tmp/portage/$PACKAGE/image = root-owned staging directory with final installed file permissions

This also happens if you try to set up a new disk image manually in a non-root-owned directory. Here is a minimal reproducer of this:

truncate --size=1G hda.img
mke2fs hda.img
sudo mount -o X-mount.mkdir hda.img root
sudo mkdir -p root/etc/tmpfiles.d
echo 'd /a/b/c' | sudo tee root/etc/tmpfiles.d/test.conf
sudo systemd-tmpfiles --root="$PWD/root" --create

Note it prints an error and creates the parents /a/b but not the full /a/b/c.

Here are some changes that attempt to work around it. The only change necessary for the problems I encountered is in path_open_parent_safe, but I've adjusted chase_symlinks elsewhere in tmpfiles so it has consistent behavior. I'm not very familiar with this code, so if this solution turns out to be unusable, I can close the PR and open an issue instead.

@poettering
Copy link
Member

looks OK, but could you add a unit test for the changes to chase_symlinks() for this? The behaviour on that looks right to me, but I'd really like to see some testing applied to tha, since it's not obvious to see.

@dm0-
Copy link
Contributor Author

dm0- commented Feb 26, 2019

I've added some tests, which I think pass correctly. The failed CI checks appear to have had the same errors on other PRs. I've also added an unsigned cast since I saw an error about unsigned comparison in the original change.

@martinpitt martinpitt added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Feb 26, 2019
@martinpitt
Copy link
Contributor

This seems to add an extra / somewhere? Several unit-config tests now fail like this:

======================================================================
FAIL: test_unit_alias_enable (__main__.EnableTests)
no sysv: enable unit with an alias
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.s4e55m/build.AVx/systemd/debian/tests/unit-config", line 205, in test_unit_alias_enable
    system_unit_dir + '/test_enable.service')
AssertionError: '/lib/systemd/system/test_enable.service' != '//lib/systemd/system/test_enable.service'
- /lib/systemd/system/test_enable.service
+ //lib/systemd/system/test_enable.service
? +

@martinpitt
Copy link
Contributor

FTR, that "somewhere" is pkg-config --variable=systemdsystemunitdir systemd, which apparently gets influenced by this PR to now start with two slashes.

@dm0-
Copy link
Contributor Author

dm0- commented Feb 26, 2019

@martinpitt I looked at the CI results for the last merged PR, which was merged with the same error: #11827

Looking at the change in the PR, it seems to be the likely cause.

@martinpitt
Copy link
Contributor

Indeed #11829 has the same error, sorry for the wrong blame. Removing label again.

@martinpitt martinpitt removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Feb 27, 2019
src/basic/fs-util.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

looks good, but one typecast nitpick. please fix, will merge then.

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 28, 2019
When chase_symlinks is given a root path, it is assumed that all
processed symlinks are restricted under that path.  It should not
be necessary to verify components of that prefix path since they
are not relevant to the symlinks.

This change skips unsafe UID transitions in this root prefix, i.e.
it now ignores when an unprivileged user's directory contains a
root-owned directory above the symlink root.
This informs chase_symlinks that symlinks should be treated as if
the path given by --root= is the root of their file system.

With the parent commit, this allows tmpfiles to create files as the
root user under a prefix that may be owned by an unprivileged user.
In particular, this fixes the case where tmpfiles generates initial
files in a staging root directory for packaging under a directory
owned by the unprivileged packager user (e.g. in Gentoo).
This verifies the fix for the issue described in:
#11820
This verifies the fix for the issue described in:
#11820
@poettering
Copy link
Member

hmm, please always add a comment when you push a new version onto an existing PR. for some reasons github doesn't notify us about that in many cases, so we never notice...

@poettering poettering merged commit 46d4d67 into systemd:master Mar 4, 2019
@poettering poettering removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Mar 4, 2019
@dm0- dm0- deleted the chase branch March 4, 2019 12:07
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Sep 23, 2019
This verifies the fix for the issue described in:
systemd/systemd#11820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants