Bug 213538

Summary: KSaveFile loose gid permissions and guid on save (if the user is not owner or root)
Product: [Unmaintained] kdelibs Reporter: stephane <stephane.bouthors>
Component: kdecoreAssignee: kdelibs bugs <kdelibs-bugs>
Status: RESOLVED FIXED    
Severity: normal    
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description stephane 2009-11-07 13:46:26 UTC 
Version:            (using KDE 4.3.2)
Compiler:          g++ (Ubuntu 4.4.1-4ubuntu8) 4.4.1  g++ (Ubuntu 4.4.1-4ubuntu8) 4.4.1 
OS:                Linux
Installed from:    Ubuntu Packages

If you use KSaveFile from a different owner you loose guid and group permissions:

example:
1) I have two user: (both member of group_common)
user1 => group: user1 group_common
user2 => group: user2 group_common

2) user1 create a file with following right:
-rw-rw-r--  1 user1 group_common   myfile.jpg

3) user2 rotate the jpeg with gwenview and save (gwenview use KSaveFile for saving changes)

4)result: gid is changed and user1 has no more permissions on the file:
-rw-------  1 user2 user2   myfile.jpg
Comment 1 stephane 2009-11-07 14:00:43 UTC 
I have found the error:
in open function, fchown is used to uid and gid and permissions are set only on success.
It can succeed only for root or if uid are the same (user and file). 
It explains the BUG.


Here is my fix proposal:


original code: (look in open)

115     // if we're overwriting an existing file, ensure temp file's
116     // permissions are the same as existing file so the existing
117     // file's permissions are preserved. this will succeed only if we
118     // are the same owner and group - or allmighty root.
119     QFileInfo fi ( d->realFileName );
120     if (fi.exists()) {
121         //Qt apparently has no way to change owner/group of file :(
122         if (!fchown(tempFile.handle(), fi.ownerId(), fi.groupId()))
123             tempFile.setPermissions(fi.permissions());
124     }
125     else {
126         mode_t umsk = KGlobal::umask();
127         fchmod(tempFile.handle(), 0666&(~umsk));
128     }


mine:

   QFileInfo fi ( d->realFileName );
    if (fi.exists())
    {
        // set permissions
        tempFile.setPermissions(fi.permissions());

        // Qt apparently has no way to change owner/group of file :(
        // try to set user and group (changing user may requiere root privilege)
        if (fchown(tempFile.handle(), fi.ownerId(), fi.groupId()))
        {
            // failed to set user and group => try to restore group anyway
            fchown(tempFile.handle(), -1, fi.groupId());
        }
    }
    else {
        mode_t umsk = KGlobal::umask();
        fchmod(tempFile.handle(), 0666&(~umsk));
    }



=> may a better way should be to set separetly gid and uid.
Hope it helps!
Comment 2 stephane 2009-11-07 14:08:43 UTC 
the file is part of kdelibs/kdecore :
kde4libs-4.3.2/kdecore/io/ksavefile.cpp
Comment 3 Oswald Buddenhagen 2010-03-07 20:19:08 UTC 
SVN commit 1100544 by ossi:

don't lock out the original owner when stealing a file

there should be no harm in applying the original file's permissions even
if the chown failed - after all, these permissions made it possible for
us to work with the file, so why would we suddenly have an interest in
disallowing others to do the same?

BUG: 213538

 M  +8 -4      ksavefile.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1100544