GNOME Bugzilla – Bug 678661
context plugin uses insecure temporary directory
Last modified: 2012-07-24 22:01:54 UTC
Original report by Hans Spaans: http://bugs.debian.org/616673
The context plugin has the same terrible programming mistake all over:
self.album_template = Template (filename = self.path,
module_directory = '/tmp/context')
The module_directory parameter being a place for Python modules, it is very easy for another user to usurp it and put other files in place, running code in your stead.
Created attachment 218103 [details] [review]
use mkdtemp to securely create temp dir
This patch uses tempfile.mkdtemp to securely create a unique temporary directory.
(An alternative could also be to simply drop all "module_directory" and let the mako template engine regenerate everything every time....)
Part of the problem with the original implementation is that it doesn't clean up itself and will reuse /tmp/context/... unconditionally so another user or subverted process could write here and execute arbitrary code (as Josselin said). Obviously that is bad and this patch addresses that.
A concern I initially had with the proposed patch is if rhythmbox could be made to reload the imported modules. Since the .py and .pyc persist for the life of the rhythmbox process, a subverted user process could notice the rb-context-* directory in /tmp and change the modules out from underneath a running rhythmbox. Fortunately, based on upstream python documentation this is not an issue. It states:
"Note: For efficiency reasons, each module is only imported once per interpreter session. Therefore, if you change your modules, you must restart the interpreter – or, if it’s just one module you want to test interactively, use reload(), e.g. reload(modulename)."
I verified the rhymthbox code and it does not use reload(module) (it does use self.reload, but this is different). I also tested this behavior by removing the .py* files while rhythmbox was running, and I could not make rhythmbox regenerate them until after restart.
As such, this patch seems sufficient to close the security vulnerability. That said, I'm not particularly fond of how the plugin auto-generates python modules in a temporary directory and later imports them. Going forward perhaps it would be better to use your alternative of dropping the module_directory altogether.
Can we get this patch in? Or is there any issue with it?
FYI, I pushed this out as a distro patch in Ubuntu with no reported issues.
I don't see much point in using the module_directory thing. If it turns out that it's too slow without the cache, I'd prefer to use the XDG cache dir.
I briefly tested it without the module_directory setting and it seems to work fine, so I removed it in commit 01a829f.