KAuth integration in document saving - vol. 2
ClosedPublic

Authored by martinkostolny on Apr 11 2017, 12:37 AM.

Details

Summary

This is a follow-up of D4847, trying to fix issues pointed out by Fabian:

  • send the whole document content to KAuth action
  • therefore permissions are not changed to a new file after its creation

There are a few questions / missing features:

  • should I make a checksum of the data before sending to KAuth action and then match it with checksum sent back by the action?
  • how do we handle big files?
    • dbus default is 32MiB so I limited the sent data to 30MiB
    • but dbus can probably be configured to allow less
    • I'm not aware of a different way to send the data to the helper process, any ideas? Or is it OK, as it is?
Test Plan

in Kate:

  • edit a regular file
  • edit a file of a root user
  • edit a file of a different user (check that owner is preserved)
  • open, write and save a new file in a folder owned by root

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added subscribers: Frameworks, kwrite-devel. · View Herald TranscriptApr 11 2017, 12:37 AM
fvogt added a comment.EditedApr 11 2017, 7:04 AM

Looks good!

should I make a checksum of the data before sending to KAuth action and then match it with checksum sent back by the action?

Yes, that way you could also implement a way to show the checksum to the user. Make sure to avoid any TOCTTOU issues there. That way you can avoid sending the file content over DBus entirely, if you use a cryptographically secure checksum of the contens.

I see the code for changing the permissions is still there, what are the permissions of the temporary file that QSaveFile creates?
It mustn't be something other than 0600 (-rw----).

aacid added a comment.Apr 11 2017, 8:11 AM
In D5394#101275, @fvogt wrote:

what are the permissions of the temporary file that QSaveFile creates?

If the file exists it re-uses the existing permissions, otherwise it uses 666
https://github.com/qt/qtbase/blob/dev/src/corelib/io/qsavefile.cpp#L235

fvogt requested changes to this revision.EditedApr 11 2017, 8:30 AM
In D5394#101291, @aacid wrote:
In D5394#101275, @fvogt wrote:

what are the permissions of the temporary file that QSaveFile creates?

If the file exists it re-uses the existing permissions, otherwise it uses 666
https://github.com/qt/qtbase/blob/dev/src/corelib/io/qsavefile.cpp#L235

Edit: While 666 is wrong in this case, the default umask of 022 prevents anything bad from happening here.

The only issue I see remaining for now is the race between QSaveFile::commit and changing the permissions to the new owner/group.

This revision now requires changes to proceed.Apr 11 2017, 8:30 AM
martinkostolny added a comment.EditedApr 15 2017, 4:56 PM

Sorry for answering after a longer time. I need to be sure I understand everything correctly:

ad checksum: I think it would be good to revert to previous code and start from there (to ensure writing only once and not over d-bus):

  1. create a temp file in the same directory as target file (privileged action1)
  2. write contents
  3. make a checksum and remember
  4. change owner/group of the temp file (privileged action2)
  5. rename temp file to target one (privileged action2)
  6. check remembered checksum (privileged action2)
  7. if check failed, notify user

Just to be sure: the changing permissions race can only be avoided when I change it before committing the QSaveFile so the temp file is not changed after atomic rename, right?

One more question - is it necessary to show the checksum to the user? I don't see what it would be good for, but I'm probably missing something.

fvogt added a comment.Apr 15 2017, 8:22 PM

It is currently not possible to avoid a race condition as QSaveFile is broken and use of it is currently insecure as the file is created world-readable.
It can be avoided by placing the file in a drwx------ directory, but that's more work than just using QTemporaryFile directly.

However, as far as I can tell it is impossible to make this secure with a simple rename() of a user-created file as the file descriptor
will stay valid after renaming (!) so if you edit /etc/sudoers for example, any application can just open(, O_RDWR) the temporary
file and write to it after the rename without any issues.

I'd suggest one of two ways:

  1. Checksum contents
  2. Write contents into temporary file with rw-------
  3. Start the privileged action:

