Version: (using KDE KDE 3.5.6) Installed from: Unspecified Relevant file: http://websvn.kde.org/trunk/KDE/kdebase/runtime/kioslave/trash/trashimpl.cpp?revision=635967&view=markup Function: 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).
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) trashdir.diff
Any feedback on the patch?
Seems to work, actually, I'll commit.
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 );
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