Discussion:
[gentoo-portage-dev] [PATCH v3 0/2] Two insecure ownership and group-writability QA checks.​
Michael Orlitzky
2018-08-07 16:46:02 UTC
Permalink
Changes in v3:

* Undo the setguid exception from v2, and add a comment explaining why.
* Add line breaks for readability in two comments.
* Try to put back the leading "/" in the output list.
* Remove a superfluous comment mentioning the "prefix."

Michael Orlitzky (2):
bin/install-qa-check.d: add new 90bad-bin-owner QA check.
bin/install-qa-check.d: add new 90bad-bin-group-write QA check.

bin/install-qa-check.d/90bad-bin-group-write | 55 ++++++++++++++++++++++++++++
bin/install-qa-check.d/90bad-bin-owner | 48 ++++++++++++++++++++++++
2 files changed, 103 insertions(+)
create mode 100644 bin/install-qa-check.d/90bad-bin-group-write
create mode 100644 bin/install-qa-check.d/90bad-bin-owner
--
2.16.4
Michael Orlitzky
2018-08-07 16:46:03 UTC
Permalink
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(+)
create mode 100644 bin/install-qa-check.d/90bad-bin-owner

diff --git a/bin/install-qa-check.d/90bad-bin-owner b/bin/install-qa-check.d/90bad-bin-owner
new file mode 100644
index 000000000..c3ee30746
--- /dev/null
+++ b/bin/install-qa-check.d/90bad-bin-owner
@@ -0,0 +1,48 @@
+# Copyright 1999-2018 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+bad_bin_owner_check() {
+ # Warn about globally-installed executables (in /bin, /usr/bin, /sbin,
+ # /usr/sbin, or /opt/bin) that are owned by a nonzero UID.
+
+ # This check doesn't work on non-root prefix installations at
+ # the moment, because every executable therein is owned by a
+ # nonzero UID.
+ [[ "${EUID}" -ne "0" || "${PORTAGE_INST_UID}" -ne "0" ]] && return
+
+ local d f found=()
+
+ for d in "${ED%/}/opt/bin" "${ED%/}/bin" "${ED%/}/usr/bin" \
+ "${ED%/}/sbin" "${ED%/}/usr/sbin"; do
+ [[ -d "${d}" ]] || continue
+
+ # Read the results of the "find" command into the "found" bash array.
+ #
+ # Use -L to catch symlinks whose targets are owned by a non-root user,
+ # even though it won't catch ABSOLUTE symlinks until the package
+ # is RE-installed (the first time around, the target won't exist).
+ #
+ # We do want to list non-superuser setuid executables, because
+ # they can be exploited. The owner can simply wipe the setuid
+ # bit, and then alter the contents of the file. The superuser
+ # will then have a time bomb in his $PATH.
+ while read -r -d '' f; do
+ found+=( "${f}" )
+ done < <(find -L "${d}" \
+ -maxdepth 1 \
+ -type f \
+ ! -uid 0 \
+ -print0)
+ done
+
+ if [[ ${found[@]} ]]; then
+ eqawarn "system executables owned by nonzero uid:"
+ for f in "${found[@]}"; do
+ # Strip off the leading destdir before outputting the path.
+ eqawarn " ${f#${D%/}}"
+ done
+ fi
+}
+
+bad_bin_owner_check
+:
--
2.16.4
Zac Medico
2018-08-07 17:34:06 UTC
Permalink
Post by Michael Orlitzky
+ eqawarn "system executables owned by nonzero uid:"
+ # Strip off the leading destdir before outputting the path.
+ eqawarn " ${f#${D%/}}"
Why not use ${ED%/} instead of ${D%/} here, so that the output is the
same regardless of ${EPREFIX}?
--
Thanks,
Zac
Michael Orlitzky
2018-08-07 17:44:01 UTC
Permalink
Post by Zac Medico
Why not use ${ED%/} instead of ${D%/} here, so that the output is the
same regardless of ${EPREFIX}?
We want to show where the executable was actually installed, and
generally that includes EPREFIX. For example, I'd want to see

/var/tmp/whatever/.../root/prefix/usr/bin/foo

reported as

/root/prefix/usr/bin/foo

rather than

/usr/bin/foo

