Add support for shipping extended attributes in debs

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Add support for shipping extended attributes in debs

Matthew Garrett-4
This is currently something of a scratch proposal, but let's see where
it goes. This patchset adds support for shipping an mtree file within
debs, which (right now) is only used for writeout of extended
attributes. The idea is that additional metadata can be incorporated
into the mtree file, providing a unified format for metadata storage and
making it easy to compare the installed system state to the default
package state (and restore it if necessary).

I'm primarily interested in using this to ship security metadata in the
form of security.ima and security.evm, but this also provides a way to
ship file capabilities without requiring postinst to mess about with
setcap.


Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] Add extended attribute support to mtree

Matthew Garrett-4
From: Matthew Garrett <[hidden email]>

Add support for parsing extended attributes out of mtree data.
---
 lib/mtree/misc.c  | 10 +++++++++-
 lib/mtree/mtree.h | 26 +++++++++++++++++---------
 lib/mtree/spec.c  | 18 ++++++++++++++++++
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/lib/mtree/misc.c b/lib/mtree/misc.c
index fc2124f2b..207158565 100644
--- a/lib/mtree/misc.c
+++ b/lib/mtree/misc.c
@@ -51,6 +51,7 @@ typedef struct _key {
         u_int val;                      /* value */
 
 #define NEEDVALUE       0x01
+#define PREFIX 0x02
         u_int flags;
 } KEY;
 
@@ -72,6 +73,7 @@ static KEY keylist[] = {
         {"type",        F_TYPE,         NEEDVALUE},
         {"uid",         F_UID,          NEEDVALUE},
         {"uname",       F_UNAME,        NEEDVALUE},
+        {"xattr",       F_XATTR,        NEEDVALUE|PREFIX},
 };
 
 int keycompare(const void *, const void *);
@@ -95,7 +97,13 @@ parsekey(char *name, int *needvaluep)
 int
 keycompare(const void *a, const void *b)
 {
-        return (strcmp(((const KEY *)a)->name, ((const KEY *)b)->name));
+ const KEY *k = (const KEY *)b;
+
+ if (k->flags & PREFIX)
+ return (strncmp(((const KEY *)a)->name, k->name,
+ strlen(k->name)));
+ else
+ return (strcmp(((const KEY *)a)->name, k->name));
 }
 
 char *
diff --git a/lib/mtree/mtree.h b/lib/mtree/mtree.h
index e78a4fa6c..fb66b3c45 100644
--- a/lib/mtree/mtree.h
+++ b/lib/mtree/mtree.h
@@ -45,6 +45,11 @@
 
 #define MISMATCHEXIT    2
 
