[OE-core] [PATCH] [RFC] image.bbclass & image.py: Add check_image_file_ownership()

Haris Okanovic haris.okanovic at ni.com
Fri Jul 7 21:42:05 UTC 2017


> Why not just check the permissions on disk, from a rootfs postprocess 
> command? That wouldn’t be specific to tar, but would be just as 
> effective, since those run under pseudo.

I disagree that it "would be just as effective", since this approach 
relies on pseudo, tar, and OE's scripting (all of which are big, 
complicated, codebases) working correctly to create a fake sysroot 
representative of the booted system. OE sysroots are kinda leaky in my 
experience. I want to insulate this test from such bugs. Moreover, the 
current approach also implicitly verify the fake sysroot! If it were to 
fail in a way that assigns bad id or name attributes to files, say ones 
leaked from the host system, this test would catch it as presently written.

-- Haris


On 07/07/2017 03:28 PM, Christopher Larson wrote:
> Why not just check the permissions on disk, from a rootfs postprocess 
> command? That wouldn’t be specific to tar, but would be just as 
> effective, since those run under pseudo.
> 
> On Fri, Jul 7, 2017 at 12:36 PM, Haris Okanovic <haris.okanovic at ni.com 
> <mailto:haris.okanovic at ni.com>> wrote:
> 
>     An IMAGE_POSTPROCESS_COMMAND verifying image tarballs have symbolic and
>     numeric ownership attributes which match the enclosed shadow database:
>       * uid and uname of each file is present in /etc/passwd
>       * gid and gname of each file is present in /etc/group
>       * ids and names are consistent between tar metadata and shadow
>     database
> 
>     In other words, this test ensure there aren't any ownership surprises
>     when a filesystem created from tarball is booted. It's particularly
>     useful in cases where artifacts built outside of OE are included in
>     images -- E.g. packages from an external feed.
> 
>     Testing: Verified core-image-base still builds
> 
>     Patch:
>     https://github.com/harisokanovic/openembedded-core/commits/dev/hokanovi/image-ownership-sanity-check
>     <https://github.com/harisokanovic/openembedded-core/commits/dev/hokanovi/image-ownership-sanity-check>
> 
>     Signed-off-by: Haris Okanovic <haris.okanovic at ni.com
>     <mailto:haris.okanovic at ni.com>>
>     ---
>       meta/classes/image.bbclass |  25 +++++++++-
>       meta/lib/oe/image.py       | 114
>     +++++++++++++++++++++++++++++++++++++++++++++
>       2 files changed, 138 insertions(+), 1 deletion(-)
>       create mode 100644 meta/lib/oe/image.py
> 
>     diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>     index 6e30b96745..7b11291a82 100644
>     --- a/meta/classes/image.bbclass
>     +++ b/meta/classes/image.bbclass
>     @@ -192,7 +192,30 @@ python () {
>       IMAGE_CLASSES += "image_types"
>       inherit ${IMAGE_CLASSES}
> 
>     -IMAGE_POSTPROCESS_COMMAND ?= ""
>     +python check_image_file_ownership () {
>     +    import sys, os, oe.image
>     +
>     +    imgTypes = d.getVar("IMAGE_FSTYPES", True).split()
>     +    tarImgTypes = [ x for x in imgTypes if "tar" in x ]
>     +
>     +    # bail if there isn't a tar image in this build
>     +    # TODO: Maybe add other image types
>     +    if len(tarImgTypes) == 0:
>     +        bb.debug(1, "Skipping check, no 'tar' image specified")
>     +        return
>     +
>     +    imgDeployDir = d.getVar("IMGDEPLOYDIR", True)
>     +    imgName = d.getVar("IMAGE_NAME", True)
>     +    imgNameSuffix = d.getVar("IMAGE_NAME_SUFFIX", True)
>     +
>     +    for tarFileExt in tarImgTypes:
>     +        archiveFilename = (imgName + imgNameSuffix + "." + tarFileExt)
>     +        archiveFilePath = os.path.join(imgDeployDir, archiveFilename)
>     +
>     +        oe.image.check_file_ownership_tar(d, archiveFilePath)
>     +}
>     +
>     +IMAGE_POSTPROCESS_COMMAND ?= " check_image_file_ownership "
> 
>       # some default locales
>       IMAGE_LINGUAS ?= "de-de fr-fr en-gb"
>     diff --git a/meta/lib/oe/image.py b/meta/lib/oe/image.py
>     new file mode 100644
>     index 0000000000..f1ba6f2613
>     --- /dev/null
>     +++ b/meta/lib/oe/image.py
>     @@ -0,0 +1,114 @@
>     +# Helper function for image building
>     +
>     +def check_file_ownership_tar(d, archiveFilePath):
>     +    """
>     +    Verifies file ownership meta-data in image tarball matches users
>     +    and groups in shadow database (/etc/passwd and /etc/group files).
>     +    """
>     +    import sys, os, tarfile
>     +    bb.debug(1, "Running check_file_ownership() on image
>     '{0!s}'".format(archiveFilePath))
>     +
>     +    try:
>     +        archiveName = os.path.basename(archiveFilePath)
>     +        if not archiveName:
>     +            archiveName = archiveFilePath
>     +
>     +        # maps
>     +        unameMap = {}  # username -> passwd file entry array
>     +        gnameMap = {}  # groupname -> group file entry array
>     +        uidMap = {}  # uid -> passwd file entry array
>     +        gidMap   = {}  # gid -> group file entry array
>     +        fileMap = {}  # filepath -> TarInfo object
>     +
>     +        def read_shadow_file(fdName, fd, colCount, mapsList):
>     +            """ Reads a colon (:) delimited shadow database file
>     into mapsList """
>     +            for mapObj,_ in mapsList:
>     +                if len(mapObj) != 0:
>     +                    raise Exception("Already read '{0!s}'
>     file.".format(fdName))
>     +
>     +            for line in fd:
>     +                line = line.decode("utf-8").strip()
>     +                words = line.split(":")
>     +
>     +                if len(words) != colCount:
>     +                    raise Exception("Malformed '{0!s}' file.
>     Expected {1!s} cols.".format(fdName, colCount))
>     +
>     +                for mapObj,mapKeyCol in mapsList:
>     +                    mapKey = words[mapKeyCol]
>     +                    if len(mapKey) < 1:
>     +                        raise Exception("Map key in '{0!s}' must be
>     at least one char long.".format(fdName))
>     +
>     +                    if mapKey in mapObj:
>     +                        raise Exception("Malformed '{0!s}' file.
>     Did not expect to find '{0!s}' in map.".format(fdName, mapKey))
>     +
>     +                    mapObj[mapKey] = words
>     +
>     +            for mapObj,_ in mapsList:
>     +                if len(mapObj) == 0:
>     +                    raise Exception("'{0!s}' is empty.".format(fdName))
>     +
>     +        bb.debug(1, "Read the archive and populate maps")
>     +        with tarfile.open(name=archiveFilePath, mode='r:*') as tar:
>     +            for info in tar:
>     +                if info.name <http://info.name> in fileMap:
>     +                    raise Exception("Duplicate entry for '{0!s}'
>     found.".format(info.name <http://info.name>))
>     +
>     +                fileMap[info.name <http://info.name>] = info
>     +
>     +                # populate shadow db maps
>     +                if info.name <http://info.name> == './etc/passwd': 
>     read_shadow_file(fdName=info.name <http://info.name>,
>     fd=tar.extractfile(info), colCount=7, mapsList=[(unameMap, 0),
>     (uidMap, 2), ])
>     +                elif info.name <http://info.name> ==
>     './etc/group':   read_shadow_file(fdName=info.name
>     <http://info.name>, fd=tar.extractfile(info), colCount=4,
>     mapsList=[(gnameMap, 0), (gidMap, 2), ])
>     +
>     +        bb.debug(1, "Check for no shadow db")
>     +        shadowItemCount = 0
>     +        for mapObj in [unameMap, gnameMap, uidMap, gidMap]:
>     +            shadowItemCount = shadowItemCount + len(mapObj)
>     +        if shadowItemCount == 0:
>     +            bb.warn(1, "check_file_ownership(): Skip; no shadow
>     database in image '{0!s}'".format(archiveName))
>     +            return
>     +
>     +        bb.debug(1, "Map sanity check")
>     +        for mapObj in [unameMap, gnameMap, uidMap, gidMap, fileMap]:
>     +            if len(mapObj) < 1:
>     +                raise Exception("Uh oh. Empty map found.")
>     +
>     +        def badFileError(errorMessage, info, shadowEntry=None):
>     +            linkname = ""
>     +            if info.linkname:
>     +                linkname = " -> {0!s}".format(info.linkname)
>     +
>     +            bb.error("BAD FILE '{0!s}': {1!s}".format(info.name
>     <http://info.name>, errorMessage))
>     +            bb.error("## {info.uid!s}:{info.gid!s}
>     ({info.uname!s}:{info.gname!s}) 0{info.mode:o} {info.name
>     <http://info.name>!s}{linkname!s}".format(info=info, linkname=linkname))
>     +            if shadowEntry:
>     +                bb.error("## {0!s}".format(shadowEntry))
>     +
>     +        bb.debug(1, "Check for bad files")
>     +        for filepath,info in fileMap.items():
>     +            if not info.uname in unameMap:
>     +                badFileError("uname not in unameMap", info)
>     +                continue
>     +
>     +            if not info.gname in gnameMap:
>     +                badFileError("gname not in gnameMap", info)
>     +                continue
>     +
>     +            if not str(info.uid) in uidMap:
>     +                badFileError("Uid '{0!s}' not
>     found".format(info.uid), info)
>     +
>     +            if not str(info.gid) in gidMap:
>     +                badFileError("Gid '{0!s}' not
>     found".format(info.gid), info)
>     +
>     +            unameEntry = unameMap[info.uname]
>     +            gnameEntry = gnameMap[info.gname]
>     +
>     +            if str(info.uid) != unameEntry[2]:
>     +                badFileError("uid mismatch, expecting '{0!s}' for
>     uname '{1!s}'".format(unameEntry[2], info.uname), info, unameEntry)
>     +
>     +            if str(info.gid) != gnameEntry[2]:
>     +                badFileError("gid mismatch, expecting '{0!s}' for
>     gname '{1!s}'".format(gnameEntry[2], info.gname), info, gnameEntry)
>     +
>     +        bb.debug(1, "Successfully verified image
>     '{0!s}'".format(archiveName))
>     +
>     +    # Error on any exceptions
>     +    except Exception as e:
>     +        bb.error("check_file_ownership() exception: {0!s}".format(e))
>     --
>     2.13.2
> 
>     --
>     _______________________________________________
>     Openembedded-core mailing list
>     Openembedded-core at lists.openembedded.org
>     <mailto:Openembedded-core at lists.openembedded.org>
>     http://lists.openembedded.org/mailman/listinfo/openembedded-core
>     <http://lists.openembedded.org/mailman/listinfo/openembedded-core>
> 
> 
> 
> 
> -- 
> Christopher Larson
> kergoth at gmail dot com
> Founder - BitBake, OpenEmbedded, OpenZaurus
> Senior Software Engineer, Mentor Graphics



More information about the Openembedded-core mailing list