Bug 103331 - Group should be preserved when making backup
Summary: Group should be preserved when making backup
Status: REOPENED
Alias: None
Product: frameworks-kio
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: openSUSE Linux
: NOR normal
Target Milestone: ---
Assignee: Martin Koller
URL:
Keywords:
: 103333 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-04-06 00:33 UTC by bjoernv
Modified: 2021-09-20 16:29 UTC (History)
7 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description bjoernv 2005-04-06 00:33:59 UTC
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 $
Comment 1 bjoernv 2005-04-06 00:38:20 UTC
Kwrite has the same problem: see Bug #103333
Comment 2 Christoph Cullmann 2005-04-07 18:47:05 UTC
*** Bug 103333 has been marked as a duplicate of this bug. ***
Comment 3 Christoph Cullmann 2005-04-07 18:51:40 UTC
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 ;)
+    }
   }
 
Comment 4 Dominik Haumann 2005-04-07 23:37:56 UTC
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 ;)
+    }
   }
 
Comment 5 bjoernv 2007-01-16 14:27:11 UTC
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
Comment 6 David Faure 2007-01-17 19:10:08 UTC
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.
Comment 7 bjoernv 2007-01-17 19:23:23 UTC
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.  
Comment 8 David Faure 2007-01-17 19:28:45 UTC
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...
Comment 9 bjoernv 2007-01-17 19:39:40 UTC
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").
Comment 10 David Faure 2007-01-18 01:19:08 UTC
Hmm, right. So this is indeed about chown+chgrp.
Comment 11 David Faure 2007-08-20 13:56:57 UTC
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).
Comment 12 Christoph Cullmann 2007-11-08 19:12:42 UTC
In which way the kate code needs fixes for KDE 4 now? Would be happy about some hints.
Comment 13 David Faure 2007-11-09 12:28:19 UTC
Kate doesn't need fixing for this, as long as it uses KIO::file_copy. The fix has to be made in kio_file.
Comment 14 Christoph Feck 2011-07-27 13:09:57 UTC
David, what is the status of this bug?
Comment 15 Martin Koller 2017-01-22 19:50:47 UTC
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
Comment 16 Maximiliano Curia 2017-04-04 10:02:03 UTC
(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,
Comment 17 David Faure 2017-10-07 17:38:56 UTC
Martin Koller, can you take a look at the regression due to your patch, as per the last comment?
Comment 18 bjoernv 2017-10-07 22:27:07 UTC
(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
Comment 19 Christoph Feck 2017-10-25 18:02:29 UTC
Martin, any update?
Comment 20 Martin Koller 2017-10-30 06:02:07 UTC
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.
Comment 21 David Faure 2021-08-22 08:28:56 UTC
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
Comment 22 Ahmad Samir 2021-09-19 10:09:43 UTC
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
Comment 23 Nate Graham 2021-09-20 16:29:04 UTC
Anything more to do here, or is this fully fixed now?