After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 678661 - context plugin uses insecure temporary directory
context plugin uses insecure temporary directory
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: Plugins (other)
HEAD
Other Linux
: Normal critical
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-06-23 08:42 UTC by Josselin Mouette
Modified: 2012-07-24 22:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use mkdtemp to securely create temp dir (4.38 KB, patch)
2012-07-05 15:22 UTC, Andreas Henriksson
none Details | Review

Description Josselin Mouette 2012-06-23 08:42:44 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.
Comment 1 Andreas Henriksson 2012-07-05 15:22:04 UTC
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....)
Comment 2 James Strandboge 2012-07-09 15:47:23 UTC
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[1][2] 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.

[1]http://docs.python.org/tutorial/modules.html
[2]http://docs.python.org/py3k/tutorial/modules.html
Comment 3 Vincent Untz 2012-07-23 10:08:19 UTC
Can we get this patch in? Or is there any issue with it?
Comment 4 James Strandboge 2012-07-23 16:14:04 UTC
FYI, I pushed this out as a distro patch in Ubuntu with no reported issues.
Comment 5 Jonathan Matthew 2012-07-24 10:46:30 UTC
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.