CVE-2018-10361: privilege escalation
ClosedPublic

Authored by cullmann on Apr 25 2018, 10:06 AM.

Diff Detail

Repository
R39 KTextEditor
Branch
cve-2018-10361 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptApr 25 2018, 10:06 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
maximilianocuria requested review of this revision.Apr 25 2018, 10:06 AM
maximilianocuria edited the summary of this revision. (Show Details)Apr 25 2018, 10:08 AM
cullmann commandeered this revision.Apr 25 2018, 10:09 AM
cullmann added a reviewer: maximilianocuria.
cullmann added a subscriber: cullmann.

Hi, I propose this variant, that should remove all issues report.

cullmann updated this revision to Diff 33057.Apr 25 2018, 10:11 AM

Should solve all reported issues.

The only thing that is unclear:

Shall we use the target dir for the tempfile with

QTemporaryFile tempFile(targetFileInfo.absolutePath() + QStringLiteral("/secureXXXXXX"));

or shall we keep in in the normal tempdir? That would make atomic rename "unlikely" to work.

Why dropping syncToDisk? Why is that related to the current issue?

Why dropping syncToDisk? Why is that related to the current issue?

Because it always just worked on a invalid file handle.
QFile(targetFile).handle()
That is just wrong.
One can add it to an other place fixed later if one really needs it.

fvogt added a subscriber: fvogt.Apr 25 2018, 11:00 AM

There's a typo in the title, it should be "privilege escalation".

maximilianocuria retitled this revision from CVE-2018-10361: privelege escalation to CVE-2018-10361: privilege escalation.Apr 25 2018, 11:42 AM

There's a typo in the title, it should be "privilege escalation".

Done

This revision is now accepted and ready to land.Apr 25 2018, 11:57 AM

Mmh, the accept revision doesn't work as a +1, does it?
I was intending to say +1/thumbs up, but I would still prefer somebody else to review this. After all, I sent forwarded the original patch, clearly I want this to land, but it's up the frameworks/ktexteditor developers/maintainers to decide.

maximilianocuria resigned from this revision.Apr 25 2018, 12:07 PM
This revision now requires review to proceed.Apr 25 2018, 12:07 PM

I will ask the openSUSE engineer Matthias Gerstner for feedback before landing this.

aacid added a subscriber: aacid.Apr 25 2018, 8:23 PM

Any reason you guys decided to not involve security@kde.org ?

Any reason you guys decided to not involve security@kde.org ?

I think we all forgot to do that, without any real reason, I drop that address a mail now, thanks for the hint!

mgerstner added a subscriber: mgerstner.EditedApr 27 2018, 2:20 PM

Hi,

I am the guy that came up with the initial security report. I contacted
cullman about the issue and we've exchanged a couple of emails about how
to improve the code.

He asked me about what approach would be better: Setting up the temporary file
in $TMPDIR and potentially lose the atomic rename() possibility or keeping the
approach of creating the temporary file in the target directory.

We agreed upon that I add my thoughts here in this Phabricator entry for
public discussion.

The issue I reported was caused by reopening the temporary file which was
probably caused by a misunderstanding of the QTemporaryFile API. The new code
discussed so far should fix this issue and thus the exploit I published.

Apart from this I don't think it matters much if the temporary file is kept in
$TMPDIR or in the target directory. If the target directory is owned by a
non-root user then there is always room for shenanigans by the unprivileged
user. Therefore I would stick to the approach of keeping the temporary file in
the target directory and additionally to the following:

  • enter the target directory via chdir()
  • check if the owner and group of the directory:
    • if owned by root:root, good to go
    • otherwise either reject the operation (simple) or do a temporary privdrop to the owner/group of the directory including drop of supplementary groups (complex).
  • create the tmpfile in the target dir and do the renameat() using only AT_FDCWD
  • restore privileges, if necessary

The tricky thing is doing the privdrop, which is probably not covered by the
Qt core library. The good thing about it is that with doing this the kernel
takes over worrying about permission handling, which it is good at.

aacid added a comment.May 1 2018, 11:05 PM

Next time please use arc to upload patches, so that instead of those ugly "Context not available." we get nice links to see more code :)