3.1. Read temporary file into memory
3.2. Verify content with checksum
3.3. Create new temporary file somewhere with rw------ and write content into it
3.4. Change owner
3.5. Change permissions
3.6. Rename

  1. Remove temporary file
  1. Start the privileged action, send file content directly

2.1. Create new temporary file somewhere with rw------ and write content into it
2.2. Change owner
2.3. Change permissions
2.4. Rename

Would it help if QSaveFile had an API to set more restrictive permissions on the temp file?

fvogt added a comment.Apr 16 2017, 8:53 AM

Would it help if QSaveFile had an API to set more restrictive permissions on the temp file?

No, the fix is simple: Revert https://code.qt.io/cgit/qt/qtbase.git/commit/?id=a60571b3700e80f44705ebc4bab9628cf852891c

martinkostolny edited edge metadata.

Updated diff based on Fabian's advisory. Thanks, Fabian!

I've implemented the first option: checksum -> tempfile1 -> read in privileged action -> checksum -> tempfile2 -> permissions -> rename. It works with big (e.g. 50MiB) files. I hope I didn't miss something.

Known issues:

  • Using std::rename only for unix systems while using racy QFile's remove && rename for windows as fallback. I cannot test on windows otherwise I'd implemented an atomic rename there as well.
  • I'm now using QBuffer to buffer all file bytes before making the first checksum. Probably the best solution (memory-wise) would be to use QTemporaryFile directly and capture all written bytes right before they are written to the file and make the checksum from it. But I didn't find an easy way to do that.
fvogt requested changes to this revision.Apr 16 2017, 8:12 PM

Thanks!

So far I only found two issues (see comments).
Apart from that it would be great to see the application name and the target/source file in the polkit dialog, but I assume this is out of scope here.

src/buffer/katesecuretextbuffer.cpp
88

This is racy: If the newly set permissions allow someone to delete the file, it can be replaced with a symlink and the chown will take effect on the symlink target, which can be literally anything -> escalation.

This is not an issue for the rename call as if the file permissions allow deleting, they allow deleting for the destination file as well -> no escalation.

Solution: Use fchown.

92

The destructor of QTemporaryFile here tries to unlink the temporary file here, which fails if the rename was successful.

This revision now requires changes to proceed.Apr 16 2017, 8:12 PM
martinkostolny edited edge metadata.

Thanks for noticing all these security issues!

Both issues should now be fixed.

Regarding the KAuth dialog, I agree, but couldn't find a simple way of propagating additional info to the dialog. I didn't dig in KAuth code much but so far I didn't find a way. It looks like it isn't supported. KAuth::Action.setDetails(...) doesn't really add anything in the dialog.

martinkostolny marked 2 inline comments as done.Apr 17 2017, 4:21 PM
fvogt added a comment.Apr 18 2017, 1:10 PM

Thanks!

Only one issue remaining (I somehow missed during the last time): Directory rename race.

Someone with the appropriate permissions (unlikely) could change the directory the path refers to in between the verification and the chmod/fchown/rename.

Solution: Use relative paths, see also man 2 openat ("Rationale for openat() and other directory file descriptor APIs").

Understood and implemented by switching to "current directory" where the final rename is taking place. This way I could use filenames only rename. Hopefully I didn't miss anything.

One other issue I've noticed during testing:

  • open Kate to edit a privileged file1
  • open KWrite to edit a different privileged file2
  • edit & save file1 (do not enter password yet)
  • edit & save file2 -> it seems nothing happens
  • fill password to existing KAuth dialog and send -> both editors say "saved", although only the first one is really saved

...this looks like a bug in KAuth since I only should get job->exec() result when my particular job was finished. I tried to differentiate the jobs by returning a unique data in reply.data() map and retrieving them in job->data() map, but that does not work either (another bug?). I'll try to investigate KAuth code further and report/propose-fix in proper facilities :).

fvogt added a comment.Apr 20 2017, 7:04 AM

Thanks again!
The changes look good, but strace shows that the QFile operation still use absolute paths, apparently it resolves the "." manually with getcwd:

