D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: acooligan, kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, 
fvogt, cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:c81af5aa1d4f: CVE-2018-10361: privilege escalation 
(authored by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12513?vs=33057=35772

REVISION DETAIL
  https://phabricator.kde.org/D12513

AFFECTED FILES
  src/buffer/katesecuretextbuffer.cpp
  src/buffer/katesecuretextbuffer_p.h

To: cullmann, dfaure
Cc: acooligan, kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, 
fvogt, cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

cullmann added a comment.


  I can push that change, if OK.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: acooligan, kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, 
fvogt, cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Andrew Cooligan]

acooligan added a comment.


  In D12513#269889 , @aacid wrote:
  
  > 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: acooligan, kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, 
fvogt, cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Albert Astals Cid]

aacid added a comment.


  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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, fvogt, 
cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

cullmann added a comment.


  > What should we do with this?
  ==

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, fvogt, 
cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, fvogt, 
cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Matthias Gerstner]

mgerstner added a comment.
Restricted Application edited subscribers, added: kde-frameworks-devel, 
kwrite-devel; removed: Frameworks.


  In D12513#258565 , @aacid wrote:
  
  >
  
  
  
  
  > 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: kwrite-devel, kde-frameworks-devel, mgerstner, aacid, ngraham, fvogt, 
cullmann, michaelh, kevinapavew, bruns, demsking, sars, dhaumann, #frameworks

D12513: CVE-2018-10361: privilege escalation

[Albert Astals Cid]

aacid added a comment.


  In D12513#257628 , @mgerstner 
wrote:
  
  > 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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: mgerstner, aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Matthias Gerstner]

mgerstner added a comment.


  In D12513#256845 , @aacid wrote:
  
  > @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 bug review bug 
. I think the overall 
implementation can use some extra security checks and usability improvements.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: mgerstner, aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Albert Astals Cid]

aacid added a comment.


  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?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: mgerstner, aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Matthias Gerstner]

mgerstner added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: mgerstner, aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, 
kevinapavew, bruns, demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

cullmann added a comment.


  > Any reason you guys decided to not involve secur...@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!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, kevinapavew, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, kevinapavew, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Christoph Cullmann]

cullmann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Maximiliano Curia]

maximilianocuria resigned from this revision.
This revision now requires review to proceed.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Maximiliano Curia]

maximilianocuria added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, maximilianocuria, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Maximiliano Curia]

maximilianocuria accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, maximilianocuria, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Maximiliano Curia]

maximilianocuria added a comment.


  In D12513#253537 , @fvogt wrote:
  
  > There's a typo in the title, it should be "privilege escalation".
  
  
  Done

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, maximilianocuria, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12513: CVE-2018-10361: privilege escalation

[Maximiliano Curia]

maximilianocuria retitled this revision from "CVE-2018-10361: privelege 
escalation" to "CVE-2018-10361: privilege escalation".

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12513

To: cullmann, maximilianocuria, dfaure
Cc: fvogt, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann