Bug#498438: Wrong SE Linux labels on some files under /var/lib/dpkg when installing conffile
On Sat, 2010-02-06 at 22:22:56 +1100, Russell Coker wrote:
On Sat, 6 Feb 2010, Guillem Jover guil...@debian.org wrote:
Are the SE Linux file contexts handled like normal file attributes,
that get propagated with rename(2) and only get reset on unlink(2) or
with an explicit SE Linux call? Or are they handled differently and
do get reset on rename(2) (I thought SE Linux was not path based)?
rename(2) and link(2) make no difference to the label. unlink(2) doesn't
change it either. Access checks are performed on open handles to unlinked
files - when a process calls exec() file handles may be closed by SE Linux
because the process transitioned to a new domain and is no longer granted
access to the file (or isn't permitted to inherit the file handle).
fsetfilecon(3) may be called on an unlinked file, as access controls in SE
Linux are performed on all accesses if an unlinked file has it's context
changed then processes that are accessing it may suddenly get their access
denied with EPERM.
Yeah, sorry for the confusing question, there was an implied question
as if the contexts would be remembered based on their paths and applied
whenever seen (clarified now anyway). It's just that the current code
and comments in there made it more difficult to understand how this
actually works.
(Or I guess in other words are they associated to the inode or the
dentry?)
It's always been the Inode. There is no other sane way to do it.
Thanks, that clarifies it perfectly now.
If it's the former then yes the extra setfscreatecon() at the end of
tarobject() does not seem to make sense, if it's the latter we have
an additional problem, as if tarobject gets interrupted and then it
tries to restore the .dpkg-tmp backup then the context will be wrong
for that one as rename(2) would reset the context.
rename(2) does not affect the context.
Ok, given my current understanding I've just reworked the current code
to use lsetfilecon() instead (patch attached), there's no need to
restore anything anymore on error. Completely untested as I don't have
or use SE Linux anywhere, but if it seems sane and works fine I'll just
apply it.
Also should a failure from lsetfilecon() ignoring ENOTSUP, be considered
fatal then? I'd say yes but maybe there's some reason not to?
thanks,
guillem
diff --git a/src/archives.c b/src/archives.c
index 0d1d9d4..7a3d43c 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -51,8 +51,6 @@
#ifdef WITH_SELINUX
#include selinux/selinux.h
-static int selinux_enabled=-1;
-static security_context_t scontext= NULL;
#endif
#include filesdb.h
@@ -256,6 +254,40 @@ static void newtarobject_allmodes(const char *path, struct TarInfo *ti, struct f
newtarobject_utime(path,ti);
}
+static void
+set_path_selinux_context(const char *matchpath, const char *path, mode_t mode)
+{
+#ifdef WITH_SELINUX
+ static int selinux_enabled = -1;
+ security_context_t scontext = NULL;
+ int ret;
+
+ /* Set selinux_enabled if it is not already set (singleton). */
+ if (selinux_enabled 0)
+selinux_enabled = (is_selinux_enabled() 0);
+
+ /* If SE Linux is not enabled just do nothing. */
+ if (!selinux_enabled)
+return;
+
+ /* XXX: Well, we could use set_matchpathcon_printf to redirect the
+ * errors from the following bit, but that seems too much effort. */
+
+ /* 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);
+ if (ret == -1 || (ret == 0 scontext == NULL))
+return;
+
+ if (strcmp(scontext, none) != 0) {
+if (lsetfilecon(path, scontext) 0)
+ perror(Error setting security context for next file object:);
+ }
+
+ freecon(scontext);
+#endif /* WITH_SELINUX */
+}
+
void setupfnamevbs(const char *filename) {
fnamevb.used= fnameidlu;
varbufaddstr(fnamevb,filename);
@@ -593,38 +625,6 @@ int tarobject(struct TarInfo *ti) {
* its original filename.
*/
-#ifdef WITH_SELINUX
- /* Set selinux_enabled if it is not already set (singleton) */
- if (selinux_enabled 0)
-selinux_enabled = (is_selinux_enabled() 0);
-
- /* Since selinux is enabled, try and set the context */
- if (selinux_enabled 0) {
-/*
- * well, we could use
- * void set_matchpathcon_printf(void (*f)(const char *fmt, ...));
- * to redirect the errors from the following bit, but that
- * seems too much effort.
- */
-
-/*
- * 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.
- */
-if( ! ((matchpathcon(fnamevb.buf,
- (nifd-namenode-statoverride ?
- nifd-namenode-statoverride-mode : ti-Mode)
- ~S_IFMT, scontext) != 0) ||
- (strcmp(scontext, none) == 0)))
- {
- if(setfscreatecon(scontext) 0)
-
Bug#498438: Wrong SE Linux labels on some files under /var/lib/dpkg when installing conffile
Hi Russell!
[ CCing Manoj as the original author of the dpkg SE Linux support. ]
On Tue, 2009-02-17 at 13:32:26 +1100, Russell Coker wrote:
If dpkg is not going to abort on an error (not sure when/why this happens)
such that ohshit() doesn't abort, then we still have a problem.
I experimented with modifying the ohshit() function, but that meant that
dpkg-query needed to be linked against libselinux. One possible solution to
that would be to have source file that contains ohshit() compiled twice, once
for dpkg-query (without SE Linux support) and once with SE Linux support for
dpkg.
The solution to this is to install a cleanup handler, which resets the
context on error. I'm attaching a patch that should fix this in all
cases, but there's some things I don't understand in the current SE Linux
support in dpkg, which I'd like to understand before applying it.
Ok, so this is my basic understanding of how SE Linux works here (my
terminology might not be accurate), please correct were appropriate.
AFAICS there's at least two ways to apply a context to a file, one is
to set the current file system context for new created objects (via
setfscreatecon) and the other is to explicitly set the context for an
existing file (via setfilecon and friends). Letting a file with the
default context should not reduce the security, it might just not allow
others to access it.
Are the SE Linux file contexts handled like normal file attributes,
that get propagated with rename(2) and only get reset on unlink(2) or
with an explicit SE Linux call? Or are they handled differently and
do get reset on rename(2) (I thought SE Linux was not path based)?
(Or I guess in other words are they associated to the inode or the
dentry?)
If it's the former then yes the extra setfscreatecon() at the end of
tarobject() does not seem to make sense, if it's the latter we have
an additional problem, as if tarobject gets interrupted and then it
tries to restore the .dpkg-tmp backup then the context will be wrong
for that one as rename(2) would reset the context.
regards,
guillem
diff --git a/src/archives.c b/src/archives.c
index 77d67ce..ed2cb9e 100644
--- a/src/archives.c
+++ b/src/archives.c
@@ -353,6 +353,17 @@ linktosameexistingdir(const struct TarInfo *ti, const char *fname,
return true;
}
+static void
+cu_selinux_context(int argc, void **argv)
+{
+#ifdef WITH_SELINUX
+ /* If selinux is enabled, restore the default security context. */
+ if (selinux_enabled 0)
+if (setfscreatecon(NULL) 0)
+ perror(Error restoring default security context:);
+#endif /* WITH_SELINUX */
+}
+
int tarobject(struct TarInfo *ti) {
static struct varbuf conffderefn, hardlinkfn, symlinkfn;
static int fd;
@@ -623,7 +634,7 @@ int tarobject(struct TarInfo *ti) {
}
}
#endif /* WITH_SELINUX */
-
+ push_cleanup(cu_selinux_context, ~0, NULL, 0, 0);
/* Extract whatever it is as .dpkg-new ... */
switch (ti-Type) {
@@ -724,6 +735,7 @@ int tarobject(struct TarInfo *ti) {
if (nifd-namenode-flags fnnf_new_conff) {
debug(dbg_conffdetail,tarobject conffile extracted);
nifd-namenode-flags |= fnnf_elide_other_lists;
+pop_cleanup(ehflag_normaltidy); /* cu_selinux_context(). */
return 0;
}
@@ -792,15 +804,7 @@ int tarobject(struct TarInfo *ti) {
nifd-namenode-flags |= fnnf_placed_on_disk;
-#ifdef WITH_SELINUX
- /*
- * if selinux is enabled, restore the default security context
- */
- if (selinux_enabled 0)
-if(setfscreatecon(NULL) 0)
- perror(Error restoring default security context:);
-#endif /* WITH_SELINUX */
-
+ pop_cleanup(ehflag_normaltidy); /* cu_selinux_context(). */
nifd-namenode-flags |= fnnf_elide_other_lists;
Bug#498438: Wrong SE Linux labels on some files under /var/lib/dpkg when installing conffile
On Sat, 6 Feb 2010, Guillem Jover guil...@debian.org wrote: Ok, so this is my basic understanding of how SE Linux works here (my terminology might not be accurate), please correct were appropriate. AFAICS there's at least two ways to apply a context to a file, one is to set the current file system context for new created objects (via setfscreatecon) and the other is to explicitly set the context for an existing file (via setfilecon and friends). Letting a file with the default context should not reduce the security, it might just not allow others to access it. If you don't do anything special then the default context for the created file will be based on the context of the directory that it is created in. The default if there is no specific policy is for the type to match the type of the directory. There can be special policy saying when a process of domain X creates a file in directory type Y then use type Z. In addition a process can request that a certain context be used for creating a file, the policy can deny such a request or deny it based on the type of object being created. After a filesystem object is created it can be relabeled. Leaving a file with a default context can mean that SUID type functionality is not applied, this can mean that privileges are not dropped. Are the SE Linux file contexts handled like normal file attributes, that get propagated with rename(2) and only get reset on unlink(2) or with an explicit SE Linux call? Or are they handled differently and do get reset on rename(2) (I thought SE Linux was not path based)? rename(2) and link(2) make no difference to the label. unlink(2) doesn't change it either. Access checks are performed on open handles to unlinked files - when a process calls exec() file handles may be closed by SE Linux because the process transitioned to a new domain and is no longer granted access to the file (or isn't permitted to inherit the file handle). fsetfilecon(3) may be called on an unlinked file, as access controls in SE Linux are performed on all accesses if an unlinked file has it's context changed then processes that are accessing it may suddenly get their access denied with EPERM. (Or I guess in other words are they associated to the inode or the dentry?) It's always been the Inode. There is no other sane way to do it. If it's the former then yes the extra setfscreatecon() at the end of tarobject() does not seem to make sense, if it's the latter we have an additional problem, as if tarobject gets interrupted and then it tries to restore the .dpkg-tmp backup then the context will be wrong for that one as rename(2) would reset the context. rename(2) does not affect the context. -- russ...@coker.com.au http://etbe.coker.com.au/ My Main Blog http://doc.coker.com.au/ My Documents Blog -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#498438: Wrong SE Linux labels on some files under /var/lib/dpkg when installing conffile
Package: dpkg Version: 1.14.20 Severity: normal In src/archive.c the SE Linux context for file creation is set at about line 640. It's set again at about line 795 for reasons I don't understand (this second setting doesn't respect the fact that scontext might have a value of none but this is a minor issue). Then on line 824 it is unset. The problem is that on line 744 the function may return if the file is a conffile. This means that all further files created until dpkg sets the context again with setfscreatecon() get the same label. A consequence of this is that often files under /var/lib/dpkg will have the type etc_t (the type used for most files under /etc). A minimal solution to this would be to have the following before the return statement at around line 744: #ifdef WITH_SELINUX if (selinux_enabled 0) if(setfscreatecon(NULL) 0) perror(Error restoring default security context:); #endif I'm also concerned that the ohshit() function calls might result in the code later creating files with the wrong context. It wouldn't do any harm to have code such as the above inside ohshit() to deal with this. Calling setfscreatecon(NULL) an extra few times is not going to do any harm. -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