Of course, these checks are now skipped on prefix systems anyway, so
it's a bit of a moot point. But I think it's more future-proof to strip
only the $D.
Zac Medico
2018-08-07 18:52:13 UTC
Permalink
Post by Michael Orlitzky
Post by Zac Medico
Why not use ${ED%/} instead of ${D%/} here, so that the output is the
same regardless of ${EPREFIX}?
We want to show where the executable was actually installed, and
generally that includes EPREFIX. For example, I'd want to see
/var/tmp/whatever/.../root/prefix/usr/bin/foo
reported as
/root/prefix/usr/bin/foo
rather than
/usr/bin/foo
Of course, these checks are now skipped on prefix systems anyway, so
it's a bit of a moot point. But I think it's more future-proof to strip
only the $D.
Sounds good. Thanks! Merged:

https://gitweb.gentoo.org/proj/portage.git/commit/?id=807ac3d9d6eecead73f59d399b30559e5c731587
--
Thanks,
Zac
Michael Orlitzky
2018-08-07 16:46:04 UTC
Permalink
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(+)
create mode 100644 bin/install-qa-check.d/90bad-bin-group-write

diff --git a/bin/install-qa-check.d/90bad-bin-group-write b/bin/install-qa-check.d/90bad-bin-group-write
new file mode 100644
index 000000000..786dde712
--- /dev/null
+++ b/bin/install-qa-check.d/90bad-bin-group-write
@@ -0,0 +1,55 @@
+# Copyright 1999-2018 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+bad_bin_group_write_check() {
+ # Warn about globally-installed executables (in /bin, /usr/bin, /sbin,
+ # /usr/sbin, or /opt/bin) that are group-writable by a nonzero GID.
+
+ # This check doesn't work on non-root prefix installations at
+ # the moment, because every executable therein is owned by a
+ # nonzero GID.
+ [[ "${EUID}" -ne "0" || "${PORTAGE_INST_UID}" -ne "0" ]] && return
+
+ local d f found=()
+
+ for d in "${ED%/}/opt/bin" "${ED%/}/bin" "${ED%/}/usr/bin" \
+ "${ED%/}/sbin" "${ED%/}/usr/sbin"; do
+ [[ -d "${d}" ]] || continue
+
+ # Read the results of the "find" command into the "found" array.
+ #
+ # Use -L to catch symlinks whose targets are vulnerable,
+ # even though it won't catch ABSOLUTE symlinks until the package
+ # is RE-installed (the first time around, the target won't exist).
+ #
+ # We match the GID and not the name "root" here because (for
+ # example) on FreeBSD, the superuser group is "wheel".
+ #
+ # We don't make an exception for setguid executables here, because
+ # a group-writable setguid executable is likely a mistake. By
+ # altering the contents of the executable, a member of the group
+ # can allow everyone (i.e. the people running it) to obtain the
+ # full privileges available to that group. While only existing
+ # group members can make that choice, it's a decision usually
+ # limited to the system administrator.
+ while read -r -d '' f; do
+ found+=( "${f}" )
+ done < <(find -L "${d}" \
+ -maxdepth 1 \
+ -type f \
+ -perm /g+w \
+ ! -gid 0 \
+ -print0)
+ done
+
+ if [[ ${found[@]} ]]; then
+ eqawarn "system executables group-writable by nonzero gid:"
+ for f in "${found[@]}"; do
+ # Strip off the leading destdir before outputting the path.
+ eqawarn " ${f#${D%/}}"
+ done
+ fi
+}
+
+bad_bin_group_write_check
+:
--
2.16.4
M. J. Everitt
2018-08-07 20:56:30 UTC
Permalink
Post by Michael Orlitzky
* Undo the setguid exception from v2, and add a comment explaining why.
* Add line breaks for readability in two comments.
* Try to put back the leading "/" in the output list.
* Remove a superfluous comment mentioning the "prefix."
bin/install-qa-check.d: add new 90bad-bin-owner QA check.
bin/install-qa-check.d: add new 90bad-bin-group-write QA check.
bin/install-qa-check.d/90bad-bin-group-write | 55 ++++++++++++++++++++++++++++
bin/install-qa-check.d/90bad-bin-owner | 48 ++++++++++++++++++++++++
2 files changed, 103 insertions(+)
create mode 100644 bin/install-qa-check.d/90bad-bin-group-write
create mode 100644 bin/install-qa-check.d/90bad-bin-owner
May I just briefly complement you on your patch mail format ..

It's much easier to see, when you make updates to a patchset, that you
have added a summary in your 0/x mail for what changed from the previous
iteration.

I don't think git has an easy way to both describe the patch diffs from
the original *and* the changes from last iteration to current - that
really would be the cherry on the icing on the cake! (something perhaps
the avid perl users may be able to graft together!).

Keep up the good work :)
Best regards,

Michael.

Loading...