GNOME Bugzilla – Bug 540516
backup plugin doesn't escape filename
Last modified: 2010-02-01 23:05:44 UTC
The backup plugin doesn't sanitize user input, leading to a potential security vulnerability. If you give "/tmp/evolution-backup.tar.gz;rm -rf test" as backup path, it will delete the contents of the "test" file or directory in your home folder. A proper solution is to escape the shell arguments properly. g_shell_quote seems to be a good function to do that.
Created attachment 113540 [details] [review] patch which sanitizes user input with g_shell_quote simple patch which escapes the user given input properly. I wasn't able to test it though :(
Sounds fine to me. Matt do you have any comments?
I think you're correct to call g_shell_quote(), but the function returns a newly-allocated string so you're introducing a bit of a memory leak there.
Yes. You're absolutely right. I optimized the patch.
Created attachment 113555 [details] [review] revised patch which doesn't leak memory As I've said, I couldn't test whether it works, but at least it compiles.
It should work. I will test it in build sanity. Commit to stable/trunk
Patch committed to SVN stable (gnome-2-22) branch as r35699 http://svn.gnome.org/viewvc/evolution?view=revision&revision=35699 Patch committed to SVN trunk as r35700 http://svn.gnome.org/viewvc/evolution?view=revision&revision=35700
*** Bug 555058 has been marked as a duplicate of this bug. ***
this seems to be still an issue with evolution 2.28 , We're getting a few reports commenting a similar behavior on https://bugs.edge.launchpad.net/ubuntu/+source/evolution/+bug/449242 ; File name are: " /media/disk/Ubuntu Backup/Mail/evolution-backup.tar.gz The space in the Ubuntu Backup folder being the initial problem."
This is indeed a problem: (evolution:15322): e-utils-WARNING **: No parent set, or default parent available for error dialog tar: Folder/evolution-backup.tar.gz: Cannot open: No such file or directory tar: Error is not recoverable: exiting now tar: Child returned status 2 tar: Exiting with failure status due to previous errors ** Message: First result 512 ** Message: Sanity check result 1:1 256 (evolution:15322): Gtk-CRITICAL **: gtk_window_set_transient_for: assertion `parent == NULL || GTK_IS_WINDOW (parent)' failed
Okay, I think I spotted the error. Generally speaking, using system() is somewhat dangerous. It inherits all the environment, including PATH, IFS or PYTHONPATH. That can lead to further vulnerabilities. Using exec-family and providing a sanitized environment is more secure against these types of attach. Anyway, in this particular case, the filename in sanity_check.c:72 is not quoted properly. I prepared a patch and tried to test it, but I got no more time to actually build evolution (actually building dependencies costs me a lot of time which I need for other things). So please test the following patch with the simple testcase: mkdir "/tmp/New Folder", backup Evolution settings to "/tmp/New Folder" and try to restore these settings. To see, what is called, you can break on "system" with gdb.
Created attachment 146890 [details] [review] Patch which quotes filename Again: haven't tested this patch due to lack of time
Minor nitpick, but the executable path should be constructed with g_build_filename() since on Windows it would be "%s\evolution-backup" (back slash instead of forward slash).
Created attachment 146989 [details] [review] Patch which builds the filename using g_build_filename Good catch. Here's a patch on top the other to build filename using g_build_filename.
Patch looks good. I'll try to remember to test and commit this over weekend, or if you can verify it sooner feel free to commit.
Hm. It accepts the file as it should do, so I think the patches are good enough to fix this particular issue. So I will commit to master. If there's another 2.28 release I think it'd be nice to have that backported. Especially distros with fresh releases using GNOME-2.28 (talking especially about Ubuntu and Mandriva) should pick that up, since their users are just about to migrate their setting. I did, however, encounter a different issue which I'll raise in bug 600861.
Comment on attachment 146890 [details] [review] Patch which quotes filename commit c46630b0d54a31c9b27ed728f2c39283f1eef786 Author: Tobias Mueller <tobiasmue@gnome.org> Date: Wed Nov 4 20:31:25 2009 +0000 Build filename using g_build_filename instead of hardcoding forward-slash
Comment on attachment 146989 [details] [review] Patch which builds the filename using g_build_filename commit 8cff3c4e4cf078307c600bb5ce69f50912abdd63 Author: Tobias Mueller <tobiasmue@gnome.org> Date: Wed Nov 4 00:09:27 2009 +0000 Quote filename during restore to prevent user assisted arbitrary code execut Fixes bug 540516.
*** Bug 604398 has been marked as a duplicate of this bug. ***