See http://www.openwall.com/lists/oss-security/2018/04/24/1
BUG: 393493
dfaure | |
maximilianocuria |
See http://www.openwall.com/lists/oss-security/2018/04/24/1
BUG: 393493
No Linters Available |
No Unit Test Coverage |
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?
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.
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.
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!
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:
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.
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?
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.
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.
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
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?
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 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.