KAuth integration in document saving
ClosedPublic

Authored by martinkostolny on Feb 28 2017, 10:45 PM.

Details

Summary

Before this patch: if one opens a write protected document, makes changes and then wants to save, error message occurs about insufficient privileges or disk space.

With this patch kate-part will try to save the document contents with elevated privileges in case the regular save failed. So that KAuth graphical prompt is presented to user. I believe this was suggested by many KDE users as a wanted feature. Please let me know if this isn't the right approach.

I'm in fact quite new to KTextEditor as well as KAuth so feel free to criticize the code. I'll try to fix everything you point at :).

What I basically did:

  • created TextBufferSecure class (for dedicated KAuth helper binary)
  • moved most contents of TextBuffer:save() method to this new class' saveInternal() method
  • TextBuffer:save() method now first tries to save with TextBufferSecure::saveInternal() helper method
  • if that fails it will call it again through KAuth action
Test Plan
  • editing & saving document in home page
  • editing & saving /etc/hosts
  • tried with Krusader's "internal viewer" which uses ReadWritePart + tried with Kate

But I'm sure there are a lot of other use-cases I didn't think of. Maybe you think of somehting. On my mind is now saving documents with different encoding, eol type, saving huge documents. I'll do that and if problems occur, I'll fix the diff.

Diff Detail

Repository
R39 KTextEditor
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
dhaumann requested changes to this revision.Mar 1 2017, 9:51 PM
dhaumann added a subscriber: dhaumann.

This is a good patch.

My main concerns are:

  • please use const for variables that do not change anymore
  • please don't use const & for primitive data types (like bool) in function arguments, that does not make sense
  • naming conventions: Instead of TextBufferSecure, the word SecureTextBuffer sounds much more natural. Similarly, the header file can be renamed.

Could you provide an updated revision?

Some general questions

  • Does that affect other platforms such as Windows in any way? I.e., does KAuth exist on Windows?
  • The text buffer itself is unit tested pretty well. Is it possible to have a unit test for this? PS: We already have a " bool KTextEditor::EditorPrivate::unitTestMode()" function that we could use to virtually trigger the KAuth case somehow, not sure.
src/buffer/katetextbuffer.cpp
768–769

const QString textCodec = ...
const int eolMode = ...

Please use const whenever possible for locally defined variables.

