Possible issues with dpkg SELinux support

Possible issues with dpkg SELinux support

[Guillem Jover]

Hi!

I've had on my TODO for some time to clear out some doubts about the
current dpkg SELinux support (which preceded my time), to be able to
fix some possible issues with it, and because I've never actually used
a SELinux enabled system and my knowledge about it is mostly
superficial. So here goes:

An error from the lsetfilecon_raw() call in [0] does not currently
end up in the installation process aborting (just an error message
printed out), I think this is wrong as I noted with the XXX there,
but I'd like your input on this, in case it actually needs to proceed
anyway. Otherwise I'd guess at least ENOTSUP should be ignored.

  [0] <http://anonscm.debian.org/gitweb/?p=dpkg/dpkg.git;a=blob;f=src/archives.c;h=4e363474607bd916813ce772b1e5c4c7359a76fc;hb=HEAD#l479>

And when invoking package maintainer scripts, dpkg does not set a
new execution context, like rpm does with rpm_execcon(), and while
skimming over the SELinux policy related to dpkg it seemed like
dpkg would need to do so.

I'd be fixing those, if needed, for dpkg 1.17.x.

thanks,
guillem

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Stephen Smalley]

On Thu, Nov 8, 2012 at 3:26 AM, Guillem Jover <guillem@debian.org> wrote:
> Hi!
>
> I've had on my TODO for some time to clear out some doubts about the
> current dpkg SELinux support (which preceded my time), to be able to
> fix some possible issues with it, and because I've never actually used
> a SELinux enabled system and my knowledge about it is mostly
> superficial. So here goes:
>
> An error from the lsetfilecon_raw() call in [0] does not currently
> end up in the installation process aborting (just an error message
> printed out), I think this is wrong as I noted with the XXX there,
> but I'd like your input on this, in case it actually needs to proceed
> anyway. Otherwise I'd guess at least ENOTSUP should be ignored.
>
>   [0] <http://anonscm.debian.org/gitweb/?p=dpkg/dpkg.git;a=blob;f=src/archives.c;h=4e363474607bd916813ce772b1e5c4c7359a76fc;hb=HEAD#l479>
>
> And when invoking package maintainer scripts, dpkg does not set a
> new execution context, like rpm does with rpm_execcon(), and while
> skimming over the SELinux policy related to dpkg it seemed like
> dpkg would need to do so.
>
> I'd be fixing those, if needed, for dpkg 1.17.x.

Agree that lsetfilecon failure other than EOPNOTSUPP should abort
package installation if SELinux is enabled.  Note that matchpathcon
and friends are deprecated interfaces; consider converting to
selabel_open and friends instead, as has already been done in rpm.
Some mechanism to allow package scriptlets to run in a different
context than the package manager would be helpful, but rpm_execcon()
may not be a very good example.  The Tizen folks have been working on
a more general architecture for rpm security plugins that may be
relevant/helpful as a guide, see prior discussions on selinux list and
rpm-maint.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Guillem Jover]

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

On Thu, 2012-11-08 at 10:49:59 -0500, Stephen Smalley wrote:
> Agree that lsetfilecon failure other than EOPNOTSUPP should abort
> package installation if SELinux is enabled.  Note that matchpathcon
> and friends are deprecated interfaces; consider converting to
> selabel_open and friends instead, as has already been done in rpm.

Great, done now in the attached patch. Would be nice if the currently
obsolete/deprecated functions could be marked as such on the man pages
and possibly on the header files with compiler attributes, as this was
not obvious until you pointed it out. I still have some questions and
doubts though, if you don't mind:


Is the "<<none>>" check needed at all? This has bothered me for a
while, and it's not clear if calling lsetfilecon_raw() with that would
DTRT, or if selabel_lookup_raw() would never return that to start with.

Should the code be handling selinux_status_updated(), and reopening
the selabel_handle in such case? If so, how often, per extracted file,
per package processed (probably this usually happens only after
upgrading the refpolicy package?), some other time(s)?

Should the code be ignoring some other SELinux errors or considering
them warnings when running in non-enforcing mode, or is that already
handled internally by the SELinux interfaces?

Is ignoring errors from lsetfilecon_raw() enough of a grave issue as
to warrant a stable dpkg update (can it create security issues, or just
wrongful or too restrictive unpacks)? (I'd be preparing updates for the
current Debian stable and the just-being-prepared-stable releases in
such case.)


> Some mechanism to allow package scriptlets to run in a different
> context than the package manager would be helpful, but rpm_execcon()
> may not be a very good example.  The Tizen folks have been working on
> a more general architecture for rpm security plugins that may be
> relevant/helpful as a guide, see prior discussions on selinux list and
> rpm-maint.

If you mean the discussion started by Elena, I agree that for dpkg too,
it would be nice to have a more generic “plugin” interface, but for now,
given that there's no one requesting it, I'd rather just make SELinux
behave correctly, and modularize this later on, as the code would need
quite some cleanup/reorganization first in any case.

Also Elena's proposed plugin system did not seem to be directly related
to SELinux, so I've ended adapting rpm_execcon() almost verbatim to the
dpkg case. But something that came to mind is if you think it would make
sense to refactor that function (I've read it's supposed to disappear
anyway) into something slightly more generic so that it could be used by
at least both rpm and dpkg (and possibly other package managers or even
programs invoking helper programs). Something ressembling the adaptation
that I've made in the attached patch, but in addition passing to it (at
least) the fallback context type, it could perhaps have a signature
similar to something like:

  int setexecfilecon(const char *filename, const char *fallback_type);

and be called like:

  rc = setexecfilecon(path, "dpkg_script_t");
  ...
  rc = execve(path, argv, envp);

?