chdir("/tmp")                           = 0
stat("/tmp/permfile", {st_mode=S_IFREG|0644, st_size=5, ...}) = 0
open("/tmp/kate.X31571", O_RDONLY|O_CLOEXEC) = 11
fstat(11, {st_mode=S_IFREG|0600, st_size=14, ...}) = 0
getcwd("/tmp", 4096)                    = 5
open("/tmp/permfile.J31580", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 12
[...]
stat("/tmp/permfile", {st_mode=S_IFREG|0644, st_size=5, ...}) = 0
chmod("/tmp/permfile.J31580", 0644)     = 0
fchown(12, 0, 0)                        = 0
rename("permfile.J31580", "permfile")
...this looks like a bug in KAuth since I only should get job->exec() result when my particular job was finished. I tried to differentiate the jobs by returning a unique data in reply.data() map and retrieving them in job->data() map, but that does not work either (another bug?). I'll try to investigate KAuth code further and report/propose-fix in proper facilities :).

Yup, looks like a KAuth bug to me as well.

martinkostolny updated this revision to Diff 14013.EditedApr 30 2017, 12:21 AM

Thanks for noticing the security issues! And sorry for the pause. Here is an updated diff which should ensure that QFile is using relative path. I've managed to reduce the use of absolute paths to this state - strace (saving privileged example2.txt file inside ~/Downloads):

chdir("/home/kotelnik/Downloads")       = 0
stat("example2.txt", {st_mode=S_IFREG|0640, st_size=1085659, ...}) = 0
getcwd("/home/kotelnik/Downloads", 4096) = 25
getpid()                                = 3343
open("/home/kotelnik/Downloads/example2.txt.TJ3343", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 10
fcntl(10, F_SETFD, FD_CLOEXEC)          = 0
lseek(10, 0, SEEK_SET)                  = 0
close(10)                               = 0
open("/tmp/kate.nS3280", O_RDONLY|O_CLOEXEC) = 10
fcntl(10, F_SETFD, FD_CLOEXEC)          = 0
fstat(10, {st_mode=S_IFREG|0600, st_size=1085661, ...}) = 0
open("example2.txt.TJ3343", O_RDWR|O_CREAT|O_CLOEXEC, 0666) = 11
[...]
stat("example2.txt", {st_mode=S_IFREG|0640, st_size=1085659, ...}) = 0
access("example2.txt", R_OK)            = 0
access("example2.txt", W_OK)            = 0
access("example2.txt", X_OK)            = -1 EACCES (Permission denied)
chmod("example2.txt.TJ3343", 0640)      = 0
fchown(11, 33, 33)                      = 0
rename("example2.txt.TJ3343", "example2.txt") = 0

One not-nice part in code is opening and immediately closing the QTemporaryFile (the only use of absolute path). Then tempFile is opened again with relative path and written to like before. I wanted to make use of the convenient way of creating unique temporary filename. Other suggestions are welcome :).

Regarding parallel KAuth actions I didn't manage to understand KAuth code enough to make a fix myself. So there comes 2 bug reports:

One more thing I've changed in the last diff:
When creating a new file in privileged directory, the file is then set to be readable by group and others. This is because kate wouldn't be able to read it later with regular user who managed to create it in the first place. I realize this is controversial so I will gladly remove this change if we agree on that. Probably the best approach would be to ask for permission to read it with another KAuth action, right?

fvogt accepted this revision.Apr 30 2017, 7:47 AM

The strace snipped you posted looks good to me!

That new files are created with 0644 permissions is fine, the default umask is 022 anyway so running "echo test > file" as root would result in the same permissions.

This revision is now accepted and ready to land.Apr 30 2017, 7:47 AM
This revision was automatically updated to reflect the committed changes.
volkov added a subscriber: volkov.Sep 24 2019, 12:27 PM

Wouldn't it be better to avoid creation of a temporary file by opening an original file for writing in the helper and passing the file descriptor back to the main process?

Restricted Application added a project: Kate. · View Herald TranscriptSep 24 2019, 12:27 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald Transcript

D24245 - support for passing Unix file descriptors in KAuth.