handling suid/world-writable content

cgwalters commented 7 years ago

While working on https://github.com/ostreedev/ostree/pull/908, I realized that flatpak's system helper could write root-owned suid binaries.

Breaking https://github.com/flatpak/flatpak/pull/837 out into an issue, since I think we need to do more design.

A basic problem here is we have 2 separate cases to handle:

Flatpak default of /var/lib/flatpak/repo (currently bare-user)
Endless OS case of /ostree/repo (i.e. bare)

`/var` case

In the original PR I was thinking of the /var/flatpak case. For that, we have two sub-options:

Land fixups for bare-user to suppress all this
Convert to bare-user-only

Either way, I think we're going to need some sort of "repository format change" mechanism. Doing a local pull between bare-user and bare-user-only unfortunately will require duplicating all of the content right now...but, possibly we could teach ostree that it's fine to hardlink file content between them, and just delete all the user.ostreemeta xattrs after?

System case

Something like OSTREE_REPO_PULL_FLAGS_BAREUSERONLY_PERMS to ostree_repo_pull()? We'd error out on finding world-writable/setuid files. Also, we add a bareuser_perms flag to ostree_repo_checkout() which does the same thing as https://github.com/ostreedev/ostree/pull/914 but for the bare case?

cgwalters commented 7 years ago

Some prep work in https://github.com/ostreedev/ostree/pull/922

alexlarsson commented 7 years ago

For the system case, could we not ressurect your scanning code. With the fixes with reading from the staging dir we could probably implement it fine.

Otherwise, ignoring the endless case for the moment, i think we should:

Depend on latest ostree (do you post-release bump the version?)
Create new repos, for both user and system as bare-user-only
In the system helper, use the new code to convert the system repo from bare-user to bare-user-only.
For old per-user dirs, keeping them as-is should be fine due to the permissions change we did for the repo dir.

alexlarsson commented 7 years ago

Note: This all assumes we're don't have any pre-existing world writable dirs or setuid binaries in the system repo, as we're not updateing the deploy dir, and hardlinking existing files when we convert.

I think that is a fine assumption at this point though.

Another thing to consider is that once you convert the system repo we're not backwards compat with old flatpak version anymore.

alexlarsson commented 7 years ago

The code i'm refering to is in https://github.com/flatpak/flatpak/pull/837, but we would only need to do it if mode=bare and only in repo_pull_one_untrusted() which is whats used to import into the system repo.

cgwalters commented 7 years ago

Let's try not to make a format change a required part of this fix. If we wait e.g. a few months for the supporting code in flatpak/ostree to propagate around, it's a lot less likely that someone will do a system downgrade to the older flatpak due to some other regression.

cgwalters commented 7 years ago

How about:

Add ostree pull option to error out on non-0775 files, teach flatpak to use it
Add ostree checkout option to suppress non-0775 directories, teach flatpak to use it
Release new ostree, make flatpak depend on it

Those two changes would apply to both the existing bare-user and the bare cases. It's basically making the bare-user-only hardening opt-in for the existing modes.

cgwalters commented 7 years ago

Step 1 - https://github.com/ostreedev/ostree/pull/926

cgwalters commented 7 years ago

Step 2 - https://github.com/ostreedev/ostree/pull/927

alexlarsson commented 7 years ago

That sounds like a great plan. I reviewed the other patches, and they looked ok to me. I will look at the flatpak side tomorrow, and if that looks good we should do an ostree release.

Can you do a preliminary ostree version bump so i can check that.

cgwalters commented 7 years ago

PR in https://github.com/flatpak/flatpak/pull/848

carnil commented 7 years ago

This issue has been assigned CVE-2017-9780.

smcv commented 7 years ago

The Debian security team have asked me to post an advisory to the oss-security list.

Here's a draft, please edit or comment as desired:

Subject: CVE-2017-9780: Flatpak: privilege escalation via setuid/world-writable file permissions

Impact: privilege escalation
Attack range: local
Vulnerable: all versions < 0.8.7, 0.9.x < 0.9.6
Fixed in: 0.8.x >= 0.8.7, all versions >= 0.9.6

Flatpak is a desktop application distribution framework for Linux.

Colin Walters discovered a security vulnerability in versions of Flatpak prior to 0.8.7. A third-party app repository could include malicious apps that contain files with inappropriate permissions, for example setuid or world-writable. Older Flatpak versions would deploy the files with those permissions, which would let a local attacker run the setuid executable (escalating their privileges) or write to the world-writable location.

In the case of the system helper used when an app is installed system-wide, files deployed as part of the app are owned by root, so in the worst case the app repository could arrange for a setuid root executable to be present.

Mitigations:

If you are using Flatpak to install apps from a third-party vendor, then there is already a trust relationship: the app is sandboxed, but the third-party vendor chooses what parameters are used for the sandbox.
The default polkit policies will not allow apps to be installed system-wide unless a privileged (root-equivalent) user has added the third-party app repository, which indicates that the privileged user trusts the operator of that repository.
The attacker exploiting inappropriate permissions to escalate privileges must be local.

This vulnerability is tracked as https://github.com/flatpak/flatpak/issues/845, CVE-2017-9780.

In the 0.8.x stable branch this vulnerability was fixed in version 0.8.7.

In the 0.9.x development branch this vulnerability was fixed in version 0.9.6.

cgwalters commented 7 years ago

@smcv Looks sane to me; the only thing I would emphasize is a bit more is that flatpak itself uses PR_SET_NO_NEW_PRIVS which means flatpak apps can't themselves use the setuid binaries they inject. I think that's part of why this issue was overlooked.

smcv commented 7 years ago

Vulnerable: all versions <= 0.8.7, 0.9.x <= 0.9.6

That should of course have said < 0.8.7, 0.9.x < 0.9.6 (strictly less than, instead of less-than-or-equal).

smcv commented 7 years ago

Sent to oss-security with the version ranges corrected, and a note about PR_SET_NO_NEW_PRIVS added to the list of mitigations.

flatpak / flatpak