Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't follow symlinks when removing junk files #1638

Merged
merged 2 commits into from Jan 21, 2022

Conversation

Narthorn
Copy link
Contributor

@Narthorn Narthorn commented Mar 5, 2021

This prevents accidentally following a symlink that points outside of
the torrent's folder and crawling the rest of the drive for junk files
to remove.

This also prevents symlinks that point to a folder from being treated as
real directories, which would cause them to be removed even if the folder
they point to is not actually empty.

Both issues are especially bad when combined: a symlink to / inside the
torrent folder caused transmission to crawl the entire drive and delete
every single folder symlink it could find. Oops.

@lvella
Copy link
Contributor

lvella commented Mar 26, 2021

When is this function removeEmptyFoldersAndJunkFiles() called? When I remove a torrent+data, it doesn't touch files not known by the torrent...

@ckerr
Copy link
Member

ckerr commented Jan 18, 2022

@Narthorn this looks like a good PR, but it never got reviewed / merged.

Would you be willing to update this PR to fix the merge conflicts? A lot has happened in torrent.c / torrent.cc since this PR was first submitted 😄

@ckerr ckerr added needs update The PR has needs to be updated by the submitter scope:core type:fix A bug fix labels Jan 18, 2022
This prevents accidentally following a symlink that points outside of
the torrent's folder and crawling the rest of the drive for junk files
to remove.

This also prevents symlinks that point to a folder from being treated as
real directories, which would cause them to be removed even if the folder
they point to is not actually empty.

Both issues are especially bad when combined: a symlink to / inside the
torrent folder caused transmission to crawl the entire drive and delete
every single folder symlink it could find. Oops.
@Narthorn
Copy link
Contributor Author

Thank you for following up on this, I updated the patch.

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Narthorn, clang-tidy wants it to look like this:

--- a/libtransmission/torrent.cc
+++ b/libtransmission/torrent.cc
@@ -2125,7 +2125,8 @@ static void removeEmptyFoldersAndJunkFiles(char const* folder)
             auto const filename = tr_strvPath(folder, name);
 
             auto info = tr_sys_path_info{};
-            if (tr_sys_path_get_info(filename.c_str(), TR_SYS_PATH_NO_FOLLOW, &info, nullptr) && info.type == TR_SYS_PATH_IS_DIRECTORY)
+            if (tr_sys_path_get_info(filename.c_str(), TR_SYS_PATH_NO_FOLLOW, &info, nullptr) &&
+                info.type == TR_SYS_PATH_IS_DIRECTORY)
             {
                 removeEmptyFoldersAndJunkFiles(filename.c_str());
             }

and then the Code Style CI bot will be happy.

@Narthorn
Copy link
Contributor Author

The code style bot should do that itself. It's really not meaningful for humans to spend time doing that.

@mikedld
Copy link
Member

mikedld commented Jan 19, 2022

And it will in a far-far-away future, when code formatters will understand the code structure better than humans do, not requiring any manual fixes after the fact to correct changes in semantics. Until then, setting up clang-format locally is a good-enough tradeoff.

@ckerr ckerr merged commit ddca909 into transmission:main Jan 21, 2022
@ckerr ckerr removed the needs update The PR has needs to be updated by the submitter label Jan 21, 2022
ile6695 pushed a commit to sweng-group-3/transmission-1 that referenced this pull request Jan 23, 2022
… (transmission#1638)

This prevents accidentally following a symlink that points outside of
the torrent's folder and crawling the rest of the drive for junk files
to remove.

This also prevents symlinks that point to a folder from being treated as
real directories, which would cause them to be removed even if the folder
they point to is not actually empty.
ile6695 pushed a commit to sweng-group-3/transmission-1 that referenced this pull request Jan 23, 2022
@Anuskuss Anuskuss mentioned this pull request Feb 20, 2022
@Coeur
Copy link
Collaborator

Coeur commented Apr 29, 2022

@ckerr as this feels like a security risk, can we get quickly a new official Transmission release to cover that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants