[PATCH 0/3] Add checks for "host user contamination"

[PATCH 0/3] Add checks for "host user contamination"

[Christopher Larson]

From: Christopher Larson <chris_larson@mentor.com>

This adds a QA test, as well as a rootfs_ function for use in
ROOTFS_POSTPROCESS_COMMAND, to warn/error when paths are owned by the same
user/group as the user running bitbake. This is useful to catch stuff which is
written outside of pseudo's control, for example.

The following changes since commit c38acd720b3f6ffbeb544063692eb471dada8593:

  binconfig-disabled: write an message to stderr to help confused developers (2015-08-19 17:57:58 +0100)

are available in the git repository at:

  git://github.com/kergoth/openembedded-core host-user-contaminated
  https://github.com/kergoth/openembedded-core/tree/host-user-contaminated

Christopher Larson (3):
  insane.bbclass: handle tests which need fakeroot
  insane.bbclass: add host-user-contaminated test
  image.bbclass: add rootfs_check_host_user_contaminated

 meta/classes/image.bbclass  | 14 ++++++++++++++
 meta/classes/insane.bbclass | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

-- 
2.2.1

[PATCH 1/3] insane.bbclass: handle tests which need fakeroot

[Christopher Larson]

From: Christopher Larson <chris_larson@mentor.com>

If any tests listed in FAKEROOT_QA are enabled (listed in ALL_QA), then
run do_package_qa under fakeroot.

Signed-off-by: Christopher Larson <chris_larson@mentor.com>
---
 meta/classes/insane.bbclass | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 9c05c86..cd773b7 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -38,6 +38,9 @@ ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             split-strip packages-list pkgv-undefined var-undefined \
             version-going-backwards expanded-d \
             "
+FAKEROOT_QA = ""
+FAKEROOT_QA[doc] = "QA tests which need to run under fakeroot. If any \
+enabled tests are listed here, the do_package_qa task will run under fakeroot."
 
 ALL_QA = "${WARN_QA} ${ERROR_QA}"
 
@@ -1210,6 +1213,11 @@ python () {
         for var in 'RDEPENDS', 'RRECOMMENDS', 'RSUGGESTS', 'RCONFLICTS', 'RPROVIDES', 'RREPLACES', 'FILES', 'pkg_preinst', 'pkg_postinst', 'pkg_prerm', 'pkg_postrm', 'ALLOW_EMPTY':
             if d.getVar(var, False):
                 issues.append(var)
+
+        fakeroot_tests = d.getVar('FAKEROOT_QA', True).split()
+        if set(tests) & set(fakeroot_tests):
+            d.setVarFlag('do_package_qa', 'fakeroot', '1')
+            d.appendVarFlag('do_package_qa', 'depends', ' virtual/fakeroot-native:do_populate_sysroot')
     else:
         d.setVarFlag('do_package_qa', 'rdeptask', '')
     for i in issues:
-- 
2.2.1

[PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Christopher Larson]

From: Christopher Larson <chris_larson@mentor.com>

- Add a test which checks for any paths outside of /home which are owned by
  the user running bitbake.
- Add the test to WARN_QA by default.

This test has been in meta-mentor for some time, and in our ERROR_QA for our
builds, and has caught a number of issues for us.

Signed-off-by: Christopher Larson <chris_larson@mentor.com>
---
 meta/classes/insane.bbclass | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index cd773b7..aec9800 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -31,14 +31,14 @@ WARN_QA ?= "ldflags useless-rpaths rpaths staticdev libdir xorg-driver-abi \
             installed-vs-shipped compile-host-path install-host-path \
             pn-overrides infodir build-deps file-rdeps \
             unknown-configure-option symlink-to-sysroot multilib \
-            invalid-pkgconfig \
+            invalid-pkgconfig host-user-contaminated \
             "
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
             split-strip packages-list pkgv-undefined var-undefined \
             version-going-backwards expanded-d \
             "
-FAKEROOT_QA = ""
+FAKEROOT_QA = "host-user-contaminated"
 FAKEROOT_QA[doc] = "QA tests which need to run under fakeroot. If any \
 enabled tests are listed here, the do_package_qa task will run under fakeroot."
 
@@ -950,6 +950,34 @@ def package_qa_check_expanded_d(path,name,d,elf,messages):
                         sane = False
     return sane
 
+HOST_USER_UID := "${@os.getuid()}"
+HOST_USER_GID := "${@os.getgid()}"
+
+QAPATHTEST[host-user-contaminated] = "package_qa_check_host_user"
+def package_qa_check_host_user(path, name, d, elf, messages):
+    """Check for paths outside of /home which are owned by the user running bitbake."""
+
+    if not os.path.lexists(path):
+        return
+
+    check_uid = int(d.getVar('HOST_USER_UID', True))
+    check_gid = int(d.getVar('HOST_USER_GID', True))
+
+    dest = d.getVar('PKGDEST', True)
+    home = os.path.join(dest, 'home')
+    if path == home or path.startswith(home + os.sep):
+        return
+
+    stat = os.lstat(path)
+    if stat.st_uid == check_uid:
+        messages["host-user-contaminated"] = "%s is owned by uid %d, which is the same as the user running bitbake. This may be due to host contamination" % (path, check_uid)
+        return False
+
+    if stat.st_gid == check_gid:
+        messages["host-user-contaminated"] = "%s is owned by gid %d, which is the same as the user running bitbake. This may be due to host contamination" % (path, check_gid)
+        return False
+    return True
+
 # The PACKAGE FUNC to scan each package
 python do_package_qa () {
     import subprocess
-- 
2.2.1

Re: [PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Joshua Lock]

On 24/08/15 23:19, Christopher Larson wrote:
> From: Christopher Larson <chris_larson@mentor.com>
>
> - Add a test which checks for any paths outside of /home which are owned by
>    the user running bitbake.
> - Add the test to WARN_QA by default.

I do all of my builds on a separate partition in a directory hierarchy 
which is owned by my user - if I'm understanding this correctly I'll get 
QA WARNINGS for all of my builds with this change?

It would be nice to be able to bless my build directory and still 
benefit from this check.

Regards,

Joshua

> This test has been in meta-mentor for some time, and in our ERROR_QA for our
> builds, and has caught a number of issues for us.
>
> Signed-off-by: Christopher Larson <chris_larson@mentor.com>
> ---
>   meta/classes/insane.bbclass | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index cd773b7..aec9800 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -31,14 +31,14 @@ WARN_QA ?= "ldflags useless-rpaths rpaths staticdev libdir xorg-driver-abi \
>               installed-vs-shipped compile-host-path install-host-path \
>               pn-overrides infodir build-deps file-rdeps \
>               unknown-configure-option symlink-to-sysroot multilib \
> -            invalid-pkgconfig \
> +            invalid-pkgconfig host-user-contaminated \
>               "
>   ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>               perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
>               split-strip packages-list pkgv-undefined var-undefined \
>               version-going-backwards expanded-d \
>               "
> -FAKEROOT_QA = ""
> +FAKEROOT_QA = "host-user-contaminated"
>   FAKEROOT_QA[doc] = "QA tests which need to run under fakeroot. If any \
>   enabled tests are listed here, the do_package_qa task will run under fakeroot."
>
> @@ -950,6 +950,34 @@ def package_qa_check_expanded_d(path,name,d,elf,messages):
>                           sane = False
>       return sane
>
> +HOST_USER_UID := "${@os.getuid()}"
> +HOST_USER_GID := "${@os.getgid()}"
> +
> +QAPATHTEST[host-user-contaminated] = "package_qa_check_host_user"
> +def package_qa_check_host_user(path, name, d, elf, messages):
> +    """Check for paths outside of /home which are owned by the user running bitbake."""
> +
> +    if not os.path.lexists(path):
> +        return
> +
> +    check_uid = int(d.getVar('HOST_USER_UID', True))
> +    check_gid = int(d.getVar('HOST_USER_GID', True))
> +
> +    dest = d.getVar('PKGDEST', True)
> +    home = os.path.join(dest, 'home')
> +    if path == home or path.startswith(home + os.sep):
> +        return
> +
> +    stat = os.lstat(path)
> +    if stat.st_uid == check_uid:
> +        messages["host-user-contaminated"] = "%s is owned by uid %d, which is the same as the user running bitbake. This may be due to host contamination" % (path, check_uid)
> +        return False
> +
> +    if stat.st_gid == check_gid:
> +        messages["host-user-contaminated"] = "%s is owned by gid %d, which is the same as the user running bitbake. This may be due to host contamination" % (path, check_gid)
> +        return False
> +    return True
> +
>   # The PACKAGE FUNC to scan each package
>   python do_package_qa () {
>       import subprocess
>

Re: [PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On 26 August 2015 at 09:44, Joshua Lock <joshua.lock@collabora.co.uk> wrote:

> I do all of my builds on a separate partition in a directory hierarchy
> which is owned by my user - if I'm understanding this correctly I'll get QA
> WARNINGS for all of my builds with this change?
>

The paths are prefixed with ${D} so pretend the commit log says "in
packages".

Ross

[-- Attachment #2: Type: text/html, Size: 802 bytes --]

Re: [PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Wed, Aug 26, 2015 at 4:01 AM, Burton, Ross <ross.burton@intel.com> wrote:

> On 26 August 2015 at 09:44, Joshua Lock <joshua.lock@collabora.co.uk>
> wrote:
>
>> I do all of my builds on a separate partition in a directory hierarchy
>> which is owned by my user - if I'm understanding this correctly I'll get QA
>> WARNINGS for all of my builds with this change?
>>
>
> The paths are prefixed with ${D} so pretend the commit log says "in
> packages".


Heh, indeed, it's a package QA test. do_install runs under pseudo, so any
newly created files there, or files chown'd to root, will be fine. If,
however, a recipe does a cp -a or so to install without doing a chown,
you'll end up with files in your rootfs owned by the user that did the
build -- not good.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 1578 bytes --]

Re: [PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Wed, Aug 26, 2015 at 7:19 AM, Christopher Larson <clarson@kergoth.com>
wrote:

> On Wed, Aug 26, 2015 at 4:01 AM, Burton, Ross <ross.burton@intel.com>
> wrote:
>
>> On 26 August 2015 at 09:44, Joshua Lock <joshua.lock@collabora.co.uk>
>> wrote:
>>
>>> I do all of my builds on a separate partition in a directory hierarchy
>>> which is owned by my user - if I'm understanding this correctly I'll get QA
>>> WARNINGS for all of my builds with this change?
>>>
>>
>> The paths are prefixed with ${D} so pretend the commit log says "in
>> packages".
>
>
> Heh, indeed, it's a package QA test. do_install runs under pseudo, so any
> newly created files there, or files chown'd to root, will be fine. If,
> however, a recipe does a cp -a or so to install without doing a chown,
> you'll end up with files in your rootfs owned by the user that did the
> build -- not good.


I can re-submit with that commit message clarification, if needed? I rather
thought the fact that it was in insane.bbclass, not sanity.bbclass, carried
the necessary implication.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2089 bytes --]

Re: [PATCH 2/3] insane.bbclass: add host-user-contaminated test

[Joshua Lock]

On 26/08/15 15:20, Christopher Larson wrote:
>
> On Wed, Aug 26, 2015 at 7:19 AM, Christopher Larson <clarson@kergoth.com
> <mailto:clarson@kergoth.com>> wrote:
>
>     On Wed, Aug 26, 2015 at 4:01 AM, Burton, Ross <ross.burton@intel.com
>     <mailto:ross.burton@intel.com>> wrote:
>
>         On 26 August 2015 at 09:44, Joshua Lock
>         <joshua.lock@collabora.co.uk
>         <mailto:joshua.lock@collabora.co.uk>> wrote:
>
>             I do all of my builds on a separate partition in a directory
>             hierarchy which is owned by my user - if I'm understanding
>             this correctly I'll get QA WARNINGS for all of my builds
>             with this change?
>
>
>         The paths are prefixed with ${D} so pretend the commit log says
>         "in packages".
>
>
>     Heh, indeed, it's a package QA test. do_install runs under pseudo,
>     so any newly created files there, or files chown'd to root, will be
>     fine. If, however, a recipe does a cp -a or so to install without
>     doing a chown, you'll end up with files in your rootfs owned by the
>     user that did the build -- not good.

Indeed. Thanks for taking the time to clarify.

>
> I can re-submit with that commit message clarification, if needed? I
> rather thought the fact that it was in insane.bbclass, not
> sanity.bbclass, carried the necessary implication.

Personally I don't feel that's necessary - I should review with more 
care (and coffee).

Thanks for the offer though.

Regards,

Joshua

[PATCH 3/3] image.bbclass: add rootfs_check_host_user_contaminated

[Christopher Larson]

From: Christopher Larson <chris_larson@mentor.com>

This function is intended to be used in ROOTFS_POSTPROCESS_COMMAND, and checks
for any paths outside of /home which are owned by the user running bitbake.

Signed-off-by: Christopher Larson <chris_larson@mentor.com>
---
 meta/classes/image.bbclass | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index ff2ed0d..d1cd9fd 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -458,6 +458,20 @@ rootfs_trim_schemas () {
 	done
 }
 
+rootfs_check_host_user_contaminated () {
+	contaminated="${WORKDIR}/host-user-contaminated.txt"
+	HOST_USER_UID="$(PSEUDO_UNLOAD=1 id -u)"
+	HOST_USER_GID="$(PSEUDO_UNLOAD=1 id -g)"
+
+	find "${IMAGE_ROOTFS}" -wholename "${IMAGE_ROOTFS}/home" -prune \
+	    -user "$HOST_USER_UID" -o -group "$HOST_USER_GID" >"$contaminated"
+
+	if [ -s "$contaminated" ]; then
+		echo "WARNING: Paths in the rootfs are owned by the same user or group as the user running bitbake. See the logfile for the specific paths."
+		cat "$contaminated" | sed "s,^,  ,"
+	fi
+}
+
 # Make any absolute links in a sysroot relative
 rootfs_sysroot_relativelinks () {
 	sysroot-relativelinks.py ${SDK_OUTPUT}/${SDKTARGETSYSROOT}
-- 
2.2.1

Re: [PATCH 0/3] Add checks for "host user contamination"

[Richard Purdie]

On Mon, 2015-08-24 at 15:19 -0700, Christopher Larson wrote:
> From: Christopher Larson <chris_larson@mentor.com>
> 
> This adds a QA test, as well as a rootfs_ function for use in
> ROOTFS_POSTPROCESS_COMMAND, to warn/error when paths are owned by the same
> user/group as the user running bitbake. This is useful to catch stuff which is
> written outside of pseudo's control, for example.
> 
> The following changes since commit c38acd720b3f6ffbeb544063692eb471dada8593:
> 
>   binconfig-disabled: write an message to stderr to help confused developers (2015-08-19 17:57:58 +0100)
> 
> are available in the git repository at:
> 
>   git://github.com/kergoth/openembedded-core host-user-contaminated
>   https://github.com/kergoth/openembedded-core/tree/host-user-contaminated
> 
> Christopher Larson (3):
>   insane.bbclass: handle tests which need fakeroot
>   insane.bbclass: add host-user-contaminated test
>   image.bbclass: add rootfs_check_host_user_contaminated

On the autobuilder we found:

http://errors.yoctoproject.org/Errors/Details/16318/

which looks like a race against the packaging code and the creation of
package manager files. Not sure why we don't see this in other cases...

Cheers,

Richard

Re: [PATCH 0/3] Add checks for "host user contamination"

[Richard Purdie]

On Tue, 2015-09-01 at 08:50 +0100, Richard Purdie wrote:
> On Mon, 2015-08-24 at 15:19 -0700, Christopher Larson wrote:
> > From: Christopher Larson <chris_larson@mentor.com>
> > 
> > This adds a QA test, as well as a rootfs_ function for use in
> > ROOTFS_POSTPROCESS_COMMAND, to warn/error when paths are owned by the same
> > user/group as the user running bitbake. This is useful to catch stuff which is
> > written outside of pseudo's control, for example.
> > 
> > The following changes since commit c38acd720b3f6ffbeb544063692eb471dada8593:
> > 
> >   binconfig-disabled: write an message to stderr to help confused developers (2015-08-19 17:57:58 +0100)
> > 
> > are available in the git repository at:
> > 
> >   git://github.com/kergoth/openembedded-core host-user-contaminated
> >   https://github.com/kergoth/openembedded-core/tree/host-user-contaminated
> > 
> > Christopher Larson (3):
> >   insane.bbclass: handle tests which need fakeroot
> >   insane.bbclass: add host-user-contaminated test
> >   image.bbclass: add rootfs_check_host_user_contaminated
> 
> On the autobuilder we found:
> 
> http://errors.yoctoproject.org/Errors/Details/16318/
> 
> which looks like a race against the packaging code and the creation of
> package manager files. Not sure why we don't see this in other cases...

Also http://errors.yoctoproject.org/Errors/Details/16310/ which looks
like the same issue (racing dpkg rather than opkg).

Cheers,

Richard

Re: [PATCH 0/3] Add checks for "host user contamination"

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Tue, Sep 1, 2015 at 12:52 AM, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Tue, 2015-09-01 at 08:50 +0100, Richard Purdie wrote:
> > On Mon, 2015-08-24 at 15:19 -0700, Christopher Larson wrote:
> > > From: Christopher Larson <chris_larson@mentor.com>
> > >
> > > This adds a QA test, as well as a rootfs_ function for use in
> > > ROOTFS_POSTPROCESS_COMMAND, to warn/error when paths are owned by the
> same
> > > user/group as the user running bitbake. This is useful to catch stuff
> which is
> > > written outside of pseudo's control, for example.
> > >
> > > The following changes since commit
> c38acd720b3f6ffbeb544063692eb471dada8593:
> > >
> > >   binconfig-disabled: write an message to stderr to help confused
> developers (2015-08-19 17:57:58 +0100)
> > >
> > > are available in the git repository at:
> > >
> > >   git://github.com/kergoth/openembedded-core host-user-contaminated
> > >
> https://github.com/kergoth/openembedded-core/tree/host-user-contaminated
> > >
> > > Christopher Larson (3):
> > >   insane.bbclass: handle tests which need fakeroot
> > >   insane.bbclass: add host-user-contaminated test
> > >   image.bbclass: add rootfs_check_host_user_contaminated
> >
> > On the autobuilder we found:
> >
> > http://errors.yoctoproject.org/Errors/Details/16318/
> >
> > which looks like a race against the packaging code and the creation of
> > package manager files. Not sure why we don't see this in other cases...
>
> Also http://errors.yoctoproject.org/Errors/Details/16310/ which looks
> like the same issue (racing dpkg rather than opkg).


Ah, indeed, sorry about that, I must have gotten lucky with our builds thus
far. I'll re-submit, ignoring ENOENT.
-- 
Christopher Larson
kergoth at gmail dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2978 bytes --]