Bug 18446 - Session dbus-daemon should not change umask
Summary: Session dbus-daemon should not change umask
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.2.x
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-08 12:31 UTC by Matt McCutchen
Modified: 2008-11-10 05:56 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix (693 bytes, patch)
2008-11-08 12:37 UTC, Matt McCutchen
Details | Splinter Review
Oops, correct bug number in patch (693 bytes, patch)
2008-11-08 12:38 UTC, Matt McCutchen
Details | Splinter Review
Revised patch with keep_umask option (9.35 KB, patch)
2008-11-08 19:37 UTC, Matt McCutchen
Details | Splinter Review

Description Matt McCutchen 2008-11-08 12:31:55 UTC
dbus-daemon changes its umask to 0022.  gnome-settings-daemon and Firefox processes that I start with the /apps/gnome_settings_daemon/keybindings/www shortcut key are inheriting this umask, which makes Firefox save downloaded files with the wrong permissions.

I think dbus-daemon should not change its umask.
Comment 1 Matt McCutchen 2008-11-08 12:37:07 UTC
Created attachment 20155 [details] [review]
Fix
Comment 2 Matt McCutchen 2008-11-08 12:38:30 UTC
Created attachment 20156 [details] [review]
Oops, correct bug number in patch
Comment 3 Havoc Pennington 2008-11-08 16:31:16 UTC
If applying this, it's not necessary to leave an archeology comment saying what code used to be there, just remove the old code with a ChangeLog entry.

That said, this is a potentially dangerous change, and would be ill-advised without at minimum reviewing all places where dbus creates files, and evaluating how this could impact them. Generally daemons set umask to avoid surprises, because umask is well-known to cause surprises. Most daemons do this.

A safer change might be to record the old umask (returned from the umask call), and when we spawn child processes, set the old umask in the child. That way dbus-daemon itself is never affected by the system environment and always gets the permissions it expects.

Comment 4 Matt McCutchen 2008-11-08 17:52:03 UTC
(In reply to comment #3)
> That said, this is a potentially dangerous change, and would be ill-advised
> without at minimum reviewing all places where dbus creates files, and
> evaluating how this could impact them. Generally daemons set umask to avoid
> surprises, because umask is well-known to cause surprises. Most daemons do
> this.

I was just thinking about the per-user daemon and had forgotten that there's a system daemon too.  In my view, setting the umask makes perfect sense for the system daemon since it is in control of the security model and knows who it wants to be able to access its files.  On the other hand, I don't see why a per-user daemon should override the umask that I have established for all the other processes in my session.

I ran the following to find all the places dbus creates files:

git grep -A 1 -E '\<(O_CREAT\>|(mkdir|symlink|mknod|bind) *\()'

And it appears that there is no created file whose permissions would become looser as a result of not setting the umask.  I'm guessing that permissions becoming tighter is not a problem because the daemon is single-user.

> A safer change might be to record the old umask (returned from the umask call),
> and when we spawn child processes, set the old umask in the child. That way
> dbus-daemon itself is never affected by the system environment and always gets
> the permissions it expects.

That would also fix my problem, but please consider whether it would not make sense to drop the umask setting altogether for a per-user daemon.
Comment 5 Havoc Pennington 2008-11-08 18:14:18 UTC
Agreed, could make sense to drop it for the session daemon. To do this, we'd have to add an option to the config file like <keep_umask> and set it in session.conf.
Comment 6 Matt McCutchen 2008-11-08 18:16:42 UTC
I was about to hard-code dropping the umask for a session daemon but not a system daemon.  I will implement the config option instead.
Comment 7 Matt McCutchen 2008-11-08 19:37:44 UTC
Created attachment 20160 [details] [review]
Revised patch with keep_umask option

That was an awful lot of work to add one boolean!
Comment 8 Havoc Pennington 2008-11-09 11:55:14 UTC
Patch looks great to me. Can someone merge this? (I think I still have ssh key problems with freedesktop.org git)

Comment 9 Colin Walters 2008-11-10 05:56:13 UTC
Applied, thanks!

commit 0314e701c812565bd7bdac548cadfea5d310d66c
Author: Matt McCutchen <matt@mattmccutchen.net>
Date:   Mon Nov 10 08:55:27 2008 -0500

    Bug 18446: Keep umask for session bus
    Signed-off-by: Colin Walters <walters@verbum.org>



Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.