src/buffer/katetextbuffer_secure.cpp
1 ↗(On Diff #11991)

I would prefer katesecuretextbuffer.h as filename. Intermixing underscrores is rather uncommon in KDE's sources.

26–31 ↗(On Diff #11991)

Please prepend 'const' for all local variables whenever possible.

34 ↗(On Diff #11991)

const bool ok = ...

87 ↗(On Diff #11991)

const int lineCount = ...

src/buffer/katetextbuffer_secure.h
10 ↗(On Diff #11991)

Please no using namespace KAuth here, especially since this is a header file.

12 ↗(On Diff #11991)

Please call it SecureTextBuffer, that feels much more natural than adding the suffix "Secure".

18 ↗(On Diff #11991)

Pedantic: no semicolon after a closing function brace, i.e.:

SecureTextBuffer() {}
22 ↗(On Diff #11991)

Please never use const references for primitive types. Here:

  • const bool & --> bool
This revision now requires changes to proceed.Mar 1 2017, 9:51 PM
  • Does that affect other platforms such as Windows in any way? I.e., does KAuth exist on Windows?

KAuth had backends for OSX and Windows. Whether they are working or just stubs however, I don't know.

martinkostolny edited edge metadata.

Thanks for all the feedback!

I'm updating the diff to reflect most Dominik's points. I'll learn from them next time :). Anyway I was unable to remove the KAuth namespace. It doesn't work without it, this is also stated in KAuth documentation - e.g. here https://api.kde.org/frameworks/kauth/html/namespaceKAuth.html

Please note that due to QMetaObject being picky about namespaces, you NEED to declare the return type as ActionReply and not KAuth::ActionReply.

But this diff-update is not final, yet. I'm now working on the unit test and I need to address the issue mentioned by Martin - preserving file permissions when owner and group of edited file is not root.

martinkostolny marked 2 inline comments as done.Mar 2 2017, 12:06 PM
martinkostolny updated this revision to Diff 12058.EditedMar 2 2017, 2:07 PM

Next iteration, still without autotests, I need to study them more.

This diff includes preserving owner and group when file is saved with elevated privileges. Actually loosing owner was (and is) already happening before my patch when user has the permission to write the file and is not its owner. In this case, I believe we have 3 options:

  1. Leave it as it is
  2. Detect different owner and in such situation use direct writing (without QSaveFile)
  3. Use QSaveFile but in case of different owner ask for elevated privilege to be able to reset the owner

Is there another solution I'm missing?

PS: Regarding windows and other non-linux platforms I unfortunately cannot test the code there right now.

This diff includes preserving owner and group when file is saved with elevated privileges. Actually loosing owner was (and is) already happening before my patch when user has the permission to write the file and is not its owner. In this case, I believe we have 3 options:

  1. Leave it as it is

I've seen a downstream bug which could be about the same issue (https://bugzilla.redhat.com/show_bug.cgi?id=1143965) so I think that leaving it as it is is not a solution, if you can find a way to prevent this from happening.

I think that leaving it as it is is not a solution

In that case, I favour the second option (direct write to file).

After reading though bug mentioned by Luigi and this QT bug (https://bugreports.qt.io/browse/QTBUG-56366) I'm not so convinced that we want to directly write to file in order to maintain owner, because we would loose atomicity of the save operation. I've updated the diff so at least group is restored to the previous one if owner cannot be changed. I think loosing group was the most painful outcome of QSaveFile design.

Now to the autotests, I didn't make much progress, yet :). Maybe these will be a bit rookie questions but: Do you know of any KDE project I can learn from? A project with autotests that include testing KAuth action? I mean can I even test that root-owned file will only be saved with elevated privileges through KAuth action execution?

I've learnt a few things about autotests (KTextEditor::EditorPrivate::unitTestMode() was really helpful, thanks!). I managed to create a test case, which allowed the code to go through KAuth action. But I was unsuccessful to finish it to my satisfaction - I couldn't come up with a solution where in case of unit testing KAuth dialog is not shown and just allows the execution. action.exectute(ExecutionMode::AuthorizeOnlyMode) will not help here since it does not really execute the action. I also tried to create "autotestsave" action alongside existing "save" action and set it to be always allowed. Then triggered it only from unit test. This worked well but I don't find it safe having such action available in non-testing runtime.

If somebody with better KAuth knowledge would know how to allow running otherwise secured KAuth action in unit-test runtime, that would be really helpful :).

Hi,

I think this is a great thing to have!
My biggest complaint ATM is: I would not move the saving code out to the helper binary, instead, it should stay in the place it is and we could just save to a tmpfile and tell the helper to move that file over the destination.
With that, the with higher privileges running application is simpler (e.g. no need for filterdev which might execute complex compression libs) and we would avoid to store the complete file once more in memory.

Good point, thanks! I now the helper is really light-weight. Helper is now only moving a temporary file and setting permissions/owner.

I've also added a unit test. It directly calls the action method instead of calling KAuth action. It's not nice but I think it's safe and it works.

anthonyfieroni added inline comments.
src/buffer/katetextbuffer.cpp
791

use QScopedPointer, remove manual delete

886

Should be -> instead of .

Thanks for your guidance and for having the patience with me. QScopedPointer was indeed very useful.

One thing I've noticed - it seems there isn't any naming convention for KAuth helper binary. Sometimes it ends with "_helper", sometimes it has different separators like dashes or non separators at all. Shouldn't this binary at least start with proper namespace like "org.kde.ktexteditor" to prevent future collisions?

martinkostolny marked 2 inline comments as done.Mar 5 2017, 10:01 AM

Thanks for in cooperating my advice!
I think for the naming, we could just call it kauth_ktexteditor_helper.
That makes clear what it is and with ktexteditor in the name, we will not have clashs, or?
We should avoid new "kate.." names in the installation as actually the framework is really non-kate. The internal naming is just historical.

Understood and agreed, kauth_ktexteditor_helper it is :).

Thanks, as my KAuth knowledge is very limited (aka 0), any other input on this?

dfaure added a subscriber: dfaure.Mar 5 2017, 6:55 PM
dfaure added inline comments.
src/buffer/katesecuretextbuffer.cpp
1

Missing copyright header

39

Isn't it possible to call setPermissions on the QTemporaryFile instance instead?

61

Not sure it matters, but exists+remove+rename is racy. The file could be deleted by another process after exists() and before remove() (which would then fail) or the file could be created by another process after the call to remove() (and then rename() would fail).

QSaveFile doesn't have these issues because it uses the POSIX rename() function which is atomic (and replaces any existing files). Unfortunately QFile doesn't expose such a method (one is supposed to use QSaveFile instead...).

src/buffer/katesecuretextbuffer.h
1

Missing copyright header

Assuming the file is not installed, I would name it _p.h (but I don't know if the rest of this framework follows this convention - I think it's in the KF5 guidelines though).

11

a bit of docu maybe?

src/buffer/katetextbuffer.cpp
75

Isn't it weird to ignore the constructor argument in some cases?

912

Nested event loops are evil. Any chance to make this asynchronous (connecting to a slot instead, for anything that needs to be done after completion of the job) ?

Thanks a lot for all the thoughts and suggestions! I tried to work them in, but I need help with some of them.

I'm struggling with the atomic rename. I still find Christoph's suggestion (save to temp file and then move it in helper) a good option to stick with. But then we are in many cases unable to atomically move the tempfile because tempfile is usually in tmpfs (RAM) -> different device then target file to save. So we can't even use std::rename for this. Exist & remove & rename are racy so I went for copying to the QSaveFile and then commit. It's not exactly a one-liner, is there a better way? I may very well be on a wrong path here.

About the event loop evil. I agree it should be re-written with QT connects and I don't mind digging in the code. But if I understand the existing code correctly, I'd also have to rewrite at least KateBuffer::saveFile(), DocumentPrivate::saveFile() and DocumentPrivate::documentSaveCopyAs(). I'd like to suggest doing all this in a separate diff/review.

Did I miss something else?

martinkostolny marked 4 inline comments as done.Mar 6 2017, 10:59 PM
dfaure added a comment.Mar 7 2017, 8:15 AM

Right the only way to get atomic renaming is to write the tempfile next to its final destination, NOT in /tmp.
(just like QSaveFile does ;)

src/buffer/katesecuretextbuffer.cpp
75

It's Qt, not QT.

QT is QuickTime ;)

89

You definitely don't need QDataStream just to call read and write on QIODevices.

102

Not necessary if you use QSaveFile, which does this already.

But maybe the best solution here (to avoid the file copy) is a temp file that you can atomically rename, so let's discuss that before you get rid of all the stuff-that-QSaveFile-does ;)

src/buffer/katetextbuffer.cpp
807

The thing about using Qt's setPermissions() is that it should work on Windows too.

815

Doesn't QSaveFile do this?

880

(pre-existing) done by QSaveFile already

899

use .insert() to avoid the creation of a temporary.

apol added a subscriber: apol.Mar 7 2017, 11:15 AM
apol added inline comments.
src/buffer/katetextbuffer.cpp
899

Or better use an initializer list?

Thanks a sorry for all the rookie mistakes. I tried to fix the problems You mentioned:

  • QT->Qt
  • get rid of useless streams
  • no more setting permissions and sync-to-disk when using QSaveFile
  • using of QMap::insert (should I use an initializer list instead?)

Now, while discussing the atomic rename, I'm throwing a thought: how about using QStandardPaths::CacheLocation (~/.cache) for the temporary file? It should always exist, it is more likely the same FS device and should be writable by the user. I did that in this diff and left a fallback for using QSaveFile if atomic rename fails.

martinkostolny marked 7 inline comments as done.Mar 8 2017, 1:31 AM
dfaure added a comment.Mar 8 2017, 8:03 AM

I don't know the full architecture of this kauth stuff, but isn't it possible to save to <destinationfile>.tmp rather than /tmp, ~/.cache or anywhere else? Then instead of "more likely" we are *sure* it's on the same device as the destination.

Good point! I was about to say that the target folder is in most cases non-writable without elevated privileges. But we can actually use KAuth action twice, for 2 simple jobs:

  1. Create temporary file right next to target file, set current user as owner, so it is writable without root privileges.
  2. And after storing file contents outside kauth helper binary. atomically rename the temporary file.

Any objections against using 2 actions? KAuth action session ensures that user is asked for password only first time using a KAuth action.

I've used QTemporaryFile(filename) (instead of "xxx.tmp") so Qt took care of uniqueness of filename.

Another note: I didn't call windows specific atomic rename method because I cannot test it on my machine. So on windows QSaveFile fallback is used for now.

I know there is a lot of other stuff going on. So just to be sure: am I supposed to do something else - are we waiting for me?

Also if I'm too rookie for this, feel free to commandeer the revision. :)

I am no security expert, therefore I might not see obvious security flaws.
Beside that we still have issues with the atomicity of the rename, it should work as it is now, or I am mistaken?

it should work as it is now, or I am mistaken?

I believe the latest diff update is indeed making use of atomic rename. I will roughly summarize what the code currently does:

  1. First try to open QSaveFile, if succeeded -> finish writing as before the patch
  2. If opening QSaveFile fails KAuth action is called for creation of a temporary file in the same directory as the original target file
  3. Then writing to this file is performed as regular user (same as before the patch)
  4. Finally, second KAuth action is called to atomically rename the temporary file

Owner and group is taken care of. Atomic rename is used only for Unix. On Windows there is a fallback using another QSaveFile which is also atomic when renaming but there is otherwise useless file copy beforehand. From my (non-expert) point of view this fallback is the only thing that needs fixing right now. But I currently cannot do that, it seems to me that it can be done later, too.

I am no security expert.

Me neither. OK let's wait for somebody better qualified for this task :).

Looks good to me. Just some minor things I noticed.

src/buffer/katesecuretextbuffer.cpp
27

where is stdio.h used?

101

make this static so that the next line doesn't look to the compiler like a variable-size array (which isn't standard, although I'm not sure about C++11).

Maybe it has to be a #define even, I'm not sure.

src/buffer/katesecuretextbuffer_p.h
21 ↗(On Diff #12309)

...._P_H to match the header filename

83 ↗(On Diff #12309)

A static slot is.... creative ;-)
But if it works, that's fine.

Updating diff with refinements based on David's insights, thanks David!

Regarding static slot, it was quite convenient to call it statically in unit test mode and it works. So I left it there like this. I have no problems making the slot non-static if necessary.

martinkostolny marked 3 inline comments as done.Mar 28 2017, 10:21 PM
dfaure accepted this revision.Mar 29 2017, 7:05 AM

Could you wait with this commit until after the 5.33 tag? Then we have 1 month instead of 3 days to find and fix regressions. The tag will happen this Saturday. :)

Sure, no problem :).

This revision was automatically updated to reflect the committed changes.

Notice from downstream integration: this feature is considered an "abuse" of the Polkit system and at least openSUSE will explicitly disable it. See https://bugzilla.suse.com/show_bug.cgi?id=1033055 for the reasoning.

fvogt added a subscriber: fvogt.Apr 10 2017, 2:02 PM

Adding to Luca's comment, I also find two additional issues with this approach, it is absolutely impossible to make this secure.
There is always a race condition between acquiring the privilege and renaming the file to the new location.
Only solution for that is to pass the full file content to the helper (which would then give the user a checksum of the full document).

Additional race condition is for new files: They are moved into the directory first and only after that the permissions are set. This is not the right approach, it needs to be done like:

  • Create empty file with the right permissions and owner in the new path
  • Rename temp file to the new path

Therefore two more NAKs from me.

@lbeltrame

I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'.

In D4847#101088, @ivan wrote:

@lbeltrame

I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'.

That said, the inclusion of the feature is vetoed for now... We'll try to convince them otherwise, but not before the issues mentioned in this review are fixed.

fvogt added a comment.Apr 10 2017, 2:32 PM
In D4847#101088, @ivan wrote:

@lbeltrame

I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'.

Me neither. In my opinion this approach here is the most secure and also user friendly option possible, once the issues I found are fixed properly.
We'll raise the issue with the security team again and try to convince them.

In D4847#101163, @aacid wrote:

How is this related with https://phabricator.kde.org/T5202 ?

Once implemented, it will be possible to get kauth for free when calling (for example) KIO::move(). Not sure if that's enough for what this patch does, though.

I've created a follow-up diff D5394 and added every subscriber from here. I hope it wasn't too invasive of me.

Once implemented, it will be possible to get kauth for free when calling (for example) KIO::move(). Not sure if that's enough for what this patch does, though.

I'm not sure about this. Maybe it can be used but I think we now want more atomic way of saving the file contents then saving in user's space and then moving to privileged location with KIO. But I can be missing something.