Bug 142148 - Directory permission checks for .Trash directory are to restrictive
Summary: Directory permission checks for .Trash directory are to restrictive
Alias: None
Product: kio
Classification: Unclassified
Component: trash (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: David Faure
Depends on:
Reported: 2007-02-24 17:14 UTC by Stefan Brüns
Modified: 2009-01-26 17:41 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:

trashdir.diff (1.35 KB, text/x-diff)
2007-02-26 21:42 UTC, David Faure

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Brüns 2007-02-24 17:14:52 UTC
Version:            (using KDE KDE 3.5.6)
Installed from:    Unspecified

Relevant file:
QString TrashImpl::trashForMountPoint( const QString& topdir, bool createIfNeeded ) const

According to Trash specification:
"An administrator can create an $topdir/.Trash directory. The permissions on this directories should permit all users who can trash files at all to write in it.; and the “sticky bit” in the permissions must be set, if the file system supports it."

Consider a user "jeff" with groups "jeff,staff". If $topdir/.Trash has permissions "1770" belonging to root:staff, it should be fine for this user.

Even ownership by the root user is not enforced by the specification. It only has to be writable, accessable and must have the sticky bit set, so permissions "1730" would be enough (.Trash does _not_ have to be readable by the user, it is enough that there is a readable/accessible directory $uid in it).
Comment 1 David Faure 2007-02-26 21:42:08 UTC
You are right. I think I special-cased "the directory has to be writable by the user", but instead of assuming a certain uid + permissions
I should just use access()...
What do you think of the attached patch?

Created an attachment (id=19832)
Comment 2 David Faure 2007-05-04 10:49:25 UTC
Any feedback on the patch?
Comment 3 David Faure 2007-05-04 11:07:49 UTC
Seems to work, actually, I'll commit.
Comment 4 David Faure 2007-05-04 11:11:11 UTC
SVN commit 660968 by dfaure:

[Bug 142148] Directory permission checks for .Trash directory are to restrictive
It's actually fine if root-owned .Trash isn't readable/writable by users, users only need to be able to access their own subdir.
BUG: 142148

 M  +5 -5      trashimpl.cpp  

--- branches/KDE/3.5/kdebase/kioslave/trash/trashimpl.cpp #660967:660968
@@ -786,16 +786,16 @@
     // (1) Administrator-created $topdir/.Trash directory
     const QString rootTrashDir = topdir + "/.Trash";
+    const QCString rootTrashDir_c = QFile::encodeName( rootTrashDir );
     // Can't use QFileInfo here since we need to test for the sticky bit
     uid_t uid = getuid();
     KDE_struct_stat buff;
-    // Minimum permissions required: write+execute for 'others', and sticky bit
-    uint requiredBits = S_IWOTH | S_IXOTH | S_ISVTX;
-    if ( KDE_lstat( QFile::encodeName( rootTrashDir ), &buff ) == 0 ) {
-        if ( (buff.st_uid == 0) // must be owned by root
-             && (S_ISDIR(buff.st_mode)) // must be a dir
+    const uint requiredBits = S_ISVTX; // Sticky bit required
+    if ( KDE_lstat( rootTrashDir_c, &buff ) == 0 ) {
+        if ( (S_ISDIR(buff.st_mode)) // must be a dir
              && (!S_ISLNK(buff.st_mode)) // not a symlink
              && ((buff.st_mode & requiredBits) == requiredBits)
+             && (::access(rootTrashDir_c, W_OK))
             ) {
             const QString trashDir = rootTrashDir + "/" + QString::number( uid );
             const QCString trashDir_c = QFile::encodeName( trashDir );
Comment 5 David Faure 2009-01-26 17:41:21 UTC
SVN commit 916987 by dfaure:

Fix wrong fix for 142148, I missed the ==0 so the logic was reversed, oops.
CCBUG: 142148
CCBUG: 178479
This fix will be in 4.2.1, probably not in 4.2.0.

 M  +4 -2      trashimpl.cpp  

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