If the attached patch looks fine in principle, I'd ask the Debian
SELinux folks for some testing (as I've only build tested it), and if
they need to somehow adapt the Debian SELinux refpolicy.

thanks,
guillem

[-- Attachment #2: dpkg-selinux-rework.patch --]
[-- Type: text/x-diff, Size: 5359 bytes --]

diff --git a/src/archives.c b/src/archives.c
index 4e36347..90a528a 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -59,6 +59,7 @@
 
 #ifdef WITH_SELINUX
 #include <selinux/selinux.h>
+#include <selinux/label.h>
 #endif
 
 #include "filesdb.h"
@@ -476,11 +477,38 @@ tarobject_set_perms(struct tar_entry *te, const char *path, struct file_stat *st
   }
 }
 
+#ifdef WITH_SELINUX
+static struct selabel_handle *
+dpkg_get_selabel_handle(void)
+{
+  static int selinux_enabled = -1;
+  static struct selabel_handle *sehandle;
+
+  if (selinux_enabled >= 0)
+    return sehandle;
+
+  /* Set selinux_enabled if it is not already set (singleton). */
+  selinux_enabled = (is_selinux_enabled() > 0);
+
+  if (!selinux_enabled)
+    return NULL;
+
+  sehandle = selabel_open(SELABEL_CTX_FILE, NULL, 0);
+  if (sehandle == NULL)
+    ohshite(_("cannot get security labeling handle"));
+
+  /* XXX: We could use selinux_set_callback() to redirect the errors from
+   * the other SELinux calls, but that does not seem worth it right now. */
+
+  return sehandle;
+}
+#endif
+
 static void
 tarobject_set_se_context(const char *matchpath, const char *path, mode_t mode)
 {
 #ifdef WITH_SELINUX
-  static int selinux_enabled = -1;
+  struct selabel_handle *sehandle;
   security_context_t scontext = NULL;
   int ret;
 
@@ -488,33 +516,28 @@ tarobject_set_se_context(const char *matchpath, const char *path, mode_t mode)
   if ((mode & S_IFMT) == 0)
     return;
 
-  /* Set selinux_enabled if it is not already set (singleton). */
-  if (selinux_enabled < 0) {
-    selinux_enabled = (is_selinux_enabled() > 0);
-
-    /* Do not translate from computer to human readable forms, to avoid
-     * issues when mcstransd has disappeared during the unpack process. */
-    if (selinux_enabled)
-      set_matchpathcon_flags(MATCHPATHCON_NOTRANS);
-  }
-
-  /* If SE Linux is not enabled just do nothing. */
-  if (!selinux_enabled)
+  /* If SELinux is not enabled just do nothing. */
+  sehandle = dpkg_get_selabel_handle();
+  if (sehandle == NULL)
     return;
 
-  /* XXX: Well, we could use set_matchpathcon_printf() to redirect the
-   * errors from the following bit, but that seems too much effort. */
+  /*
+   * We use the _raw function variants here so that no translation happens
+   * from computer to human readable forms, to avoid issues when mcstransd
+   * has disappeared during the unpack process.
+   *
+   * And we do nothing if we can't figure out what the context is, or if it
+   * has no context; in which case the default context shall be applied.
+   */
 
-  /* Do nothing if we can't figure out what the context is, or if it has
-   * no context; in which case the default context shall be applied. */
-  ret = matchpathcon(matchpath, mode & S_IFMT, &scontext);
+  ret = selabel_lookup_raw(sehandle, &scontext, matchpath, mode & S_IFMT);
   if (ret == -1 || (ret == 0 && scontext == NULL))
     return;
 
   if (strcmp(scontext, "<<none>>") != 0) {
-    if (lsetfilecon_raw(path, scontext) < 0)
-      /* XXX: This might need to be fatal instead!? */
-      perror("Error setting security context for next file object:");
+    ret = lsetfilecon_raw(path, scontext);
+    if (ret < 0 && ret != ENOTSUP)
+      ohshite(_("cannot set security context for file object '%s'"), path);
   }
 
   freecon(scontext);
diff --git a/src/script.c b/src/script.c
index c03441b..2aeaf40 100644
--- a/src/script.c
+++ b/src/script.c
@@ -31,6 +31,12 @@
 #include <unistd.h>
 #include <stdlib.h>
 
+#ifdef WITH_SELINUX
+#include <selinux/selinux.h>
+#include <selinux/flask.h>
+#include <selinux/context.h>
+#endif
+
 #include <dpkg/i18n.h>
 #include <dpkg/dpkg.h>
 #include <dpkg/dpkg-db.h>
@@ -137,6 +143,59 @@ preexecscript(struct command *cmd)
 }
 
 static int
+maintscript_set_exec_context(struct command *cmd)
+{
+	int rc = 0;
+#ifdef WITH_SELINUX
+	security_context_t curcon = NULL, newcon = NULL, filecon = NULL;
+	context_t tmpcon = NULL;
+
+	if (is_selinux_enabled() < 1)
+		return 0;
+
+	rc = getcon(&curcon);
+	if (rc < 0)
+		goto out;
+
+	rc = getfilecon(cmd->filename, &filecon);
+	if (rc < 0)
+		goto out;
+
+	rc = security_compute_create(curcon, filecon, SECCLASS_PROCESS, &newcon);
+	if (rc < 0)
+		goto out;
+
+	if (strcmp(curcon, newcon) == 0) {
+		/* No default transition, use dpkg_script_t for now. */
+		rc = -1;
+		tmpcon = context_new(curcon);
+		if (tmpcon == NULL)
+			goto out;
+		if (context_type_set(tmpcon, "dpkg_script_t"))
+			goto out;
+		freecon(newcon);
+		newcon = strdup(context_str(tmpcon));
+		if (newcon == NULL)
+			goto out;
+		rc = 0;
+	}
+
+	rc = setexeccon(newcon);
+
+out:
+	if (rc < 0 && security_getenforce() == 0)
+		rc = 0;
+
+	context_free(tmpcon);
+	freecon(newcon);
+	freecon(curcon);
+	freecon(filecon);
+#endif
+
+	return rc < 0 ? rc : 0;
+}
+
+static int
 do_script(struct pkginfo *pkg, struct pkgbin *pkgbin,
           struct command *cmd, struct stat *stab, int warn)
 {
@@ -156,6 +215,11 @@ do_script(struct pkginfo *pkg, struct pkgbin *pkgbin,
 			ohshite(_("unable to setenv for maintainer script"));
 
 		cmd->filename = cmd->argv[0] = preexecscript(cmd);
+
+		if (maintscript_set_exec_context(cmd) < 0)
+			ohshite(_("cannot set security execution context for "
+			          "maintainer script"));
+
 		command_exec(cmd);
 	}
 	subproc_signals_setup(cmd->name); /* This does a push_cleanup(). */

Re: Possible issues with dpkg SELinux support

[Guillem Jover]

On Sat, 2012-11-10 at 04:19:32 +0100, Guillem Jover wrote:
> Is the "<<none>>" check needed at all? This has bothered me for a
> while, and it's not clear if calling lsetfilecon_raw() with that would
> DTRT, or if selabel_lookup_raw() would never return that to start with.

Oh, rechecking now selabel_file(5) it seems like selabel_lookup_raw()
might never return "<<none>>", so I've preventively removed locally
that check now.

thanks,
guillem

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/11/2012 04:18 AM, Guillem Jover wrote:
> On Sat, 2012-11-10 at 04:19:32 +0100, Guillem Jover wrote:
>> Is the "<<none>>" check needed at all? This has bothered me for a while,
>> and it's not clear if calling lsetfilecon_raw() with that would DTRT, or
>> if selabel_lookup_raw() would never return that to start with.
> 
> Oh, rechecking now selabel_file(5) it seems like selabel_lookup_raw() might
> never return "<<none>>", so I've preventively removed locally that check
> now.
> 
> thanks, guillem
> 
> -- This message was distributed to subscribers of the selinux mailing
> list. If you no longer wish to subscribe, send mail to
> majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes
> as the message.
> 
I don't agree that matchpatchcon is obsoleted, I see it as more of a helper
function.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCg868ACgkQrlYvE4MpobMw/wCdEFI4dQvsWXk/Lc0sNuPbM4Sd
xyYAnAzG9LYOGg9811tEjpUITij1SF8D
=s8TS
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Stephen Smalley]

On 11/09/2012 10:19 PM, Guillem Jover wrote:
> Is the "<<none>>" check needed at all? This has bothered me for a
> while, and it's not clear if calling lsetfilecon_raw() with that would
> DTRT, or if selabel_lookup_raw() would never return that to start with.

As you already saw, this check is no longer necessary.

> Should the code be handling selinux_status_updated(), and reopening
> the selabel_handle in such case? If so, how often, per extracted file,
> per package processed (probably this usually happens only after
> upgrading the refpolicy package?), some other time(s)?

I believe rpm does selinux_status_open() at the beginning of the 
transaction and selinux_status_close() at the end of the transaction, 
and during the transaction, checks selinux_status_updated() once per 
package processed to see whether it needs to re-open the selabel handle. 
  This ensures that rpm gets a fresh view of file_contexts if it is 
modified during the transaction by a semodule or semanage command.

> Should the code be ignoring some other SELinux errors or considering
> them warnings when running in non-enforcing mode, or is that already
> handled internally by the SELinux interfaces?

This should generally be done within libselinux rather than in the caller.

> Is ignoring errors from lsetfilecon_raw() enough of a grave issue as
> to warrant a stable dpkg update (can it create security issues, or just
> wrongful or too restrictive unpacks)? (I'd be preparing updates for the
> current Debian stable and the just-being-prepared-stable releases in
> such case.)

It could be a security issue, although obviously no current Debian 
SELinux users were relying on it.

> Also Elena's proposed plugin system did not seem to be directly related
> to SELinux, so I've ended adapting rpm_execcon() almost verbatim to the
> dpkg case. But something that came to mind is if you think it would make
> sense to refactor that function (I've read it's supposed to disappear
> anyway) into something slightly more generic so that it could be used by
> at least both rpm and dpkg (and possibly other package managers or even
> programs invoking helper programs). Something ressembling the adaptation
> that I've made in the attached patch, but in addition passing to it (at
> least) the fallback context type, it could perhaps have a signature
> similar to something like:
>
>    int setexecfilecon(const char *filename, const char *fallback_type);
>
> and be called like:
>
>    rc = setexecfilecon(path, "dpkg_script_t");
>    ...
>    rc = execve(path, argv, envp);
>
> ?

