Version: (using KDE KDE 3.4.0) Installed from: SuSE RPMs OS: Linux When kate creates backup files during saving, this files may have more permissions than the original file. This can be a security problem, if the original file contains sensitive information. The following script show the problem. The user uses a umask of 0022 and creates a file with permissions 600. After editing and saving the file with kate, the backup file has permission 644. I suggest, that the backup files should be created with the same permissions like the original. Sample script: -------------- /tmp/dir $ umask 0022 /tmp/dir $ touch afile.txt /tmp/dir $ ls -l total 0 -rw-r--r-- 1 bv bv 0 Apr 6 00:27 afile.txt /tmp/dir $ chmod 600 afile.txt /tmp/dir $ ls -l total 0 -rw------- 1 bv bv 0 Apr 6 00:27 afile.txt /tmp/dir $ kate afile.txt /tmp/dir $ ls -l total 4 -rw------- 1 bv bv 8 Apr 6 00:27 afile.txt -rw-r--r-- 1 bv bv 0 Apr 6 00:27 afile.txt~ /tmp/dir $
Kwrite has the same problem: see Bug #103333
*** Bug 103333 has been marked as a duplicate of this bug. ***
CVS commit by cullmann: security fix: use right permissions for the backup files if the filepermissions of the original are not accessable, why ever, use 0600 as fallback dominik: please test and backport BUG:103331 M +31 -5 katedocument.cpp 1.818 --- kdelibs/kate/part/katedocument.cpp #1.817:1.818 @@ -45,4 +45,6 @@ #include <kio/job.h> #include <kio/netaccess.h> +#include <kio/kfileitem.h> + #include <kparts/event.h> @@ -2431,13 +2433,37 @@ bool KateDocument::openFile(KIO::Job * j bool KateDocument::save() { - // FIXME reorder for efficiency, prompt user in case of failure bool l ( url().isLocalFile() ); - if ( ( ( l && config()->backupFlags() & KateDocumentConfig::LocalFiles ) || - ( ! l && config()->backupFlags() & KateDocumentConfig::RemoteFiles ) ) - && isModified() ) { + + if ( ( l && config()->backupFlags() & KateDocumentConfig::LocalFiles ) + || ( ! l && config()->backupFlags() & KateDocumentConfig::RemoteFiles ) ) + { KURL u( url() ); u.setFileName( config()->backupPrefix() + url().fileName() + config()->backupSuffix() ); - if ( ! KIO::NetAccess::upload( url().path(), u, kapp->mainWidget() ) ) + + kdDebug () << "backup src file name: " << url() << endl; + kdDebug () << "backup dst file name: " << u << endl; + + // get the right permissions, start with safe default + mode_t perms = 0600; + KIO::UDSEntry fentry; + if (KIO::NetAccess::stat (url(), fentry, kapp->mainWidget())) + { + kdDebug () << "stating succesfull: " << url() << endl; + KFileItem item (fentry, url()); + perms = item.permissions(); + } + + // first del existing file if any, than copy over the file we have + // failure if a: the existing file could not be deleted, b: the file could not be copied + if ( (!KIO::NetAccess::exists( u, false, kapp->mainWidget() ) || KIO::NetAccess::del( u, kapp->mainWidget() )) + && KIO::NetAccess::file_copy( url(), u, perms, true, false, kapp->mainWidget() ) ) + { + kdDebug(13020)<<"backing up successfull ("<<url().prettyURL()<<" -> "<<u.prettyURL()<<")"<<endl; + } + else + { kdDebug(13020)<<"backing up failed ("<<url().prettyURL()<<" -> "<<u.prettyURL()<<")"<<endl; + // FIXME: notify user for real ;) + } }
CVS commit by dhaumann: cullmann-backport: security fix: use right permissions for the backup files if the filepermissions of the original are not accessable, why ever, use 0600 as fallback CCBUG:103331 M +31 -5 katedocument.cpp 1.796.2.5 --- kdelibs/kate/part/katedocument.cpp #1.796.2.4:1.796.2.5 @@ -46,4 +46,6 @@ #include <kio/job.h> #include <kio/netaccess.h> +#include <kio/kfileitem.h> + #include <kparts/event.h> @@ -2750,13 +2752,37 @@ bool KateDocument::openFile(KIO::Job * j bool KateDocument::save() { - // FIXME reorder for efficiency, prompt user in case of failure bool l ( url().isLocalFile() ); - if ( ( ( l && config()->backupFlags() & KateDocumentConfig::LocalFiles ) || - ( ! l && config()->backupFlags() & KateDocumentConfig::RemoteFiles ) ) - && isModified() ) { + + if ( ( l && config()->backupFlags() & KateDocumentConfig::LocalFiles ) + || ( ! l && config()->backupFlags() & KateDocumentConfig::RemoteFiles ) ) + { KURL u( url() ); u.setFileName( config()->backupPrefix() + url().fileName() + config()->backupSuffix() ); - if ( ! KIO::NetAccess::upload( url().path(), u, kapp->mainWidget() ) ) + + kdDebug () << "backup src file name: " << url() << endl; + kdDebug () << "backup dst file name: " << u << endl; + + // get the right permissions, start with safe default + mode_t perms = 0600; + KIO::UDSEntry fentry; + if (KIO::NetAccess::stat (url(), fentry, kapp->mainWidget())) + { + kdDebug () << "stating succesfull: " << url() << endl; + KFileItem item (fentry, url()); + perms = item.permissions(); + } + + // first del existing file if any, than copy over the file we have + // failure if a: the existing file could not be deleted, b: the file could not be copied + if ( (!KIO::NetAccess::exists( u, false, kapp->mainWidget() ) || KIO::NetAccess::del( u, kapp->mainWidget() )) + && KIO::NetAccess::file_copy( url(), u, perms, true, false, kapp->mainWidget() ) ) + { + kdDebug(13020)<<"backing up successfull ("<<url().prettyURL()<<" -> "<<u.prettyURL()<<")"<<endl; + } + else + { kdDebug(13020)<<"backing up failed ("<<url().prettyURL()<<" -> "<<u.prettyURL()<<")"<<endl; + // FIXME: notify user for real ;) + } }
I reopened this bug because there is still a situation where the backup file may have too much permissions. I found the problem in Kate and Kwrite (KDE 3.5.5). Currently Kate copies the permissions from the original file to the backup file. There is a potential problem if the original file is not owned by the standard group of the user. Kate creates the backup file with the standard group. Two files with the same permission bits but with other owners or groups can have different effective permissions. For fixing the problem there are two things to consider: 1) Kate should try to change the owner and group of the backup file according to the original file. 2) chown and chmod may fail. In this case Kate should copy the usually lower permissions from the other/world entry to the group entry. I can recommend Gedit for example code: http://svn.gnome.org/svn/gedit/tags/GEDIT_2_16_2/gedit/gedit-document-saver.c
Retitling. Indeed the group should be preserved when making a backup file. This needs fixing at several levels in KIO, pretty much like all the places where the modification time is preserved when copying a file. Added to my TODO list for kde4.
Yes, good. In cases, where the group could not be preserved, special lower permissions from other/world could be used. For user root, the file owner could also be preserved. I saw such code in Gedit and VIM.
On Wednesday 17 January 2007 19:23, bjoern@cs.tu-berlin.de wrote: > For user root, the file owner could also be preserved. Well, KDE is not supposed to be run as root...
Yes, I agree that running KDE as root is generally no good idea. But running a KDE editor (kate, kwrite) as root? Some users may use the KDE editors for editing system files (e.g. "kdesu kate /etc/hosts").
Hmm, right. So this is indeed about chown+chgrp.
Martin Koller added KIO::chown() [which takes user and group] in kdelibs4. However I think the actual fix for file_copy is to add a chown+chgrp call at the end of FileProtocol::copy(), much like we preserve modification time there, and a call in CopyJob for the case of copying directories (but that's more for konqueror than for kate).
In which way the kate code needs fixes for KDE 4 now? Would be happy about some hints.
Kate doesn't need fixing for this, as long as it uses KIO::file_copy. The fix has to be made in kio_file.
David, what is the status of this bug?
Git commit d252f7958633f3374e77e1cc52f11c534ca55219 by Martin Koller. Committed on 22/01/2017 at 19:49. Pushed by mkoller into branch 'master'. preserve group/owner on file copy REVIEW: 129864 M +28 -12 src/ioslaves/file/file_unix.cpp https://commits.kde.org/kio/d252f7958633f3374e77e1cc52f11c534ca55219
(In reply to Martin Koller from comment #15) > Git commit d252f7958633f3374e77e1cc52f11c534ca55219 by Martin Koller. > Committed on 22/01/2017 at 19:49. > Pushed by mkoller into branch 'master'. > > preserve group/owner on file copy > REVIEW: 129864 > > M +28 -12 src/ioslaves/file/file_unix.cpp > > https://commits.kde.org/kio/d252f7958633f3374e77e1cc52f11c534ca55219 This patch breaks the setgid directories and default acl semantics. It might be better to avoid setting the group if the dest dir is setgid, and avoid setting the file permissions if the destdir has a default acl value. Happy hacking,
Martin Koller, can you take a look at the regression due to your patch, as per the last comment?
(In reply to Maximiliano Curia from comment #16) > This patch breaks the setgid directories and default acl semantics. It might > be better to avoid setting the group if the dest dir is setgid, and avoid > setting the file permissions if the destdir has a default acl value. I agree in case of normal copy operations. But in case of Kate editor's backups (initial subject of this report) it may not be wise to ignore to set permissions or groups in special situations. Both can again result in situations, where some users have more permissions on the backup file than on the original. A flag like "--preserve" for the "cp" command can be used to make preserving of permissions and groups optional. I can recommend the file operations code of the VIM editor. It contains code for handling backups and all kinds of ACLs: https://github.com/vim/vim/blob/master/src/fileio.c https://github.com/vim/vim/blob/master/src/os_unix.c
Martin, any update?
It seems there is disagreement between people to how this actually should work. Since I have no experience with ACL, I did not look into this further.
Git commit ff4bb61b2f5492f1942c1f11fe4c8b9a7eb654ea by David Faure, on behalf of Ahmad Samir. Committed on 22/08/2021 at 08:28. Pushed by dfaure into branch 'master'. file_unix: minor cleanup Separate ::chmod() from acl_set_file() call, more readable that way. If the permissions are -1, that means preserve the permissions/attributes of the source file, this matches what the code is/was doing and the docs. Maybe we should add a couple more setters for the job, e.g. setPermissions(), setAcl(), then users can fine-tune the permissions of the dest, preserve, don't set, leave as default M +9 -5 src/ioslaves/file/file_unix.cpp https://invent.kde.org/frameworks/kio/commit/ff4bb61b2f5492f1942c1f11fe4c8b9a7eb654ea
Git commit de7aff60e81b05239fd3d6c17895c48a985dd27a by Ahmad Samir. Committed on 19/09/2021 at 10:06. Pushed by ahmadsamir into branch 'master'. Fix permissions when copying files This reverts commit 7863f595991c5, I was wrong :) Reading the code some more (in copyjob, file_copy and file_unix), the permissions arg is '-1' when we shouldn't touch the dest permissions at all, i.e. leave them as the system default permissions for newly created files (umask ...etc). The scenarios are: - calling KIO::copy() A to B, permissions are preserved by default, i.e. the job copies A's permissions to the permissions arg when it calls file_copy - calling KIO::copy() A to B, and calling setDefaultPermissions(true), the job sets the permissions arg to '-1' when it calls file_copy - calling KIO::file_copy() directly, the you have full control, if you want to preserve the permissions use A's permissions as the permissions arg, otherwise use '-1' to use the system default permissions ACL attributes are handled implicitly at the moment, which isn't ideal but can be improved later on. Related: bug 404777 FIXED-IN: 5.87 M +4 -6 src/core/filecopyjob.h M +15 -22 src/ioslaves/file/file_unix.cpp https://invent.kde.org/frameworks/kio/commit/de7aff60e81b05239fd3d6c17895c48a985dd27a
Anything more to do here, or is this fully fixed now?