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 540516 - backup plugin doesn't escape filename
backup plugin doesn't escape filename
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Plugins
2.28.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-plugin-maintainers
Evolution QA team
: 555058 604398 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-06-27 16:58 UTC by Tobias Mueller
Modified: 2010-02-01 23:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch which sanitizes user input with g_shell_quote (1.51 KB, patch)
2008-06-27 16:59 UTC, Tobias Mueller
reviewed Details | Review
revised patch which doesn't leak memory (2.45 KB, patch)
2008-06-27 21:27 UTC, Tobias Mueller
committed Details | Review
Patch which quotes filename (1.13 KB, patch)
2009-11-04 00:57 UTC, Tobias Mueller
committed Details | Review
Patch which builds the filename using g_build_filename (1.21 KB, patch)
2009-11-05 09:56 UTC, Tobias Mueller
committed Details | Review

Description Tobias Mueller 2008-06-27 16:58:27 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.
Comment 1 Tobias Mueller 2008-06-27 16:59:41 UTC
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 :(
Comment 2 Srinivasa Ragavan 2008-06-27 17:26:44 UTC
Sounds fine to me. Matt do you have any comments?
Comment 3 Matthew Barnes 2008-06-27 20:12:21 UTC
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.
Comment 4 Tobias Mueller 2008-06-27 21:24:57 UTC
Yes. You're absolutely right. I optimized the patch.
Comment 5 Tobias Mueller 2008-06-27 21:27:05 UTC
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.
Comment 6 Srinivasa Ragavan 2008-06-30 03:17:31 UTC
It should work. I will test it in build sanity.
Commit to stable/trunk
Comment 7 Suman Manjunath 2008-06-30 03:49:07 UTC
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
Comment 8 Milan Crha 2009-10-29 16:43:52 UTC
*** Bug 555058 has been marked as a duplicate of this bug. ***
Comment 9 Pedro Villavicencio 2009-11-03 18:54:45 UTC
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."
Comment 10 Tobias Mueller 2009-11-03 23:19:25 UTC
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
Comment 11 Tobias Mueller 2009-11-04 00:56:11 UTC
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.
Comment 12 Tobias Mueller 2009-11-04 00:57:05 UTC
Created attachment 146890 [details] [review]
Patch which quotes filename

Again: haven't tested this patch due to lack of time
Comment 13 Matthew Barnes 2009-11-04 04:23:11 UTC
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).
Comment 14 Tobias Mueller 2009-11-05 09:56:41 UTC
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.
Comment 15 Matthew Barnes 2009-11-05 14:45:21 UTC
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.
Comment 16 Tobias Mueller 2009-11-05 18:52:04 UTC
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 17 Tobias Mueller 2009-11-05 19:12:27 UTC
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 18 Tobias Mueller 2009-11-05 19:12:43 UTC
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.
Comment 19 Matthew Barnes 2010-02-01 23:05:44 UTC
*** Bug 604398 has been marked as a duplicate of this bug. ***