+struct xattr {
+ char *name;
+ char *value;
+};
+
 typedef struct _node {
         struct _node    *parent, *child;        /* up, down */
         struct _node    *prev, *next;           /* left, right */
@@ -53,6 +58,8 @@ typedef struct _node {
         u_long  cksum;                          /* check sum */
         char    *md5digest;                     /* MD5 digest */
         char    *slink;                         /* symbolic link reference */
+ struct xattr *xattrs;                   /* extended attributes */
+ int xattr_num;                          /* number of xattrs */
         uid_t   st_uid;                         /* uid */
         gid_t   st_gid;                         /* gid */
 #define MBITS   (S_ISUID|S_ISGID|S_ISVTX|S_IRWXU|S_IRWXG|S_IRWXO)
@@ -68,15 +75,16 @@ typedef struct _node {
 #define F_MAGIC 0x000020 /* name has magic chars */
 #define F_MODE 0x000040 /* mode */
 #define F_NLINK 0x000080 /* number of hardlinks */
-#define F_SIZE 0x000100 /* size */
-#define F_SLINK 0x000200 /* symbolic link path */
-#define F_TIME 0x000400 /* modification time */
-#define F_TYPE 0x000800 /* file type */
-#define F_UID 0x001000 /* uid */
-#define F_UNAME 0x002000 /* user name */
-#define F_VISIT 0x004000 /* file visited */
-#define F_MD5 0x008000 /* MD5 digest */
-#define F_NOCHANGE 0x010000 /* If owner/mode "wrong", do */
+#define F_XATTR         0x000100                /* extended attribute */
+#define F_SIZE 0x000200 /* size */
+#define F_SLINK 0x000400 /* symbolic link path */
+#define F_TIME 0x000800 /* modification time */
+#define F_TYPE 0x001000 /* file type */
+#define F_UID 0x002000 /* uid */
+#define F_UNAME 0x004000 /* user name */
+#define F_VISIT 0x008000 /* file visited */
+#define F_MD5 0x010000 /* MD5 digest */
+#define F_NOCHANGE 0x020000 /* If owner/mode "wrong", do */
  /* not change */
 #define F_FLAGS 0x080000 /* file flags */
 #define F_OPT 0x200000 /* existence optional */
diff --git a/lib/mtree/spec.c b/lib/mtree/spec.c
index 47d2bc636..fb2ea78a5 100644
--- a/lib/mtree/spec.c
+++ b/lib/mtree/spec.c
@@ -294,6 +294,24 @@ set(char *t, NODE *ip)
                             errx(1, "line %d: unknown user %s", lineno, val);
                         ip->st_uid = pw->pw_uid;
                         break;
+                case F_XATTR:
+                        /* Verify that there's an actual xattr name */
+ if (strlen(kw) < strlen("xattr."))
+  continue;
+ ip->xattrs = realloc(ip->xattrs, (ip->xattr_num + 1)
+     * sizeof(struct xattr));
+ if (!ip->xattrs)
+ errx(1, "realloc");
+                        /* Skip the prefix when setting the name */
+                        ip->xattrs[ip->xattr_num].name = strdup(kw +
+     strlen("xattr."));
+                        if(!ip->xattrs[ip->xattr_num].name)
+                                errx(1, "strdup");
+                        ip->xattrs[ip->xattr_num].value = strdup(val);
+                        if(!ip->xattrs[ip->xattr_num].value)
+                                errx(1, "strdup");
+ ip->xattr_num++;
+                        break;
                 }
         }
 }
--
2.17.0.441.gb46fe60e1d-goog

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5] Add an implementation of base64

Matthew Garrett-4
In reply to this post by Matthew Garrett-4
From: Matthew Garrett <[hidden email]>

Signatures are encoded as base64 in the mtree file, so add a decoder
(derived from glib) so we can turn them back into binary
---
 lib/dpkg/dpkg.h  |  2 ++
 lib/dpkg/utils.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 19b7914f4..9a321b88a 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -67,6 +67,7 @@ DPKG_BEGIN_DECLS
 
 #define CONTROLFILE        "control"
 #define CONFFILESFILE      "conffiles"
+#define MTREEFILE          "mtree"
 #define PREINSTFILE        "preinst"
 #define POSTINSTFILE       "postinst"
 #define PRERMFILE          "prerm"
@@ -153,6 +154,7 @@ void m_output(FILE *f, const char *name);
 
 int fgets_checked(char *buf, size_t bufsz, FILE *f, const char *fn);
 int fgets_must(char *buf, size_t bufsz, FILE *f, const char *fn);
+char *base64_decode (const char *in, unsigned int *outlen);
 
 DPKG_END_DECLS
 
diff --git a/lib/dpkg/utils.c b/lib/dpkg/utils.c
index 363624c4c..8eb85ec5f 100644
--- a/lib/dpkg/utils.c
+++ b/lib/dpkg/utils.c
@@ -56,3 +56,80 @@ fgets_must(char *buf, size_t bufsz, FILE *f, const char *fn)
 
  return l;
 }
