[gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michael Orlitzky
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, there is no good reason why the superuser should
not own every system executable. 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 | 38 ++
 1 file changed, 38 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..188d67a51
--- /dev/null
+++ b/bin/install-qa-check.d/90bad-bin-owner
@@ -0,0 +1,38 @@
+# 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,
+   # or /usr/sbin) 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%/}/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).
+   while read -r -d '' f; do
+   found+=( "${f}" )
+   done < <(find -L "${d}" -maxdepth 1 -type f ! -uid 0 -print0)
+
+   if [[ ${found[@]} ]]; then
+   eqawarn "system executables owned by nonzero uid:"
+   for f in "${found[@]}"; do
+   # Strip off the leading destdir before 
outputting the path,
+   # but leave the prefix if there is one.
+   eqawarn "  ${f#${D%/}/}"
+   done
+   fi
+   done
+}
+
+bad_bin_owner_check
+:
-- 
2.16.4




[gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michael Orlitzky
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, there is no good reason why the superuser should
not own every system executable. 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 | 47 ++
 1 file changed, 47 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..748e1dc99
--- /dev/null
+++ b/bin/install-qa-check.d/90bad-bin-owner
@@ -0,0 +1,47 @@
+# 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,
+   # but leave the prefix if there is one.
+   eqawarn "  ${f#${D%/}/}"
+   done
+   fi
+}
+
+bad_bin_owner_check
+:
-- 
2.16.4




Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michał Górny
W dniu nie, 29.07.2018 o godzinie 13∶37 -0400, użytkownik Michael
Orlitzky napisał:
> 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, there is no good reason why the superuser should
> not own every system executable. 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 | 38 
> ++
>  1 file changed, 38 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..188d67a51
> --- /dev/null
> +++ b/bin/install-qa-check.d/90bad-bin-owner
> @@ -0,0 +1,38 @@
> +# 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,
> + # or /usr/sbin) 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%/}/bin" "${ED%/}/usr/bin" "${ED%/}/sbin" 
> "${ED%/}/usr/sbin"; do

I think you should include /opt/bin as well.  Or maybe simply all
locations on ${PATH}.

> + [[ -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).
> + while read -r -d '' f; do
> + found+=( "${f}" )
> + done < <(find -L "${d}" -maxdepth 1 -type f ! -uid 0 -print0)
> +
> + if [[ ${found[@]} ]]; then
> + eqawarn "system executables owned by nonzero uid:"
> + for f in "${found[@]}"; do
> + # Strip off the leading destdir before 
> outputting the path,
> + # but leave the prefix if there is one.
> + eqawarn "  ${f#${D%/}/}"
> + done
> + fi
> + done
> +}
> +
> +bad_bin_owner_check
> +:

-- 
Best regards,
Michał Górny


signature.asc
Description: This is a digitally signed message part


Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Ulrich Mueller
> On Sun, 29 Jul 2018, Michael Orlitzky wrote:

> 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, there is no good reason why the superuser should
> not own every system executable. 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.

Shouldn't this check for setuid binaries like /usr/bin/mandb (which is
owned by man:man)? I think these are legitimate usage case.

Ulrich


pgptqtrJo8E1U.pgp
Description: PGP signature


Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michael Orlitzky
On 07/29/2018 03:43 PM, Ulrich Mueller wrote:
> 
> Shouldn't this check for setuid binaries like /usr/bin/mandb (which is
> owned by man:man)? I think these are legitimate usage case.
> 

In general, yeah. I think we should be skeptical of setuid/gid
executables, but this isn't the right place to make that stand.

In this specific case, though, I don't see why that program is setuid.
In fact, I'm pretty sure it lets the "man" user gain root privileges.



Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michael Orlitzky
On 07/29/2018 03:43 PM, Ulrich Mueller wrote:
> 
>> On a "normal" system, there is no good reason why the superuser should
>> not own every system executable. 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.
> 
> Shouldn't this check for setuid binaries like /usr/bin/mandb (which is
> owned by man:man)? I think these are legitimate usage case.
> 

After thinking about this for a while, I think we should ignore setgid
but not setuid executables. The problem with setuid and a non-root owner
is that the owner can always exploit the situation:

Suppose /bin/foo is owned by "foo" and setuid. If root (or any other
privileged user) is about to run /bin/foo, then the "foo" user can
simply strip away the setuid bit and fill /bin/foo with malicious code.

The same situation with setgid is safe because (as far as I know)
members of the group can't strip off the setgid bit.



Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Ulrich Mueller
> On Sun, 29 Jul 2018, Michael Orlitzky wrote:

> After thinking about this for a while, I think we should ignore setgid
> but not setuid executables. The problem with setuid and a non-root owner
> is that the owner can always exploit the situation:

> Suppose /bin/foo is owned by "foo" and setuid. If root (or any other
> privileged user) is about to run /bin/foo, then the "foo" user can
> simply strip away the setuid bit and fill /bin/foo with malicious code.

Staying with the man:man example, how would anybody become the "man"
user, in the first place? That user has /bin/false as a shell and no
valid password.

> The same situation with setgid is safe because (as far as I know)
> members of the group can't strip off the setgid bit.

Setgid executables shouldn't be group writable, so I believe that part
of the test is fine as-is in v1 of your patch.

Ulrich


pgppMBCz_RDB1.pgp
Description: PGP signature


Re: [gentoo-portage-dev] [PATCH 1/2] bin/install-qa-check.d: add new 90bad-bin-owner QA check.

2018-07-29 Thread Michael Orlitzky
On 07/29/2018 09:16 PM, Ulrich Mueller wrote:
> 
> Staying with the man:man example, how would anybody become the "man"
> user, in the first place? That user has /bin/false as a shell and no
> valid password.

One way would be to exploit a process that's running as the "man" user.
Ostensibly such a thing is possible; otherwise we wouldn't be dropping
privileges in the first place.

The shell/password settings for that user are also in no way guaranteed.

But on principle: who cares, non-root users shouldn't be able to gain
root =)


> Setgid executables shouldn't be group writable

Why not? I'm happy to pull that part back out.