Opened 8 years ago
Last modified 8 years ago
#5825 new Bug
Prevent relative or invalid paths to public functions
Reported by: | cfpp2p | Owned by: | |
---|---|---|---|
Priority: | Normal | Milestone: | None Set |
Component: | Transmission | Version: | 2.84 |
Severity: | Normal | Keywords: | |
Cc: |
Description
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.
Attachments (3)
Change History (7)
Changed 8 years ago by cfpp2p
comment:1 follow-ups: ↓ 2 ↓ 4 Changed 8 years ago by mike.dld
Changed 8 years ago by mike.dld
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 8 years ago by 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.
comment:3 in reply to: ↑ 2 Changed 8 years ago by 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.
comment:4 in reply to: ↑ 1 Changed 8 years ago by 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.
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:
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.