+
+static const unsigned char mime_base64_rank[256] = {
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255, 62,255,255,255, 63,
+   52, 53, 54, 55, 56, 57, 58, 59, 60, 61,255,255,255,  0,255,255,
+  255,  0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14,
+   15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,255,255,255,255,255,
+  255, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40,
+   41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+  255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,
+};
+
+char *
+base64_decode (const char *in, unsigned int *outlen)
+{
+  const unsigned char *inptr;
+  unsigned char *out, *outptr;
+  const unsigned char *inend;
+  unsigned char c, rank;
+  unsigned char last[2];
+  unsigned int v;
+  int i;
+  size_t len;
+
+  len = strlen(in);
+  out = malloc((len/4) * 3 + 1);
+  inend = (const unsigned char *)in+len;
+  outptr = out;
+
+  /* convert 4 base64 bytes to 3 normal bytes */
+  v=0;
+  i=0;
+
+  last[0] = last[1] = 0;
+
+  /* we use the sign in the state to determine if we got a padding character
+     in the previous sequence */
+  if (i < 0)
+    {
+      i = -i;
+      last[0] = '=';
+    }
+
+  inptr = (const unsigned char *)in;
+  while (inptr < inend)
+    {
+      c = *inptr++;
+      rank = mime_base64_rank [c];
+      if (rank != 0xff)
+        {
+          last[1] = last[0];
+          last[0] = c;
+          v = (v<<6) | rank;
+          i++;
+          if (i==4)
+            {
+              *outptr++ = v>>16;
+              if (last[1] != '=')
+                *outptr++ = v>>8;
+              if (last[0] != '=')
+                *outptr++ = v;
+              i=0;
+            }
+        }
+    }
+
+  *outlen = outptr - out;
+  return (char *)out;
+}
--
2.17.0.441.gb46fe60e1d-goog

Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5] Allow (and require) absolute pathnames in mtree files

Matthew Garrett-4
In reply to this post by Matthew Garrett-4
From: Matthew Garrett <[hidden email]>

The mtree format arbitrarily prohibits absolute full pathnames. Remove
the restriction and also remove support for relative paths.
---
 lib/mtree/spec.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/lib/mtree/spec.c b/lib/mtree/spec.c
index fb2ea78a5..f1d249eac 100644
--- a/lib/mtree/spec.c
+++ b/lib/mtree/spec.c
@@ -118,25 +118,6 @@ mtree_readspec(FILE *fi)
                                 continue;
                         }
 
-                if (index(p, '/'))
-                        errx(1, "line %d: slash character in file name",
-                        lineno);
-
-                if (!strcmp(p, "..")) {
-                        /* Don't go up, if haven't gone down. */
-                        if (!root)
-                                goto noparent;
-                        if (last->type != F_DIR || last->flags & F_DONE) {
-                                if (last == root)
-                                        goto noparent;
-                                last = last->parent;
-                        }
-                        last->flags |= F_DONE;
-                        continue;
-
-noparent:               errx(1, "line %d: no parent node", lineno);
-                }
-
                 if ((centry = calloc(1, sizeof(NODE) + strlen(p))) == NULL)
                         errx(1, "calloc");
                 *centry = ginfo;
@@ -150,9 +131,6 @@ noparent:               errx(1, "line %d: no parent node", lineno);
                 if (!root) {
                         last = root = centry;
                         root->parent = root;
-                } else if (last->type == F_DIR && !(last->flags & F_DONE)) {
-                        centry->parent = last;
-                        last = last->child = centry;
                 } else {
                         centry->parent = last->parent;
                         centry->prev = last;
--
2.17.0.441.gb46fe60e1d-goog

Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] Add support for writing out extended attributes

Matthew Garrett-4
In reply to this post by Matthew Garrett-4
From: Matthew Garrett <[hidden email]>

Parse any mtree file present in control.tar.gz and attempt to write any
extended attributes. Ignore any failures caused by the filesystem not
having extended attribute support.
---
 src/Makefile.am |  1 +
 src/archives.c  | 15 +++++++++++
 src/filesdb.c   |  1 +
 src/filesdb.h   |  9 +++++++
 src/unpack.c    | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 94 insertions(+)