@mgerstner I don't really understand why we need the chdir, renameat, etc.

Dropping privileges to the minimum needed should be enough, shouldn't it?

I mean at that point the only thing that can happen is that some user breaks files he can write to anyway, so why should we take extra precautions from that point on?

mgerstner added a comment.EditedMay 3 2018, 9:21 AM

@mgerstner I don't really understand why we need the chdir, renameat, etc.

Dropping privileges to the minimum needed should be enough, shouldn't it?

I mean at that point the only thing that can happen is that some user breaks files he can write to anyway, so why should we take extra precautions from that point on?

Strictly speaking the renameat() in the target CWD is not necessary when the privdrop is in place. But the approach is mostly implemented already at the moment and it works reasonably well (given the temporary file is not unnecessarily reopened like it was the case). You have to decide how and where to create the temporary file and this is one valid approach that has the charme of supporting the atomic renameat(). Once the target directory has been entered all directory traversal questions are out of the way.

If you choose a different approach then you will have to open the target file explicitly, which raises other questions like how to safely replace symlinks. Of course such an approach can also be implemented safely. In any case a prudent handling of the temporary file handling improves trust in and robustness of the code and provides additional barriers should errors slip in in the future by way of refactoring or extending the code.

While this discussion here focuses on the CVE at hand, we have a broader discussion about the savefile() feature in an openSUSE review bug. I think the overall implementation can use some extra security checks and usability improvements.

aacid added a comment.May 5 2018, 7:34 PM

If you choose a different approach then you will have to open the target file explicitly, which raises other questions like how to safely replace symlinks. Of course such an approach can also be implemented safely. In any case a prudent handling of the temporary file handling improves trust in and robustness of the code and provides additional barriers should errors slip in in the future by way of refactoring or extending the code.

Honestly i don't understand why i have to care about anything.

If we drop privileges, it's just some code running with regular user level privileges, why are symlinks a problem?

Because some malicious code can create symlinks that make the code write to file X when we wanted to write to file Y?

Sure that's bad, but if you have in your system something that can create such symlink, it already has user level privileges, so it can already write to file X or file Y itself, without "exploiting" kate to do it.

Or am I missing something?

Honestly i don't understand why i have to care about anything.

If we drop privileges, it's just some code running with regular user level
privileges, why are symlinks a problem?

Well for one, if the target directory is owned by root, then you will be
dropping privileges to root i.e. you won't drop privileges at all.

The owners of the directory and the target file may differ. Another case might
be that target directory and file are owned by root, but one of the upper
directories is owned by a non-root user. Maybe it is a root-owned directory
that is only temporary in nature and a race condition is involved i.e. it gets
deleted before the actual file operations begin.

It would be uncommon but we never now what the situation might be. The feature
seems targeted towards users that have no big technical insight. So strange
situations can be expected. IMO prudence is the better part of valor here.

Restricted Application edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: Frameworks. · View Herald TranscriptMay 9 2018, 11:51 AM
aacid added a comment.May 9 2018, 10:01 PM

I meant dropping privileges to the user that is running the ktexteditor program, not to the user that owns the target directory, but now that i think about it that's pretty stupid since otherwise we wouldn't be needing root :D

I'll try to go over this with a fresh mind again

> What should we do with this?

aacid added a comment.May 28 2018, 9:43 PM

I think it was agreed this is an improvement, so i'm going to suggest we commit it.

I'm definitely very short on time to spend here because someone added poppler to oss-fuzz and i've a pile of files that are crashing / causing bad behaviour on poppler to care for.

Once this is in, we should open a bug/phabricator task/wathever with what is missing and the recommendations to fix it.

Also not sure if useful but since kio is getting support for writting to "root owned" files we should investigate if maybe we can just simply drop this code altogether?

Also not sure if useful but since kio is getting support for writting to "root owned" files we should investigate if maybe we can just simply drop this code altogether?

As KIO has its own similar issues and it's not ready yet, can you merge this one? More than a month passed since vulnerabilities were reported, fix posted here and we're still in limbo.

I can push that change, if OK.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.

I followed the "I think it was agreed this is an improvement, so i'm going to suggest we commit it." comment from above.
In any case, it is an improvement to the old situation.