That seems reasonable, except that we'd ideally like to get rid of the 
hardcoded type altogether and get it from a configuration, preferably 
supplied by the policy.  But the problem of course is what to do  before 
the policy package is installed.

> If the attached patch looks fine in principle, I'd ask the Debian
> SELinux folks for some testing (as I've only build tested it), and if
> they need to somehow adapt the Debian SELinux refpolicy.

Looks mostly correct to me, but the error check for lsetfilecon_raw() 
should be:
	if (ret < 0 && errno != EOPNOTSUPP)



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Guillem Jover]

On Tue, 2012-11-13 at 14:16:08 -0500, Stephen Smalley wrote:
> On 11/09/2012 10:19 PM, Guillem Jover wrote:
> >Should the code be handling selinux_status_updated(), and reopening
> >the selabel_handle in such case? If so, how often, per extracted file,
> >per package processed (probably this usually happens only after
> >upgrading the refpolicy package?), some other time(s)?
> 
> I believe rpm does selinux_status_open() at the beginning of the
> transaction and selinux_status_close() at the end of the
> transaction, and during the transaction, checks
> selinux_status_updated() once per package processed to see whether
> it needs to re-open the selabel handle.  This ensures that rpm gets
> a fresh view of file_contexts if it is modified during the
> transaction by a semodule or semanage command.

Ok, I'll handle this in the code then.

> >Is ignoring errors from lsetfilecon_raw() enough of a grave issue as
> >to warrant a stable dpkg update (can it create security issues, or just
> >wrongful or too restrictive unpacks)? (I'd be preparing updates for the
> >current Debian stable and the just-being-prepared-stable releases in
> >such case.)
> 
> It could be a security issue, although obviously no current Debian
> SELinux users were relying on it.

