103331 2005-04-06 00:33:59 +0000 Group should be preserved when making backup 2026-02-06 17:58:52 +0000 1 1 3 Frameworks and Libraries frameworks-kio general unspecified openSUSE Linux RESOLVED FIXED NOR normal --- 1 bjoernv martin cfeck christoph faure kdedev kdelibs-bugs-null martin maxy nate https://commits.kde.org/kio/d252f7958633f3374e77e1cc52f11c534ca55219 0 oldest_to_newest 332134 0 bjoernv 2005-04-06 00:33:59 +0000 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 $ 332139 1 bjoernv 2005-04-06 00:38:20 +0000 Kwrite has the same problem: see Bug #103333 332661 2 christoph 2005-04-07 18:47:05 +0000 *** Bug 103333 has been marked as a duplicate of this bug. *** 332663 3 christoph 2005-04-07 18:51:40 +0000 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 ;) + } } 332728 4 dhaumann 2005-04-07 23:37:56 +0000 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 ;) + } } 501956 5 bjoernv 2007-01-16 14:27:11 +0000 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 502178 6 faure 2007-01-17 19:10:08 +0000 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. 502181 7 bjoernv 2007-01-17 19:23:23 +0000 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. 502182 8 faure 2007-01-17 19:28:45 +0000 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... 502184 9 bjoernv 2007-01-17 19:39:40 +0000 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"). 502254 10 faure 2007-01-18 01:19:08 +0000 Hmm, right. So this is indeed about chown+chgrp. 541778 11 faure 2007-08-20 13:56:57 +0000 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). 554188 12 christoph 2007-11-08 19:12:42 +0000 In which way the kate code needs fixes for KDE 4 now? Would be happy about some hints. 554277 13 faure 2007-11-09 12:28:19 +0000 Kate doesn't need fixing for this, as long as it uses KIO::file_copy. The fix has to be made in kio_file. 1146085 14 cfeck 2011-07-27 13:09:57 +0000 David, what is the status of this bug? 1656091 15 martin 2017-01-22 19:50:47 +0000 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 1669863 16 maxy 2017-04-04 10:02:03 +0000 (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, 1704815 17 faure 2017-10-07 17:38:56 +0000 Martin Koller, can you take a look at the regression due to your patch, as per the last comment? 1704827 18 bjoernv 2017-10-07 22:27:07 +0000 (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 1708132 19 cfeck 2017-10-25 18:02:29 +0000 Martin, any update? 1708912 20 martin 2017-10-30 06:02:07 +0000 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. 2055130 21 faure 2021-08-22 08:28:56 +0000 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 2062121 22 a.samirh78 2021-09-19 10:09:43 +0000 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 2062468 23 nate 2021-09-20 16:29:04 +0000 Anything more to do here, or is this fully fixed now? 2491979 24 kdedev 2026-02-06 17:58:52 +0000 Closing, due to the age of this issue. If there is still work to be done, please open a new report. Thanks!