Skip to content
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.

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

Initial fs-verity support #1959

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@cgwalters
Copy link
Member

cgwalters commented Oct 25, 2019

Using fs-verity is natural for OSTree because it's file-based,
as opposed to block based (like dm-verity). This only covers
files - not symlinks or directories. And we clearly need to
have integrity for the deployment directories at least.

More background on fs-verity:

So making this truly secure would need a lot more work. Nevertheless,
I think it's time to start experimenting with it. Among other things,
it does finally add an API that makes files immutable, which will
help against some accidental damage.

@openshift-ci-robot

This comment has been minimized.

Copy link
Collaborator

openshift-ci-robot commented Oct 25, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 28, 2019

I think Android is basically going to continue using dm-verity for the base OS, and use fs-verity just for apps which come as single .zip files.

So they don't have the problem of verifying full filesystem trees.

I was thinking about this some more, and a rather gross hack might be to store all of our directories/symlinks as effectively tarballs in the real root (that are fs-verity protected), then on boot we unpack them into a tmpfs, and use overlayfs with the tmpfs and the underlying fs-verity files.

But clearly what we really want is an extension to fs-verity that lets us "seal" a directory and set of symlinks in it too.

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 30, 2019

Continuing my half-baked ideation...

Maybe we can use a dual-partition dm-verity to hold the directories/symlinks, and an overlayfs pointing to the underlying real rootfs with fs-verity files?

But...what's bugging me here is that if an attacker gains CAP_SYS_ADMIN and the ability to write directly to the underlying block device, then on reboot, even though the files are fs-verity protected, all of the rest of the disk (starting from the root directory) isn't. And it's known that mounting malicious filesystems is unsafe. I wonder if the fs-verity developers (or potential Android/ChromeOS use of it) have considered this.

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 30, 2019

Also, one thing we can and should do is use fs-verity for /boot - this PR needs to do the same thing for /boot that it does for /ostree/repo/objects (at least when /boot is a separate FS). For Fedora CoreOS we could probably turn on ext4+verity for /boot by default even if we don't make the switch for /. That could at least help us prove out some of the secure/trusted boot bits combining with fs-verity even if we aren't doing it for the rest of userspace yet.

@cgwalters cgwalters force-pushed the cgwalters:ostree-verity branch from 21c36dd to 957f05b Oct 31, 2019
@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 31, 2019

OK a number of tweaks, biggest is that we now also do fsverity for files in /boot.

Also now:
Depends: https://gitlab.gnome.org/GNOME/libglnx/merge_requests/11

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 31, 2019

I think the remaining files to cover are the config file (circular, need to write it then verity it), and the origin file.

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Oct 31, 2019

Side note: I just confirmed that fs-verity is perfectly happy to run without any privileges (plain Unix user). This means that ostree's fsverity support would work just fine for flatpak user installs. One could also use fs-verity for random config files like ~/.bashrc.

We also should consider how this intersects with /etc of course - ext4 doesn't do reflinks, so we'll end up with un-veritied copies. With a theoretical XFS w/fsverity, we could rely on reflinks.

Hmm. Maybe now that overlayfs is pretty stable...we should go with adding a model where overlayfs is used for /etc with OSTree (ostree config set sysroot.overlayetc true) This would have a few benefits; it'd be more efficient on filesystems without reflinks, and it'd also let us rely on having the default config files be read with verity.

I'm excited about the potential of fs-verity!

@cgwalters cgwalters force-pushed the cgwalters:ostree-verity branch 3 times, most recently from 3a9132d to 2f516e7 Nov 4, 2019
@cgwalters cgwalters force-pushed the cgwalters:ostree-verity branch from 2f516e7 to c2301cb Dec 6, 2019
@cgwalters cgwalters changed the title WIP: fsverity Initial fs-verity support Dec 6, 2019
@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Dec 6, 2019

Lifted the WIP from this - it definitely works and does something useful today: makes files immutable. I particularly want to do this for FCOS for /boot to start since we don't have the readonly bind mount over /boot.

The question is; is that worth merging as is? I'm uncertain. I think it's quite likely we'll need a "sign this file" callback. And...looking at that, we have the big decision whether to import the existing C code or to execute it as a subprocess (or, try to get a library made out of it upstream) (Or, git submodule it).

And, none of the signing will really be worth much until we implement a full plan per above.

And, actually need support for shipping the signatures out of band too. That's going to be hard to do elegantly while retaining backwards compatibility. Bigger picture, if one can assume verity, then we don't need the "basic OSTree sha256" anymore...which argues for thinking of this more like a fully separate "mode" for OSTree with its own repository format...

Using fs-verity is natural for OSTree because it's file-based,
as opposed to block based (like dm-verity).  This only covers
files - not symlinks or directories.  And we clearly need to
have integrity for the deployment directories at least.

So making this truly secure would need a lot more work.  Nevertheless,
I think it's time to start experimenting with it.  Among other things,
it does *finally* add an API that makes files immutable, which will
help against some accidental damage.

This is basic enablement work that is being driven by
Fedora CoreOS; see also coreos/coreos-assembler#876
@cgwalters cgwalters force-pushed the cgwalters:ostree-verity branch from c2301cb to 4aec484 Dec 6, 2019
@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Dec 6, 2019

I updated this to disable even the "opportunistic" use of fs-verity (previously it'd only happen if you happened to mkfs.ext4 -O verity which isn't the default so extremely unlikely to do accidentally), but at least now it's totally off unless one sets the repo config flag to require it.

(That said also, we need to care for the case that /boot is of a different filesystem type)

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Dec 11, 2019

OK, so confirm here, without the "measure" step, this is essentially a stronger version of chattr +i, right? And this patch also acts as a stepping stone towards when we do figure out integrity checking?

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Dec 11, 2019

OK, so confirm here, without the "measure" step, this is essentially a stronger version of chattr +i, right?

Stronger in some ways, but also weaker in others; e.g. it does allow unlink() and link() which we really want. It also does allow chmod u+s and chown foo:bar which we don't...

And this patch also acts as a stepping stone towards when we do figure out integrity checking?

I think a bottom line is this is useful as a starting point.

@cgwalters

This comment has been minimized.

Copy link
Member Author

cgwalters commented Dec 11, 2019

/override f29-rpmostree

@openshift-ci-robot

This comment has been minimized.

Copy link
Collaborator

openshift-ci-robot commented Dec 11, 2019

@cgwalters: Overrode contexts on behalf of cgwalters: f29-rpmostree

In response to this:

/override f29-rpmostree

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.