Re: [gentoo-portage-dev] [PATCH v3 0/2] Two insecure ownership and group-writability QA checks.
On 07/08/18 17:46, Michael Orlitzky wrote: > 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 > 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. signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v3 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.
On 08/07/2018 10:44 AM, Michael Orlitzky wrote:
> On 08/07/2018 01:34 PM, Zac Medico wrote:
>>
>> 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
signature.asc
Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v3 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.
On 08/07/2018 01:34 PM, Zac Medico wrote:
>
> 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.
Re: [gentoo-portage-dev] [PATCH v3 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.
On 08/07/2018 09:46 AM, Michael Orlitzky wrote:
> + 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%/}}"
Why not use ${ED%/} instead of ${D%/} here, so that the output is the
same regardless of ${EPREFIX}?
--
Thanks,
Zac
signature.asc
Description: OpenPGP digital signature
[gentoo-portage-dev] [PATCH v3 2/2] 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(+)
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 0..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
[gentoo-portage-dev] [PATCH v3 1/2] 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(+)
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 0..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
[gentoo-portage-dev] [PATCH v3 0/2] Two insecure ownership and group-writability QA checks.
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
