Prevent relative or invalid paths to public functions

[cfpp2p]

Currently libtransmission allows clients or settings.json to supply relative and invalid paths containing values such as ../, "", ..

This can cause problems such as hangs and crashes especially when an empty or null string is supplied for the download or incomplete directory. Other unwanted results occur when directory traversal is allowed.

I've attached my working test patch userClientSentBadPath.diff which addresses the following issues.

1) path_is_bad() function to determine what to disallow.

2) In torrentInit() don't allow torrents to start and post appropriate error with tr_torrentSetLocalError()

3) In torrentStart() post an error with tr_torrentSetLocalError() and prevent torrent starting.

4) In tr_torrentVerify() don't verify data in inaccessible or invalid directories.

5) In deleteLocalData() don't allow deletion or creation of temp directory in inaccessible or invalid directories. This also prevents a hang when the invalid directory does not have the correct permissions. Log an error.

6) In tr_torrentSetLocation() Don't allow bad directories and post appropriate log messages.

7) In tr_torrentFindFile2() prevent crashes when the download directory is null. Maybe also disallow saving of the resume file containing invalid paths. Patching resume.c could do that. Right now I'm just testing disallowing resume null directories but path_is_bad() could be used for that also.

This is a starting point so feel free about comments and alternative patches or ideas.

This patch doesn't supply any messages to the sending client.

[mike.dld]

First thing noticed --- you don't follow existing code formatting style. I'm sure you're aware of implications (e.g. see #3262). We all have our own preferences, but we also work together on a single project.

Now to the code itself:

1. Paths containing ".." of any form aren't the issue at all. The issue is relative paths (as in "paths relative to current working directory", so paths which don't start with "/" on *NIX or "X:\" or "\\" on Windows), because user wouldn't (and shouldn't) know current working directory of transmission process handling those paths, which would lead to files/directories being created in arbitrary places or not created at all due to lack of permissions. And in this regard I don't think path_is_bad does its job now.
2. torrentInit is only called from tr_torrentNew which in turn allocates tr_torrent structure with tr_new0 which means tor->downloadDir and tor->incompleteDir are already NULL so those two lines (897 and 902) are superfluous.
3. tor->isRunning is boolean but you assign 0 (not false) to it.
4. I'd rephrase some messages. Make them consistent in paths quoting ("(%s)" vs. "\"%s\"", I prefer the latter). Don't use "currentDir" term: it's not a real English word and also users don't know what it means; same goes for "set-location"; just remember that usually users are those who read the log, not developers.
5. I feel like conditional expressions could be simplified in a couple of places, but have no real proposal here.

Overall I'm not sure whether allowing bad paths but failing operations relying on them is better than disallowing bad paths in the first place. If we have N places where paths get into libtransmission and M places where those paths are being used, and N < M, I'd go with adding checks to N places.

Since I was working on this issue as well, I'd like to share at least a small bit which you may find useful: a function detecting whether the path is relative or not.

[cfpp2p]

Replying to mike.dld:

Thank you for the response.

And in this regard I don't think path_is_bad does its job now.

My initial appreciation of the idea for me was to simply stop the crashes and hangs I was experiencing due to supplying a quantity of ridiculous paths to libtransmission. So in this regard the patch was successful. I agree that a well constructed path_is_bad() that addresses all the relevant issues and platforms is what's needed in the long run. I have a different path_is_bad() for Windows and certainly we need to put this together for all the platforms.

Since I was working on this issue as well, I'd like to share at least a small bit which you may find useful: a function detecting whether the path is relative or not.

I didn't mean to step in front of you but merely meant to share the work I'd done as a starting point or inspiration for a final clean platform independent approach. Thanks for the tr_sys_path_is_relative patch.

[mike.dld]

Replying to cfpp2p:

I didn't mean to step in front of you but merely meant to share the work I'd done as a starting point or inspiration for a final clean platform independent approach. Thanks for the tr_sys_path_is_relative patch.

Oh, I don't have any other patch to share, so you could proceed by all means :) As I didn't want to change existing public API before any approval from core devs (and changing it is what I think is the right thing to do here), I never finished my patch.

[cfpp2p]

Here is a patch that instead of just warning the user about bad paths during torrentInit or torrentStart we reset the download directory to defaults based on platform.c -->> tr_getDefaultDownloadDir().

https://trac.transmissionbt.com/wiki/ConfigFiles#Locations https://trac.transmissionbt.com/wiki/ConfigFiles

I'm not sure I'd want to do this for all types of platforms & devices such as routers or NAS or after the user has already set (even if a bad path) it to something other than the platform default. I tested my first patch thoroughly but I haven't done the same for all platforms for this new patch.

Replying to mike.dld:

I have tried better to follow existing code formatting style.

I made adjustments concerning your points 2., 3. and 4.

same goes for "set-location";

The "set-location" words are used in the web client and QT anyway.

web index.html <h2 class="dialog_heading">Set Location</h2> <li id="context_move">Set Location…</li> QT relocate.cc hig->addSectionTitle (tr ("Set Location"));

but I did change it from "set-location" to "set location"

. . . I'm not sure that we shouldn't include just enough in path_is_bad() to crash proof the application rather than try to "idiot proof" what paths a user can choose. Right now we can set paths via settings.json, clients or by individual torrent that transmission will hang, crash or a denial of service upon restart (due to consequential resume files be created). A patch changing public API would be a lot of work to pursue without core devs approval. At the very least stopping the crashes, hangs and DOS should be attended to and these relatively simple patches address that.