Right, but I'd assume the users might just not know this could happen,
so as this might be a security issue, I think I'll be preparing those
updates.

> >Also Elena's proposed plugin system did not seem to be directly related
> >to SELinux, so I've ended adapting rpm_execcon() almost verbatim to the
> >dpkg case. But something that came to mind is if you think it would make
> >sense to refactor that function (I've read it's supposed to disappear
> >anyway) into something slightly more generic so that it could be used by
> >at least both rpm and dpkg (and possibly other package managers or even
> >programs invoking helper programs). Something ressembling the adaptation
> >that I've made in the attached patch, but in addition passing to it (at
> >least) the fallback context type, it could perhaps have a signature
> >similar to something like:
> >
> >   int setexecfilecon(const char *filename, const char *fallback_type);
> >
> >and be called like:
> >
> >   rc = setexecfilecon(path, "dpkg_script_t");
> >   ...
> >   rc = execve(path, argv, envp);
> >
> >?
> 
> That seems reasonable, except that we'd ideally like to get rid of
> the hardcoded type altogether and get it from a configuration,
> preferably supplied by the policy.  But the problem of course is
> what to do  before the policy package is installed.

And the type would be applied based on the filename? That makes sense,
but what about scripts stored on a db that need to be written into
temporary files before executing them (as I think rpm is doing right
now)? And the pre-policy bootstrapping problem is tricky indeed. So
while the hardcoded type might not be pretty, it's at least hardcoded
on the caller, which seems better than on the function itself. :)
Any better ideas?

> >If the attached patch looks fine in principle, I'd ask the Debian
> >SELinux folks for some testing (as I've only build tested it), and if
> >they need to somehow adapt the Debian SELinux refpolicy.
> 
> Looks mostly correct to me, but the error check for
> lsetfilecon_raw() should be:
> 	if (ret < 0 && errno != EOPNOTSUPP)

Hmm, I got the ENOTSUP error from the lsetfilecon(3) man page, also
that's the error code that makes sense given its description of “not
supported”, while EOPNOTSUPP is “operation not supported on socket”,
although on Linux it does not make a difference as they are aliased
to the same value. In any case checking now the code it seems it's
implemented in means of lsetxattr(2) which is also documented as
returning ENOTSUP. Grepping though the libselinux code there's the
*getfilecon functions which seems to be setting EOPNOTSUPP, maybe
that's why you mentioned it? It might be a good idea to change those
to ENOTSUP, if only for pedantry? :)

And thanks for the answers and review.

regards,
guillem


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: Possible issues with dpkg SELinux support

[Stephen Smalley]

On 11/13/2012 03:13 PM, Guillem Jover wrote:
>> That seems reasonable, except that we'd ideally like to get rid of
>> the hardcoded type altogether and get it from a configuration,
>> preferably supplied by the policy.  But the problem of course is
>> what to do  before the policy package is installed.
>
> And the type would be applied based on the filename? That makes sense,
> but what about scripts stored on a db that need to be written into
> temporary files before executing them (as I think rpm is doing right
> now)? And the pre-policy bootstrapping problem is tricky indeed. So
> while the hardcoded type might not be pretty, it's at least hardcoded
> on the caller, which seems better than on the function itself. :)
> Any better ideas?

No, I'm afraid not.  So for now, I guess your approach is the best option.

>>> If the attached patch looks fine in principle, I'd ask the Debian
>>> SELinux folks for some testing (as I've only build tested it), and if
>>> they need to somehow adapt the Debian SELinux refpolicy.
>>
>> Looks mostly correct to me, but the error check for
>> lsetfilecon_raw() should be:
>> 	if (ret < 0 && errno != EOPNOTSUPP)
>
> Hmm, I got the ENOTSUP error from the lsetfilecon(3) man page, also
> that's the error code that makes sense given its description of “not
> supported”, while EOPNOTSUPP is “operation not supported on socket”,
> although on Linux it does not make a difference as they are aliased
> to the same value. In any case checking now the code it seems it's
> implemented in means of lsetxattr(2) which is also documented as
> returning ENOTSUP. Grepping though the libselinux code there's the
> *getfilecon functions which seems to be setting EOPNOTSUPP, maybe
> that's why you mentioned it? It might be a good idea to change those
> to ENOTSUP, if only for pedantry? :)

I suppose ENOTSUP might be more portable.  We don't exactly support 
libselinux on kernels other than Linux ;)  But I guess that does matter 
for dpkg.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.