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;
