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

handling suid/world-writable content #845

Closed
cgwalters opened this Issue Jun 12, 2017 · 15 comments

Comments

Projects
None yet
4 participants
@cgwalters
Collaborator

cgwalters commented Jun 12, 2017

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

Breaking #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 ostreedev/ostree#914 but for the bare case?

@cgwalters

This comment has been minimized.

Collaborator

cgwalters commented Jun 12, 2017

Some prep work in ostreedev/ostree#922

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Jun 13, 2017

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

This comment has been minimized.

Member

alexlarsson commented Jun 13, 2017

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

This comment has been minimized.

Member

alexlarsson commented Jun 13, 2017

The code i'm refering to is in #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

This comment has been minimized.

Collaborator

cgwalters commented Jun 13, 2017

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

This comment has been minimized.

Collaborator

cgwalters commented Jun 13, 2017

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 added a commit to cgwalters/ostree that referenced this issue Jun 13, 2017

lib/pull: Add OSTREE_REPO_PULL_FLAGS_BAREUSERONLY_FILES
This is an option which is intended mostly for flatpak;
see: flatpak/flatpak#845

We're adding an option for pulling into *all*
repo modes that has an effect similar to the `bare-user-only`
change from ostreedev#909

This way one can pull content into e.g. a root-owned `bare` repository and
ensure that there aren't any setuid or world-writable files.
@cgwalters

This comment has been minimized.

Collaborator

cgwalters commented Jun 13, 2017

cgwalters added a commit to cgwalters/ostree that referenced this issue Jun 13, 2017

lib/pull: Add OSTREE_REPO_PULL_FLAGS_BAREUSERONLY_FILES
This is an option which is intended mostly for flatpak;
see: flatpak/flatpak#845

We're adding an option for pulling into *all*
repo modes that has an effect similar to the `bare-user-only`
change from ostreedev#909

This way one can pull content into e.g. a root-owned `bare` repository and
ensure that there aren't any setuid or world-writable files.

cgwalters added a commit to cgwalters/ostree that referenced this issue Jun 13, 2017

lib/checkout: Add bareuseronly_dirs option
This is a continuation of ostreedev#926
for directories instead of files.

See: flatpak/flatpak#845

This option suppresses mode bits outside of `0775` for directory
checkouts.  I think most people should start doing this by default,
and use explicit overrides for e.g. `/tmp` if doing a recommit based
on a checkout.
@cgwalters

This comment has been minimized.

Collaborator

cgwalters commented Jun 13, 2017

rh-atomic-bot added a commit to ostreedev/ostree that referenced this issue Jun 13, 2017

lib/pull: Add OSTREE_REPO_PULL_FLAGS_BAREUSERONLY_FILES
This is an option which is intended mostly for flatpak;
see: flatpak/flatpak#845

We're adding an option for pulling into *all*
repo modes that has an effect similar to the `bare-user-only`
change from #909

This way one can pull content into e.g. a root-owned `bare` repository and
ensure that there aren't any setuid or world-writable files.

Closes: #926
Approved by: alexlarsson
@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Jun 13, 2017

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 added a commit to cgwalters/ostree that referenced this issue Jun 13, 2017

lib/checkout: Add bareuseronly_dirs option
This is a continuation of ostreedev#926
for directories instead of files.

See: flatpak/flatpak#845

This option suppresses mode bits outside of `0775` for directory
checkouts.  I think most people should start doing this by default,
and use explicit overrides for e.g. `/tmp` if doing a recommit based
on a checkout.

rh-atomic-bot added a commit to ostreedev/ostree that referenced this issue Jun 13, 2017

lib/checkout: Add bareuseronly_dirs option
This is a continuation of #926
for directories instead of files.

See: flatpak/flatpak#845

This option suppresses mode bits outside of `0775` for directory
checkouts.  I think most people should start doing this by default,
and use explicit overrides for e.g. `/tmp` if doing a recommit based
on a checkout.

Closes: #927
Approved by: alexlarsson

cgwalters added a commit to cgwalters/flatpak that referenced this issue Jun 14, 2017

system-helper: Use new ostree APIs to reject world-writable/suid content
This uses the new libostree APIs that landed recently to ensure
that we reject any files with mode outside of `0775` for system
helper pulls, and we also neuter directories during checkout.

However, this does *not* fix up any already downloaded content.
For that, one could uninstall/reinstall; or a future patch could
do a one-time fixup pass.

Closes: flatpak#845
@cgwalters

This comment has been minimized.

Collaborator

cgwalters commented Jun 14, 2017

PR in #848

cgwalters added a commit to cgwalters/flatpak that referenced this issue Jun 14, 2017

Use new libostree APIs to reject world-writable/suid content
This uses the new libostree APIs that landed recently to ensure
that we reject any files with mode outside of `0775` for system
helper pulls, and we also mask directory modes during checkout.

However, this does *not* fix up any already downloaded content.
For that, one could uninstall/reinstall; or a future patch could
do a one-time fixup pass.