diff --git a/src/Makefile.am b/src/Makefile.am
index 2d776dd9a..89a013b57 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -7,6 +7,7 @@ AM_CPPFLAGS = \
  -DLOCALEDIR=\"$(localedir)\" \
  -DADMINDIR=\"$(admindir)\" \
  -idirafter $(top_srcdir)/lib/compat \
+ -idirafter $(top_srcdir)/lib/mtree \
  -I$(top_builddir) \
  -I$(top_srcdir)/lib
 LDADD = \
diff --git a/src/archives.c b/src/archives.c
index 113b76c3d..22ef85eb3 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -41,6 +41,8 @@
 #define obstack_chunk_alloc m_malloc
 #define obstack_chunk_free free
 
+#include <attr/xattr.h>
+
 #include <dpkg/i18n.h>
 #include <dpkg/dpkg.h>
 #include <dpkg/dpkg-db.h>
@@ -353,6 +355,7 @@ tarobject_extract(struct tarcontext *tc, struct tar_entry *te,
   char fnamebuf[256];
   char fnamenewbuf[256];
   char *newhash;
+  int ret, i;
 
   switch (te->type) {
   case TAR_FILETYPE_FILE:
@@ -388,6 +391,18 @@ tarobject_extract(struct tarcontext *tc, struct tar_entry *te,
             namenode->statoverride->uid,
             namenode->statoverride->gid,
             namenode->statoverride->mode);
+
+    /* Attempt to set any extended attributes.If the output filesystem
+     * doesn't support xattrs, skip it silently. */
+    if (namenode->xattr_num) {
+      for (i = 0; i < namenode->xattr_num; i++) {
+ struct dpkg_xattr *xattr = &namenode->xattrs[i];
+ ret = fsetxattr(fd, xattr->name, xattr->val, xattr->len, 0);
+ if (ret < 0 && errno != ENOTSUP)
+  ohshite(_("error setting IMA attribute of '%.255s'"), te->name);
+      }
+    }
+
     if (fchown(fd, st->uid, st->gid))
       ohshite(_("error setting ownership of '%.255s'"), te->name);
     if (fchmod(fd, st->mode & ~S_IFMT))
diff --git a/src/filesdb.c b/src/filesdb.c
index 7f157727d..e7b35f1f4 100644
--- a/src/filesdb.c
+++ b/src/filesdb.c
@@ -632,6 +632,7 @@ struct filenamenode *findnamenode(const char *name, enum fnnflags flags) {
   newnode->newhash = EMPTYHASHFLAG;
   newnode->filestat = NULL;
   newnode->trig_interested = NULL;
+  newnode->xattr_num = 0;
   *pointerp= newnode;
   nfiles++;
 
diff --git a/src/filesdb.h b/src/filesdb.h
index 954d67bd8..3f0b4c50d 100644
--- a/src/filesdb.h
+++ b/src/filesdb.h
@@ -77,6 +77,12 @@ enum filenamenode_flags {
  fnnf_filtered = DPKG_BIT(9),
 };
 
+struct dpkg_xattr {
+  char *name;
+  char *val;
+  unsigned int len;
+};
+
 struct filenamenode {
   struct filenamenode *next;
   const char *name;
@@ -106,6 +112,9 @@ struct filenamenode {
 
   struct stat *filestat;
   struct trigfileint *trig_interested;
+
+  struct dpkg_xattr *xattrs;
+  int xattr_num;
 };
 
 struct fileinlist {
diff --git a/src/unpack.c b/src/unpack.c
index f43c01e45..2668a5e32 100644
--- a/src/unpack.c
+++ b/src/unpack.c
@@ -28,6 +28,8 @@
 #include <sys/stat.h>
 #include <sys/wait.h>
 
+#include <mtree.h>
+
 #include <errno.h>
 #include <string.h>
 #include <time.h>
@@ -316,6 +318,68 @@ pkg_deconfigure_others(struct pkginfo *pkg)
   }
 }
 
+static void
+walk_mtree(NODE *mtree)
+{
+  struct filenamenode *namenode;
+  NODE *old;
+  int i;
+
+  while (mtree) {
+    if (mtree->child)
+      walk_mtree(mtree->child);
+
+    namenode = findnamenode(mtree->name, 0);
+    if (namenode) {
+      if (mtree->xattr_num) {
+ namenode->xattrs = malloc(mtree->xattr_num * sizeof(struct dpkg_xattr));
+ for (i=0; i<mtree->xattr_num; i++) {
+  struct dpkg_xattr *xattr = &namenode->xattrs[i];
+  xattr->name = strdup(mtree->xattrs[i].name);
+  xattr->val = base64_decode(mtree->xattrs[i].value, &xattr->len);
+ }
+      }
+      namenode->xattr_num = mtree->xattr_num;
+    }
+    old = mtree;
+    mtree = mtree->next;
+    free(old->md5digest);
+    free(old->slink);
+    free(old);
+  }
+}
+
+/**
+ *
+ * Read the mtree data
+ */
+static void
+deb_parse_mtree(struct pkginfo *pkg, const char *control_mtree)
+{
+  FILE *mtreef;
+  NODE *mtreedata;
+
+  mtreef = fopen(control_mtree, "r");
+  if (mtreef == NULL) {
+    if (errno == ENOENT)
+      return;
+    ohshite(_("error trying to open %.250s"), control_mtree);
+  }
+
+  push_cleanup(cu_closestream, ehflag_bombout, NULL, 0, 1, mtreef);
+
+  mtreedata = mtree_readspec(mtreef);
+
+  pop_cleanup(ehflag_normaltidy); /* mtreef = fopen() */
+  if (fclose(mtreef))
+    ohshite(_("error closing %.250s"), control_mtree);
+
+  if (!mtreedata)
+    return;
+
+  walk_mtree(mtreedata);
+}
+
 /**
  * Read the conffiles, and copy the hashes across.
  */
@@ -1212,6 +1276,10 @@ void process_archive(const char *filename) {
   strcpy(cidirrest, TRIGGERSCIFILE);
   trig_parse_ci(cidir, NULL, trig_cicb_statuschange_activate, pkg, &pkg->available);
 
+  /* Parse the mtree metadata. */
+  strcpy(cidirrest,MTREEFILE);
+  deb_parse_mtree(pkg, cidir);
+
   /* Read the conffiles, and copy the hashes across. */
   newconffiles.head = NULL;
   newconffiles.tail = &newconffiles.head;
--
2.17.0.441.gb46fe60e1d-goog

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Ian Jackson-2
In reply to this post by Matthew Garrett-4
Matthew Garrett writes ("Add support for shipping extended attributes in debs"):

> This is currently something of a scratch proposal, but let's see where
> it goes. This patchset adds support for shipping an mtree file within
> debs, which (right now) is only used for writeout of extended
> attributes. The idea is that additional metadata can be incorporated
> into the mtree file, providing a unified format for metadata storage and
> making it easy to compare the installed system state to the default
> package state (and restore it if necessary).
>
> I'm primarily interested in using this to ship security metadata in the
> form of security.ima and security.evm, but this also provides a way to
> ship file capabilities without requiring postinst to mess about with
> setcap.

Why do you want to ship security metadata and have dpkg apply it ?

Security mechanisms serve the interests of some people over others,
and we should understand who we are helping and who we are hurting, so
we can decide whether that's a good thing.

FAOD I generally trust your personal judgement about these kinds of
things but I think this is a question that you ought to have addressed
in your mail.

(In the unlikely event that the answer now is that we shouldn't
include this feature because the users would be contrary to our
values, of course we might need to revisit that if other use cases
turn up.)

Thanks,
Ian.

--
Ian Jackson <[hidden email]>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Matthew Garrett-4
On Wed, May 2, 2018 at 5:39 AM Ian Jackson <[hidden email]>
wrote:
> Why do you want to ship security metadata and have dpkg apply it ?

For our internal systems, we want to be able to distinguish between
binaries that have been produced by our internal build infrastructure and
binaries that have been built locally or obtained from a third party. We
impose an LSM policy that distinguishes between "trusted" and "untrusted"
binaries, and forbids untrusted binaries from accessing some sensitive
resources (such as credentials for access to production systems). Trusted
binaries are signed at build time, and we verify that the signatures are
valid before allowing anything to execute in the trusted security context.

 From a more generally practical perspective - Linux filesystem capabilities
are implemented using extended attributes, and right now dpkg has no way of
representing that. This means that packages that make use of filesystem
capabilities do so by running setcap in their postinst. This isn't
meaningfully parsable, so there's no good way to verify that the system
state is valid.

More niche - this could also be used to ship SELinux labels in band, rather
than having to fix up labeling after package install. I don't know whether
that would be something Debian would want, but it could be useful for some
special cases.

> Security mechanisms serve the interests of some people over others,
> and we should understand who we are helping and who we are hurting, so
> we can decide whether that's a good thing.

Absolutely. In itself any IMA or EVM signatures do nothing - the kernel
needs to be provided with a policy to restrict them, and that can be
overridden from the kernel command line. If a vendor is building a
Debian-based device that uses debs for updates and wants to lock down the
device such that users aren't able to run code of their choosing, this
would make it easier for them to do that. But I don't think we've seen many
cases of vendors trying to use Debian in that way.

My personal perspective is that making it easier for users to choose the
policy that they want to impose on their system is worthwhile, and if a
user wants to voluntarily lock down their system so it only runs software
that comes from official Debian repositories that's a thing they ought to
be able to do - as a hypothetical example, if non-free was signed with a
different key, a user would be able to ensure that their machine didn't run
any non-free software.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Matthew Garrett-4
In reply to this post by Matthew Garrett-4
It looks like the first patch was too big to make it to the list, so I've
pushed the tree to https://gitlab.com/mjg59/dpkg/commits/master

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Ian Jackson-2
In reply to this post by Matthew Garrett-4
Matthew Garrett writes ("Re: Add support for shipping extended attributes in debs"):

> On Wed, May 2, 2018 at 5:39 AM Ian Jackson <[hidden email]>
> wrote:
> > Why do you want to ship security metadata and have dpkg apply it ?
>
> For our internal systems, we want to be able to distinguish between
> binaries that have been produced by our internal build infrastructure and
> binaries that have been built locally or obtained from a third party. We
> impose an LSM policy that distinguishes between "trusted" and "untrusted"
> binaries, and forbids untrusted binaries from accessing some sensitive
> resources (such as credentials for access to production systems). Trusted
> binaries are signed at build time, and we verify that the signatures are
> valid before allowing anything to execute in the trusted security context.

I see.  That's a nice explanation of the next layer up.  But I was
hoping for a layer 9 anser.

Ian.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Matthew Garrett-4
On Thu, May 3, 2018 at 8:39 AM Ian Jackson <[hidden email]>
wrote:

> Matthew Garrett writes ("Re: Add support for shipping extended attributes
in debs"):
> > On Wed, May 2, 2018 at 5:39 AM Ian Jackson <
[hidden email]>
> > wrote:
> > > Why do you want to ship security metadata and have dpkg apply it ?
> >
> > For our internal systems, we want to be able to distinguish between
> > binaries that have been produced by our internal build infrastructure
and
> > binaries that have been built locally or obtained from a third party. We
> > impose an LSM policy that distinguishes between "trusted" and
"untrusted"
> > binaries, and forbids untrusted binaries from accessing some sensitive
> > resources (such as credentials for access to production systems).
Trusted
> > binaries are signed at build time, and we verify that the signatures are
> > valid before allowing anything to execute in the trusted security
context.

> I see.  That's a nice explanation of the next layer up.  But I was
> hoping for a layer 9 anser.

I'm not sure I understand. In order to achieve this we need to ship the
signatures. The signatures are directly associated with the files. If dpkg
is installing the files then it also ought to be writing out the
signatures, otherwise things can end up out of sync - if a binary is
executed before the signature is written out then either it'll end up in
the untrusted tier or the kernel will block execution because the IMA or
EVM validation will fail.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Ian Jackson-2
Matthew Garrett writes ("Re: Add support for shipping extended attributes in debs"):

> On Thu, May 3, 2018 at 8:39 AM Ian Jackson <[hidden email]>
> wrote:
> > I see.  That's a nice explanation of the next layer up.  But I was
> > hoping for a layer 9 anser.
>
> I'm not sure I understand. In order to achieve this we need to ship the
> signatures. The signatures are directly associated with the files. If dpkg
> is installing the files then it also ought to be writing out the
> signatures, otherwise things can end up out of sync - if a binary is
> executed before the signature is written out then either it'll end up in
> the untrusted tier or the kernel will block execution because the IMA or
> EVM validation will fail.

Who wants the unapproved binaries to run, and who wants to prevent
them from running, and (in each case) why ?

My reference to `layer 9' is to the usage seen here:
  http://www.flora.ca/osw2004/osw2004.pdf

Ian.

--
Ian Jackson <[hidden email]>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Matthew Garrett-4
On Fri, May 4, 2018 at 4:12 AM Ian Jackson <[hidden email]>
wrote:

> Matthew Garrett writes ("Re: Add support for shipping extended attributes
in debs"):
> > On Thu, May 3, 2018 at 8:39 AM Ian Jackson <
[hidden email]>
> > wrote:
> > > I see.  That's a nice explanation of the next layer up.  But I was
> > > hoping for a layer 9 anser.
> >
> > I'm not sure I understand. In order to achieve this we need to ship the
> > signatures. The signatures are directly associated with the files. If
dpkg
> > is installing the files then it also ought to be writing out the
> > signatures, otherwise things can end up out of sync - if a binary is
> > executed before the signature is written out then either it'll end up in
> > the untrusted tier or the kernel will block execution because the IMA or
> > EVM validation will fail.

> Who wants the unapproved binaries to run, and who wants to prevent
> them from running, and (in each case) why ?

For our case: we don't want binaries of unknown provenance to have access
to sensitive credentials. Attackers who've compromised user accounts do.
We're not actually seeking to block execution of unsigned binaries, we just
want unsigned binaries to run in a less privileged security domain.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Ian Jackson-2
Matthew Garrett writes ("Re: Add support for shipping extended attributes in debs"):
> On Fri, May 4, 2018 at 4:12 AM Ian Jackson <[hidden email]>
> wrote:
> > Who wants the unapproved binaries to run, and who wants to prevent
> > them from running, and (in each case) why ?
>
> For our case: we don't want binaries of unknown provenance to have access
> to sensitive credentials. Attackers who've compromised user accounts do.
> We're not actually seeking to block execution of unsigned binaries, we just
> want unsigned binaries to run in a less privileged security domain.

Thanks for the explanation, that makes sense.

Ian.

--
Ian Jackson <[hidden email]>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Guillem Jover
In reply to this post by Matthew Garrett-4
Hi!

On Tue, 2018-05-01 at 13:37:26 -0700, Matthew Garrett wrote:
> This is currently something of a scratch proposal, but let's see where
> it goes. This patchset adds support for shipping an mtree file within
> debs, which (right now) is only used for writeout of extended
> attributes. The idea is that additional metadata can be incorporated
> into the mtree file, providing a unified format for metadata storage and
> making it easy to compare the installed system state to the default
> package state (and restore it if necessary).

Thanks for the patches. And sorry again for having been so slow on the
dpkg front for the last several months, and not having finished the work
on this as I wanted to, life got in the way. :/

Unfortunately this does not integrate quite well with the dpkg
internals, some of the files are also using BSD 4-clause license, so I
think I'll continue with the implementation I've got half done. My plan
is still to first switch the internal database into using mtree (and
stop using the files list files), then consider binaries shipping such
metadata, and then exposing that in the source package.

  <https://wiki.debian.org/Teams/Dpkg/Spec/MetadataTracking>

For the first stage, I'm moving the file metadata from dpkg into libdpkg,
which will also make it easier to unit test it.

> I'm primarily interested in using this to ship security metadata in the
> form of security.ima and security.evm, but this also provides a way to
> ship file capabilities without requiring postinst to mess about with
> setcap.

As I've mentioned in the past on IRC, I'm yet not sure how we'd integrate
the signing mechanism part into the toolchain, also because this might
require external tooling that for example in Debian or derivatives we
might not directly want to use, but others might, so this might need
to be something to hook into somewhere.

Also I'm not sure file capabilities is a good example, because those
tend to be conditional, and when they fail to be set, the maintscripts
tend to fallback to using set-user-ID on the binaries, and also because
these are inherently non-portable.

Thanks,
Guillem

Reply | Threaded
Open this post in threaded view
|

Re: Add support for shipping extended attributes in debs

Matthew Garrett-4
On Tue, May 8, 2018 at 5:04 AM Guillem Jover <[hidden email]> wrote:
> On Tue, 2018-05-01 at 13:37:26 -0700, Matthew Garrett wrote:
> > This is currently something of a scratch proposal, but let's see where
> > it goes. This patchset adds support for shipping an mtree file within
> > debs, which (right now) is only used for writeout of extended
> > attributes. The idea is that additional metadata can be incorporated
> > into the mtree file, providing a unified format for metadata storage and
> > making it easy to compare the installed system state to the default
> > package state (and restore it if necessary).

> Thanks for the patches. And sorry again for having been so slow on the
> dpkg front for the last several months, and not having finished the work
> on this as I wanted to, life got in the way. :/

> Unfortunately this does not integrate quite well with the dpkg
> internals, some of the files are also using BSD 4-clause license, so I
> think I'll continue with the implementation I've got half done. My plan
> is still to first switch the internal database into using mtree (and
> stop using the files list files), then consider binaries shipping such
> metadata, and then exposing that in the source package.

I'm more than happy to contribute to your implementation, but it'd be
easier if you'd post what you have :) The 4-clause stuff all appears to be
copyright Berkeley, so my understanding is that it can just be relicensed
to 3-clause.

> > I'm primarily interested in using this to ship security metadata in the
> > form of security.ima and security.evm, but this also provides a way to
> > ship file capabilities without requiring postinst to mess about with
> > setcap.

> As I've mentioned in the past on IRC, I'm yet not sure how we'd integrate
> the signing mechanism part into the toolchain, also because this might
> require external tooling that for example in Debian or derivatives we
> might not directly want to use, but others might, so this might need
> to be something to hook into somewhere.

We're generating the signatures in our internal build system and then
merging those into any existing mtree file, so there's no inherent conflict
there. I'd definitely like to work with Debian to make it possible for
Debian to ship signatures, but I'd imagine that being a long-term process.

> Also I'm not sure file capabilities is a good example, because those
> tend to be conditional, and when they fail to be set, the maintscripts
> tend to fallback to using set-user-ID on the binaries, and also because
> these are inherently non-portable.

I don't think you'd end up with file capabilities in any arch:all packages,
so the non-portability doesn't seem to be a huge problem - I was imagining
a way to express this in the source package, with tooling generating
something appropriate depending on the build architecture.