In bug 629380 we have a pretty silly security hole: a daemon in /usr/sbin is owned by a non-root user, so he can replace it to gain root the next time the daemon is started. I'd like to add a QA check for this... there's already a 90world-writable check, so we don't have to worry about that case. What's left? * Files in /sbin or /usr/sbin that are owned by a non-root user. * Symlinks in /sbin or /usr/sbin whose target is owned by a non-root user. * Group-writable files in /sbin or /usr/sbin with a non-root group. Did I miss anything that's safe to warn about? Will these generate false positives on prefix, and if so, how can I skip them there?
Why is sbin special? It seems like it would be just as dangerous for a random user to own files in /bin or /usr/bin.
(In reply to Mike Gilbert from comment #1) > Why is sbin special? Now that I think about it, I guess it isn't.
Created attachment 491116 [details] 91bad-bin-owner Proof of concept, this catches the /usr/sbin/nagios problem but I've no idea what it does on prefix. Still need the group-writable check too.
I think this could be made more generic still. Is it valid for a non-root user to own *any* file installed by a package?
(In reply to Mike Gilbert from comment #4) > I think this could be made more generic still. > > Is it valid for a non-root user to own *any* file installed by a package? In a perfect world, probably not, but check e.g. /var/log.
Created attachment 491120 [details] 92bad-bin-group-write Same caveats apply to this one. Miscellany: * "local" isn't POSIX * it would be faster to pipe "find" through sed * the output should probably be formatted as a list, like the "pre-stripped" warning * spaces instead of tabs........
(In reply to Michael Orlitzky from comment #6) > Created attachment 491120 [details] > 92bad-bin-group-write > > Same caveats apply to this one. > > Miscellany: > > * "local" isn't POSIX Portage extensively uses bash features, and depends on it, so this shouldn't be a problem? > * it would be faster to pipe "find" through sed > * the output should probably be formatted as a list, like the > "pre-stripped" > warning > * spaces instead of tabs........ If you'd replace D with ED in your for-loop you would add Prefix support. Leave D in the strip section.
(In reply to Mike Gilbert from comment #4) > I think this could be made more generic still. > > Is it valid for a non-root user to own *any* file installed by a package? A grep of the tree for "fowners" actually turns up quite a few root exploits, where e.g. an unprivileged user owns the config file for a daemon that starts as root. On the other hand, it also turns up a ton of "because I could" calls to fowners, that I can't really explain but which don't seem to hurt anything. Eventually it might make sense to be more strict here, but I think if we turn on the warnings for every installed file right now, the important ones will be drowned out in the noise. (In reply to Fabian Groffen from comment #7) > > Portage extensively uses bash features, and depends on it, so this shouldn't > be a problem? > So long as these are supposed to be bash, it's not. I wasn't sure. > If you'd replace D with ED in your for-loop you would add Prefix support. > Leave D in the strip section. But will throw a QA warning for every executable on prefix because it's owned by e.g. "mjo" and not root?
Ah, I missed the -group root, I'm not sure if mainline portage has rootgid stored somewhere like prefix branch does.
now I think of it, this breaks FreeBSD too, since root's group is wheel (its id = 0).
(In reply to Fabian Groffen from comment #10) > now I think of it, this breaks FreeBSD too, since root's group is wheel (its > id = 0). Ah, good catch. I'll change it to use -uid instead of -user and likewise for the group. Does using ${D} instead of ${ED} prevent the false positives on prefix? And is there a better way to get the "superuser" uid/gid? I considered pulling the uid and gid off of ${D} itself.
Perhaps Zac knows how to get the root/owner uid and gid. Otherwise, in Prefix this check makes little to no sense, unless run as root. So if UID=0 you may want to run this check, else skip it. The ED in your for-loop makes the paths correct, the sed replacement with D is fine, since that way the messages include EPREFIX, which for clarity is good.
Created attachment 491158 [details] 90bad-bin-owner
Created attachment 491160 [details] 90bad-bin-group-write These should clean things up a bit. I copied the [[ $D == $ED ]] test from another check to bail out on prefix installations.
just for the record, there are prefix installs which are using the root user
(In reply to Fabian Groffen from comment #15) > just for the record, there are prefix installs which are using the root user Ok, I see: we can still get some of the benefit on prefix by checking $UID == 0 rather than $D == $ED. That way the check runs on root prefix installs, at least. What's a safe way to get the current $UID? Can the bash $UID variable be relied-upon in these QA scripts?
(In reply to Michael Orlitzky from comment #16) > What's a safe way to get the current $UID? Can the bash $UID variable be > relied-upon in these QA scripts? Yes, though I would use EUID (effective UID) instead.
Created attachment 492914 [details] 90bad-bin-group-write
Created attachment 492916 [details] 90bad-bin-owner These last two revisions use the EUID == 0 check, and I've updated the output to match the test (mention uid/gid instead of "root").
What's the current fashion -- should I create a patch for portage, or for metadata/install-qa-check.d in ::gentoo?
Ping, this is ready to go.
(In reply to Michael Orlitzky from comment #18) > Created attachment 492914 [details] > 90bad-bin-group-write > > # This check doesn't work on non-root prefix installations at > # the moment, because every executable therein is owned by a > # nonzero GID. > [[ "${EUID}" -eq "0" ]] || return I'd prefer not to make an assumption about EUID, so how about this instead: [[ ${PORTAGE_INST_UID} -eq 0 ]] > local d f found=() > > for d in "${ED}/bin" "${ED}/usr/bin" "${ED}/sbin" "${ED}/usr/sbin"; do > test -d "${d}" || continue Let's normalize all ${ED} and ${D} references for the absence of a trailing slash in EAPI 7, like ${ED%/}/bin. > # Read the results of the "find" command into the "found" bash > # array. Use -L to catch symlinks whose targets are vulnerable. > # We match the GID and not the name "root" here because (for > # example) on FreeBSD, the superuser group is "wheel". > while read -r -d '' f; do > found+=( "${f}" ) > done < <(find -L "${d}" -maxdepth 1 -type f -perm /g+w ! -gid 0 -print0) Let's use -gid ${PORTAGE_INST_GID}. > > if [[ ${found[@]} ]]; then > eqawarn "system executables group-writable by nonzero gid:" > for f in "${found[@]}"; do > # Strip off the leading ${D} before outputting the path, > # but leave the prefix if there is one. > eqawarn " ${f#${D}}" ${D%/}/ > done > fi > done > } > > bad_bin_group_write_check > : (In reply to Michael Orlitzky from comment #19) > Created attachment 492916 [details] > 90bad-bin-owner > > while read -r -d '' f; do > found+=( "${f}" ) > done < <(find -L "${d}" -maxdepth 1 -type f ! -uid 0 -print0) Let's use -uid ${PORTAGE_INST_UID}.
Created attachment 540992 [details] 90bad-bin-owner
Created attachment 540994 [details] 90bad-bin-group-write Changes: * Update copyright year to 2018. * Normalize trailing slashes in D and ED. * Mention that "find -L ..." won't catch absolute symlinks on a first install. New problems: * The output is now missing the trailing slash, e.g. "usr/bin/asd" is listed instead of "/usr/bin/asd". This isn't a big deal, and could probably be fixed by stripping the whole prefix and then putting it back, normalized. Questions / comments: The choice to skip these checks when EUID != 0 was made for two reasons. First, we can't use something like PORTAGE_INST_UID if we want these to go in metadata/install-qa-check.d (see my comment #20). But more importantly, we can get false positives if we check for PORTAGE_INST_UID instead of uid=0. For example, on a system where PORTAGE_INST_UID=100, we don't want to spit out warnings for a symlink pointing to a root:root binary. So my feeling is that these checks should "do no harm" and get skipped on weird systems -- we only need bug reports from *someone*, not *everyone*. From that perspective, bailing out when EUID != 0 is OK.
(In reply to Michael Orlitzky from comment #24) > Questions / comments: > > The choice to skip these checks when EUID != 0 was made for two reasons. > First, we can't use something like PORTAGE_INST_UID if we want these to go > in metadata/install-qa-check.d (see my comment #20). But more importantly, > we can get false positives if we check for PORTAGE_INST_UID instead of > uid=0. For example, on a system where PORTAGE_INST_UID=100, we don't want to > spit out warnings for a symlink pointing to a root:root binary. Now I'm wondering what the symlink check is good for, is it going to report files owned by *other* packages? Wouldn't that be a false positive for the package being checked? > So my feeling is that these checks should "do no harm" and get skipped on > weird systems -- we only need bug reports from *someone*, not *everyone*. > From that perspective, bailing out when EUID != 0 is OK. I'm more concerned about the possibility of it running when it's not supposed to, and I want the code to clearly indicate when it's not supposed to run, so how about this: [[ ${EUID} -eq 0 || ${PORTAGE_INST_UID} -eq 0 ]] || return
(In reply to Zac Medico from comment #25) > I'm more concerned about the possibility of it running when it's not > supposed to, and I want the code to clearly indicate when it's not supposed > to run, so how about this: > > [[ ${EUID} -eq 0 || ${PORTAGE_INST_UID} -eq 0 ]] || return I mean like this: [[ ${EUID} -ne 0 || ${PORTAGE_INST_UID} -ne 0 ]] && return
(In reply to Zac Medico from comment #25) > Now I'm wondering what the symlink check is good for, is it going to report > files owned by *other* packages? Wouldn't that be a false positive for the > package being checked? I see, the current package would be at fault for including the vulnerable file in PATH via the symlink.
(In reply to Zac Medico from comment #26) > > I mean like this: > > [[ ${EUID} -ne 0 || ${PORTAGE_INST_UID} -ne 0 ]] && return I'm fine with this change... does that mean I should be preparing patches for portage.git and not metadata/install-qa-check.d, though? (In reply to Zac Medico from comment #27) > > I see, the current package would be at fault for including the vulnerable > file in PATH via the symlink. Right... the check is generally useful and catches real mistakes (for example, dev-db/aerospike-server-community).
(In reply to Michael Orlitzky from comment #28) > (In reply to Zac Medico from comment #26) > > > > I mean like this: > > > > [[ ${EUID} -ne 0 || ${PORTAGE_INST_UID} -ne 0 ]] && return > > I'm fine with this change... does that mean I should be preparing patches > for portage.git and not metadata/install-qa-check.d, though? Yes, that would be very helpful. > (In reply to Zac Medico from comment #27) > > > > I see, the current package would be at fault for including the vulnerable > > file in PATH via the symlink. > > Right... the check is generally useful and catches real mistakes (for > example, dev-db/aerospike-server-community). Great!
Created attachment 541712 [details, diff] 0001-bin-install-qa-check.d-add-new-90bad-bin-owner-QA-ch.patch
Created attachment 541714 [details, diff] 0002-bin-install-qa-check.d-add-new-90bad-bin-group-write.patch
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=807ac3d9d6eecead73f59d399b30559e5c731587 commit 807ac3d9d6eecead73f59d399b30559e5c731587 Author: Michael Orlitzky <mjo@gentoo.org> AuthorDate: 2018-08-07 16:46:04 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2018-08-07 18:39:26 +0000 bin/install-qa-check.d: add new 90bad-bin-group-write QA check. System executables that are writable by a non-root user pose a security risk. Anyone who can write to an executable can change its behavior. If that executable is later run with elevated privileges (say, by root, when the machine starts), then the non-root user can escalate his own privileges to those of the person running the modified executable. The 90bad-bin-owner check already addresses one cause for a non-root user to be able to modify an executable: because he owns it. This commit adds another check, to ensure that no non-root *groups* have write access to any system executables. On a "normal" system, all system executables should be writable only by the super-user's group, if any. To avoid false-positives, non-"normal" systems (like prefix) are skipped. Closes: https://bugs.gentoo.org/629398 bin/install-qa-check.d/90bad-bin-group-write | 55 ++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) Additionally, it has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=04e71a831bc42f2a0de1694dd2013eac0414e007 commit 04e71a831bc42f2a0de1694dd2013eac0414e007 Author: Michael Orlitzky <mjo@gentoo.org> AuthorDate: 2018-08-07 16:46:03 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2018-08-07 18:39:26 +0000 bin/install-qa-check.d: add new 90bad-bin-owner QA check. System executables that are not owned by root pose a security risk. The owner of the executable is free to modify it at any time; so, for example, he can change a daemon's behavior to make it malicious before the next time the service is started (usually by root). On a "normal" system, the superuser should own every system executable (even setuid ones, for security reasons). This commit adds a new install-time check that reports any such binaries with a QA warning. To avoid false positives, non-"normal" systems (like prefix) are skipped at the moment. Bug: https://bugs.gentoo.org/629398 bin/install-qa-check.d/90bad-bin-owner | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
