Skip to content

libarchive: Always ensure we have a label #2452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cgwalters wants to merge 1 commit into from
Closed

libarchive: Always ensure we have a label #2452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/libostree/ostree-repo-libarchive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "ostree.h"
#include "ostree-core-private.h"
#include "ostree-repo-private.h"
#include "ostree-sepolicy-private.h"

#ifdef HAVE_LIBARCHIVE
#include <archive.h>
Expand Down Expand Up @@ -167,8 +168,8 @@ builder_add_label (GVariantBuilder *builder,
if (!sepolicy)
return TRUE;

if (!ostree_sepolicy_get_label (sepolicy, path, mode, &label,
cancellable, error))
if (!_ostree_sepolicy_require_label (sepolicy, path, mode, &label,
cancellable, error))
return FALSE;

if (label)
Copy link
Copy Markdown
Member

@jlebon jlebon Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should become an assert now, I guess?

Expand Down
5 changes: 5 additions & 0 deletions src/libostree/ostree-sepolicy-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ gboolean _ostree_sepolicy_preparefscreatecon (OstreeSepolicyFsCreatecon *con,
guint32 mode,
GError **error);

gboolean
_ostree_sepolicy_require_label (OstreeSePolicy *policy, const char *relpath,
guint32 unix_mode, char **out_label,
GCancellable *cancellable, GError **error);

GVariant *_ostree_filter_selinux_xattr (GVariant *xattrs);

G_END_DECLS
19 changes: 19 additions & 0 deletions src/libostree/ostree-sepolicy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,25 @@ ostree_sepolicy_get_label (OstreeSePolicy *self,
return TRUE;
}

// If policy doesn't specify a label, try a fallback.
gboolean
_ostree_sepolicy_require_label (OstreeSePolicy *policy, const char *relpath,
guint32 unix_mode, char **out_label,
GCancellable *cancellable, GError **error)
{
char *label = NULL;
if (!ostree_sepolicy_get_label (policy, relpath, unix_mode, &label, cancellable, error))
return FALSE;
if (!label)
{
if (!ostree_sepolicy_get_label (policy, "/usr/share/some-generic-thing", unix_mode, &label, cancellable, error))
Copy link
Copy Markdown
Member

@jlebon jlebon Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more appropriate thing to do here would be to keep going up from relpath and stopping until we have a parent dir which has a label. This more closely matches the default heuristic used by SELinux.

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. In the case of this /var/tmp file, yes. On the other hand, if someone is trying to add /foobar, I don't think we want it to be root_t.

Copy link
Copy Markdown
Member

@jlebon jlebon Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be safe to trust the policy. E.g. /foobar would get caught by https://github.com/fedora-selinux/selinux-policy/blob/1715509773b3387a0c74423c05d53eb401d9b470/policy/modules/kernel/files.fc#L5.

Copy link
Copy Markdown
Member

@travier travier Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this /usr/share/i-will-never-exist?

return FALSE;
}
*out_label = label;
return TRUE;
}


/**
* ostree_sepolicy_restorecon:
* @self: Self
Expand Down
5 changes: 4 additions & 1 deletion tests/kolainst/destructive/itest-label-selinux.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ echo "ok commit --selinux-policy-from-base"

rm rootfs -rf
mkdir rootfs
mkdir -p rootfs/usr/{bin,lib,etc}
mkdir -p rootfs/usr/{bin,lib,etc} rootfs/var/tmp
# Fedora's SELinux policy doesn't give whiteouts a label, so this tests our force-labeling
touch rootfs/var/tmp/.wh..wh..opq
echo 'somebinary' > rootfs/usr/bin/somebinary
ls -Z rootfs/usr/bin/somebinary > lsz.txt
assert_not_file_has_content lsz.txt ':bin_t:'
Expand All @@ -116,4 +118,5 @@ tar -C rootfs -cf rootfs.tar .
ostree commit -b newbase --selinux-policy / --tree=tar=rootfs.tar
ostree ls -X newbase /usr/bin/somebinary > newls.txt
assert_file_has_content newls.txt ':bin_t:'
ostree fsck
echo "ok commit --selinux-policy with --tree=tar"