Note that I am not aware of a way for flatpak applications to escalate their
privileges directly with this flaw; the bubblewrap `PR_SET_NO_NEW_PRIVS` turns
of setuid. However, in combination with code execution on the host via another
mechanism (e.g. unsandboxed app), a setuid app injected could be used to gain
full host privileges.

At this time we're not aware of any flatpak content exploiting this issue.

Closes: flatpak#845

cgwalters added a commit to cgwalters/flatpak that referenced this issue Jun 15, 2017

Use new libostree APIs to reject world-writable/suid content
This uses the new libostree APIs that landed recently to ensure
that we reject any files with mode outside of `0775` for system
helper pulls, and we also mask directory modes during checkout.

However, this does *not* fix up any already downloaded content.
For that, one could uninstall/reinstall; or a future patch could
do a one-time fixup pass.

Note that I am not aware of a way for flatpak applications to escalate their
privileges directly with this flaw; the bubblewrap `PR_SET_NO_NEW_PRIVS` turns
of setuid. However, in combination with code execution on the host via another
mechanism (e.g. unsandboxed app), a setuid app injected could be used to gain
full host privileges.

At this time we're not aware of any flatpak content exploiting this issue.

Closes: flatpak#845

cgwalters added a commit to cgwalters/flatpak that referenced this issue Jun 15, 2017

Use new libostree APIs to reject world-writable/suid content
This uses the new libostree APIs that landed recently to ensure
that we reject any files with mode outside of `0775` for system
helper pulls, and we also mask directory modes during checkout.

However, this does *not* fix up any already downloaded content.
For that, one could uninstall/reinstall; or a future patch could
do a one-time fixup pass.

Note that I am not aware of a way for flatpak applications to escalate their
privileges directly with this flaw; the bubblewrap `PR_SET_NO_NEW_PRIVS` turns
of setuid. However, in combination with code execution on the host via another
mechanism (e.g. unsandboxed app), a setuid app injected could be used to gain
full host privileges.

At this time we're not aware of any flatpak content exploiting this issue.

Closes: flatpak#845

alexlarsson added a commit that referenced this issue Jun 15, 2017

Use new libostree APIs to reject world-writable/suid content
This uses the new libostree APIs that landed recently to ensure
that we reject any files with mode outside of `0775` for system
helper pulls, and we also mask directory modes during checkout.

However, this does *not* fix up any already downloaded content.
For that, one could uninstall/reinstall; or a future patch could
do a one-time fixup pass.

Note that I am not aware of a way for flatpak applications to escalate their
privileges directly with this flaw; the bubblewrap `PR_SET_NO_NEW_PRIVS` turns
of setuid. However, in combination with code execution on the host via another
mechanism (e.g. unsandboxed app), a setuid app injected could be used to gain
full host privileges.

At this time we're not aware of any flatpak content exploiting this issue.

Closes: #845
@carnil

This comment has been minimized.

carnil commented Jun 21, 2017

This issue has been assigned CVE-2017-9780.

@smcv

This comment has been minimized.

Contributor

smcv commented Jun 21, 2017

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 #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

This comment has been minimized.

Collaborator

cgwalters commented Jun 22, 2017

@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

This comment has been minimized.

Contributor

smcv commented Jun 22, 2017

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

This comment has been minimized.

Contributor

smcv commented Jun 22, 2017

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

smcv added a commit to smcv/flatpak that referenced this issue Jun 28, 2017

test-run: Allow org.test.Setuid to install, as long as it's not setuid
libostree attempts to strip the setuid and setgid bits from file
permissions in user-mode checkouts, which, if successful, would make
Flatpak's check for setuid ineffective and unnecessary. In versions
older than 2017.7 this was not consistently applied, making commits
2c8e241 and 02a299f necessary to defeat CVE-2017-9780 (see flatpak#845).

libostree 2017.7 removes setuid and setgid bits more thoroughly
as a result of fixing ostreedev/ostree#633
in PR ostreedev/ostree#903, which means that
this test fails when linking flatpak 0.8.x to libostree 2017.7.

Signed-off-by: Simon McVittie <smcv@debian.org>

alexlarsson added a commit that referenced this issue Jun 28, 2017

test-run: Allow org.test.Setuid to install, as long as it's not setuid
libostree attempts to strip the setuid and setgid bits from file
permissions in user-mode checkouts, which, if successful, would make
Flatpak's check for setuid ineffective and unnecessary. In versions
older than 2017.7 this was not consistently applied, making commits
2c8e241 and 02a299f necessary to defeat CVE-2017-9780 (see #845).

libostree 2017.7 removes setuid and setgid bits more thoroughly
as a result of fixing ostreedev/ostree#633
in PR ostreedev/ostree#903, which means that
this test fails when linking flatpak 0.8.x to libostree 2017.7.

Signed-off-by: Simon McVittie <smcv@debian.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment