Skip to content

libarchive: Always ensure we have a label#2452

Closed
cgwalters wants to merge 1 commit intoostreedev:mainfrom
cgwalters:selinux-fallback
Closed

libarchive: Always ensure we have a label#2452
cgwalters wants to merge 1 commit intoostreedev:mainfrom
cgwalters:selinux-fallback

Conversation

@cgwalters
Copy link
Copy Markdown
Member

The way SELinux works, there isn't a concept of "something without
a label". There is the unlabeled_t type that the kernel makes
up when it finds no label on a mounted filesystem. But that's still
a label, it's just a kernel fallback.

When we go to import a tar archive, in most cases it won't
have SELinux labels, and even if it does, we want to overwrite it.

Now, the concept of a "whiteout" file arose relatively recently
as a special file type that's used in the container ecosystem alongside
overlayfs.

For some reason, presumably due to its special nature, at least the
Fedora SELinux policy doesn't define a label for it.

However, when it lands on disk, the kernel will still assign it
a default label.

And that's the problem - the computed checksum won't match, which
will appear as corruption.

One doesn't see this problem when e.g. extracting a tarball
temporarily to disk, and then committing it because the act of extracting
to disk will assign a label.

This problem was hit when importing container layers for ostree-container
work in ostree-rs-ext.

The way SELinux works, there isn't a concept of "something without
a label".  There *is* the `unlabeled_t` type that the kernel makes
up when it finds no label on a mounted filesystem.  But that's still
a label, it's just a kernel fallback.

When we go to import a tar archive, in most cases it won't
have SELinux labels, and even if it does, we want to overwrite it.

Now, the concept of a "whiteout" file arose relatively recently
as a special file type that's used in the container ecosystem alongside
overlayfs.

For some reason, presumably due to its special nature, at least the
Fedora SELinux policy doesn't define a label for it.

However, when it lands on disk, the kernel will still assign it
a default label.

And that's the problem - the computed checksum won't match, which
will appear as corruption.

One doesn't see this problem when e.g. extracting a tarball
temporarily to disk, and then committing it because the act of extracting
to disk will assign a label.

This problem was hit when importing container layers for ostree-container
work in ostree-rs-ext.
@travier
Copy link
Copy Markdown
Member

travier commented Oct 4, 2021

Reading on whiteout files, I found (https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt) which suggests that whiteout are 0:0 char devices. Still looking into the .wh format.

@travier
Copy link
Copy Markdown
Member

travier commented Oct 4, 2021

http://aufs.sourceforge.net/aufs.html > More reading for the .wh..... format for filenames.

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

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

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

Choose a reason for hiding this comment

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

cancellable, error))
return FALSE;

if (label)
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

Minor nit / question to make sure I understand the logic here but LGTM.

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

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?

@cgwalters
Copy link
Copy Markdown
Member Author

After a big of digging, the cause of this is likely
https://github.com/fedora-selinux/selinux-policy/blob/1715509773b3387a0c74423c05d53eb401d9b470/policy/modules/kernel/files.fc#L321

And in practice, it is quite simply broken to have anything /var/tmp in ostree at all. And more generally it looks like effectively all use of <<none>> in the policy is for things that shouldn't be in ostree at all.

So, on one hand the answer could be "don't do that", and that's basically what ostreedev/ostree-rs-ext#112 is doing.

Perhaps we should instead try to drive more opinions on this stuff into the ostree core.

@cgwalters cgwalters marked this pull request as draft October 5, 2021 19:55
@cgwalters
Copy link
Copy Markdown
Member Author

We can address this one in a different way.

@cgwalters cgwalters closed this Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants