GNOME Bugzilla – Bug 305931
GDM hardcodes umask to 022
Last modified: 2010-06-04 21:04:52 UTC
Distribution/Version: Ubuntu Hoary Adding a different umask (such as "umask 002) to /etc/login.defs or /etc/profile or PAM is overridden by what appears to be code in GDM that resets umask to 022. Even a new program, libpam-umask, which attempts to simplify this, is rendered useless. Shouldn't GDM allow itself to defer to PAM instead for this matter? Only known way to set a user's umask is to add it to .gnomerc, which makes it complicated for multiple users working in multiple desktop environments.
Why not put umask in the gdm2 PreSession script? That should work.
Maybe it should work, but isn't the point of pam to simplify this process, regardless of what login program is used? Even that method is far more difficult than it should be. How is the average user supposed to ever figure out how to share folders with read-write access for multiple users? I'm not the average user and I've wasted hours on this. Sorry for the rant.
I think this a major security issue. I've just discovered that all my gnome applications were silently ignoring my /etc/login.defs / shell umask. How can i make gdm read my user configuration when opening a session without having to put "umask 077" in many files ?
I don't know much about login.defs. But i think that gdm starts my user session using my user shell (from /etc/passwd) and my user shell takes care of reading my rc files on startup. Is that right ? Why my umask is wrong then ?
Shouldn't status of this bug to be NEW instead of UNCONFIRMED ?
I just talked about this bug with Gary Winiger, who is the expert at Sun regarding PAM. He states that PAM should not be used for setting umask, and that using $HOME/.profile or the system-wide /etc/profile is the appropriate place for setting system-wide and user-specific umask settings. I just tested this and putting umask 027 in my $HOME/.profile does overwrite the default value. So, I don't think that the problem as reported by the original poster is a GDM bug. If the shell is not honoring /etc/profile or $HOME/.profile, that's probably a bug with their shell, or not using the right shell config files (or perhaps some other setting aside from GDM is overriding it on their system). Anyway, it is a bug that GDM hardcodes the default umask to 022. GDM should probably load the value from /etc/login.defs on Linux and /etc/defaults/login on Solaris. Or it might be nice if you could set the default umask value in the GDM configuration file. I would accept a patch that made GDM smarter and use the default system value rather than assuming 022.
Did he say *why* PAM shouldn't be used for setting the umask? It seems natual to configure a user's environment with a single mechanism--PAM--rather than relying on them all to parser the login(1)-specific /etc/login.defs file. For this purpose, pam_tmpdir, pam_umask, pam_env, pam_mail, pam_limits, etc. exist. I also do not understand how calling umask in /etc/profile or ~/.profile will set the umask for a user logging in to GDM, since GDM does not start a login shell but instead runs /etc/gdm/Xsession. Though this file is a shell script, running it will not cause ~/.profile to be sourced as the shell processing the file is neither a login shell nor an interactive shell. See the INVOCATION section of bash(1).
GDM's Xsession script sources /etc/profile, so that can be used for setting system-wide settings for all users.
Ok, my confusion is caused because Debian have their own /etc/gdm/Xsession script that doesn't process /etc/profile or ~/.profile. :(
This was reported downsream in Debian as <http://bugs.debian.org/368080>.
I don't know whether PAM is the right place to set the umask, but what I'm sure is that /etc/profile or .profile shouldn't be relied on. The simplest thing to do is probably to parse /etc/login.defs to look for the default umask and apply it instead of the hardcoded 022. Those advised by Sun experts wouldn't notice the difference because they read /etc/profile after, and those who do things right would be able to set their umask. Allowing PAM to change the umask is not as simple, as GDM does many things after PAM authentication, and sets the umask to ensure file permissions. This means the umask would have to be saved after PAM authentication and restored later.
If some distros want GDM to default to a more sensable UMASK based on system configuration files, or if people want to add the ability to specify GDM's deafult UMASK in the GDM configuration, then either/both would be reasonable (as long as the behavior is documented in the GDM documentation). I would accept a well written patch to make this work better, or make it easier to "just work" out of the box. That said, I don't think it is that bad for the umask and other environment settings to get set via a script that is sourced at session startup time. I suspect only a minority of users would really care about this sort of option since it is fairly easy to hack around (hacking the GDM session startup script to call umask is not such a big deal). Since the mechanisms for setting the system default UMASK is probably different for each distro, it may make more sense for each distro to hack GDM to meet their needs (e.g. parsing /etc/login.defs) rather than trying to imbed this sort of thing into GDM itself.
I don't understand why GDM has to set umask at all ? Can't it just leave it defaut ?
I don't think it is bad for GDM to set umask. The problem, I think, is that is is hardcoded. It would be better if this could be configured so that this feature of GDM setting umask can be enabled or disabled as desired. That said, I don't think this is a serious issue. This bug has been open for 2 years and it doesn't seem to be causing anybody enough problems to propose a patch.
I don't understand what the point about configuring yet another time umask. I don't think it is gdm job. system admin can set the default umask (distro specific file or pam) and users are free to change it with pam. I'll try to make a patch but as their are 27 umask calls in gdm2 code, i first need to understand where it is really needed.
Created attachment 82633 [details] [review] drop all unnecessary umask
I'm a little uncomfortable making this change since I'm not sure we understand exactly what the ramifications are. Also it doesn't sound like any real testing has been done with the patch explaining how this affects GDM's behavior. GDM creates many temporary files (e.g. /tmp/.gdm_socket, files in the user's $HOME directory, etc). Will such files be created with unsafe permissions after this change, if the default system UMASK isn't safe? In other words, I'm concerned that GDM currently depends on the umask being 022 when creating such files. It may also be necessary to add code to ensure that various files that GDM creates are given the correct permissions after removing the setting of umask. Would someone be willing to test this patch with the default system UMASK set to an unsafe level versus 022 and explain what the differences are for the various files that GDM creates? If there are differences, the patch may need to be updated to fix such issues. I think most of the files that GDM creates are mentioned in the GDM user docs, so testing with those would be a good start.
Well, i am running it. The umask calls which intent is to restrict mode are clearly coded : old = umask; action; umask(old). The other umask calls are just a bunch cryptic umask 022. I don't know enough about gdm to review these umask(022) but setting 3 times in a row umask(022) doesn't look right. If these 022 files are administrative files, they should not be group/world writable. But i don't think they should be readable too. I understand these umask(022) as 'restore the default umask' not as 'this file should be group/world readable'.
I'm glad to hear that GDM runs with your patch, but that does not mean that the change does not introduce any security issues. Also, you don't mention if you are testing with your default umask set to an unsafe level. I understand that the attached patch takes care of making sure that files that require a non 022 umask continue to have the right permissions. That is good. However, my concern is that GDM may be setting the umask to 022 because this was easier than making sure each file created has sane permissions. In other words, does the current code depend on setting the umask to 022 rather than making sure each file created has reasonable permissions? Before accepting a patch like this, I'd like to see some testing that shows the files that GDM creates and their permissions before/after the patch with the default system umask set to different levels. At the very least, I'd like to see some analysis explaining how the code is still secure after making this change. That said, if you wanted to add a configuration option that would make GDM use the default umask rather than hardcoding it to 022, and make the default "022", then people who wanted to experiment with a possibly less secure configuration could do so. Perhaps this might be a better way to get your change upstream without having to do all the testing now.
I don't want a configuration option : I have already configured umask. If you want gdm to use a given umask, you need to be able to start it with that umask. I don't want to have yet another software to configure for umask. PAM is also there and works great. Let gdm not insanely override umask 10 times. Now i understand what is gdm status : there are many paranoid/duplicate/useless umask(whatever) calls that nobody understands. So don't tell me that GDM is currently fine and does the right thing : the code clearly shows it's not. It works if you use umask 022, else it is buggy. Feel free to test my patch.
I apologize that I am being difficult, but please understand that GDM is a program with sensitive security issues, and I do not want to make changes that may have ramifications that are not fully understood - especially when they pertain to security. umask affects file permissions, obviously, and therefore making this change may affect security with some umask settings. I do agree that it is a bug that GDM is setting umask and it would be better if it just used the system umask. However, we need to make sure that GDM continues to set file permissions properly for the files it creates regardless of the default umask. Having said all that, I would accept a well-tested patch that makes GDM work with the default umask. I don't feel that this patch has really been fully tested. You mention that there are many paranoid/duplicate/useless umask calls that nobody understands. Please note that the code to set umask to 022 was added to GDM before I became maintainer, so I apologize that I cannot off-the-top-of-my-head tell you whether your patch adds any security issues without testing. Perhaps I will have time to do such testing in the future, but cannot make any guarantees about when I would have time to do that. Especially since this bug has been sitting idle for almost 2 years, so it doesn't seem that this bug is causing many users (aside from yourself) much grief. If you are unwilling to give this patch a comprehensive test (e.g. showing what differences - if any - exist for file permissions for files created by GDM before and after this patch with different umask settings), then perhaps someone else will do this testing at some point. My suggestion that we make this a configuration option would be to allow interested users (such as yourself) the ability to do such testing without having to hack the code directly with your patch. Once such testing is done, then we could remove the code and the configuration option. This would be one way to move forward with this issue without you needing to do the testing now. There are other people who work on GDM who read/review these bug reports, so I would appreciate any comments about this discussion. If I am off-base in my analysis, then I'm sure someone will respond. If the general consensus is that this patch is safe, then I will withdraw my concerns.
I understand. I think the umask(077) parts i changed have the same effects. But indeed, it's unclear if umask(022) means 'default umask' or 'should not be group/world writable, but may be group/world readable/browsable'. I am surely not a GDM expert. But it could help me if you could provide a list of files that GDM creates (globally and per-user). Thanks.
I did a bit of analysis of things to look at, though I'd recommend that you also look around the code and make sure I'm not missing anything. Also please verify that my analysis below seems reasonable to you. grepping through the code for open() calls that use O_WRONLY and O_RDWR, I see these: GDM_KEY_LOG_DIR/$DISPLAY.log (open specifies perms as 644) GDM_KEY_SERV_AUTHDIR/$DISPLAY.XnestAuth (open specifies perms as 600) GDM_KEY_SERV_AUTHDIR/.gdmfifo (open specifies no perms - probably needs fixing) $HOME/.xsession-errors (open specifies 644 - perhaps should be 600?) gdm_safe_fopen_w function call (open specifies 644 if the file doesn't exist) gdm_safe_fopen_ap function call (opens with O_RDWR with 644 perms if the file doesn't exist - looks okay) The logic in the above two "safe" functions to fdopen the file a second time after creating it might have a race condition if someone deleted the file between opens? Might be better to ensure reasonable perms more clearly here? Perhaps the fopen call in server.c should call gdm_safe_fopen_w since it uses the same args? Also the .XnestAuth one mentioned above could use this function if we fixed it to pass in the perms so it could use 600 instead of 644. Might make the code a bit cleaner. A number of places in the code open /dev/console or /dev/tty (daemon/gdm.c, daemon/getvt.c, and utils/gdmopen.c). Perhaps these should specify the perms? Probably not since I'm guessing we can assume the devices exist already and are not created by GDM? Does the gdm_open_dev_null function require attention? Note many places call it to read O_RDWR. Also gui/gdmXnestchooser.c opens /dev/null like this. Perhaps this also doesn't really need attention since /dev/null should exist already? utils/gdmopen.c opens vtname O_RDWR. Need to look at. daemon/gdm.c and daemon/verify-pam open the pipe O_RDWR. Need to look at. gdm_connection_open_fifo in gdm-net.c calls open RDWR on the fifo without perms, though the mkfifo call above specifies 0660, so this should be okay. Also the gdm_connection_open_unix function takes perms as an argument (0666 is used) so this looks okay. In vicious-extenions, the ve_config_save calls fopen with just "w", which probably needs to specify perms. This function is used for creating config files like $HOME/.dmrc. In daemon/auth.c and daemon/misc.c, we call fdopen with just "w", which probably needs to specify perms. These are used to create the auth file. All mkdir/g_mkdir calls seem to pass reasonable perms. Could you look a bit more closely at these and evaluate what further changes need to be made to ensure GDM is secure with these files - maybe clean things up a bit if you can? Thanks.
g_mkstemp opens a file always O_EXCL and as of glibc >= 2.0.7 with permissions 0600 (g_mkstemp calls mkstemp if available, else it opens with 0600), so we could just remove the umask call before creating the tempfile. The umask call _after_ creating the tempfile is now also unneccesary, because it was only there to restore to the umask used before the g_mkstemp call. A few lines above we already set umask to 0022, so this call is useless now. Let's remove it. I put an fchmod(0600) after creating the tempfile, just in case someone is running with an insane umask setting of e.g. 0777 to be absolutly sure to have no change in behaviour before/after this patch. But I'm not sure it is worth the effort to support such an insane umask.
Created attachment 84310 [details] [review] remove unneccessary call to umask
Created attachment 84311 [details] [review] [Patch 2/?] Remove unneccessary call to umask After the jump label 'try_user_add_again' we set the umask to 0077 so it is totally unneccesary to set the umask to something else just before jumping to the above label.
What about the issues I raised in comment #23. I'd appreciate your analysis on these and if we are comfortable that we address them, I'll accept the patches for 2.19. I think the .gdmfifo file needs to specify its umask. Perhaps the fopen call in server.c should call gdm_safe_fopen_w since it uses the same args? Also the .XnestAuth one mentioned above could use this function if we fixed it to pass in the perms so it could use 600 instead of 644. Might make the code a bit cleaner. I also had a few doubts about the "safe" open calls. utils/gdmopen.c opens vtname O_RDWR. Need to look at. daemon/gdm.c and daemon/verify-pam open the pipe O_RDWR. Need to look at. gdm_connection_open_fifo in gdm-net.c calls open RDWR on the fifo without perms, though the mkfifo call above specifies 0660, so this should be okay. Also the gdm_connection_open_unix function takes perms as an argument (0666 is used) so this looks okay. In vicious-extenions, the ve_config_save calls fopen with just "w", which probably needs to specify perms. This function is used for creating config files like $HOME/.dmrc. In daemon/auth.c and daemon/misc.c, we call fdopen with just "w", which probably needs to specify perms. These are used to create the auth file. --- I'd appreciate your analysis on these.
Sorry. I'm not that familiar with the code. I'd like to do further work on this, because this bug really annoys me. I took me over half a day to find out that gdm messes with my umask. After that, it was easy to find this bugzilla entry. I'll go through the other points you mentioned, but I just started to work on a new job and I am a little short on time now :-). But hopefully this will get better soon ...
Sounds great! I look forward to making this change in GDM 2.19. I hope you have time to wrap it up - I think we are getting close.
*ping* Any luck finishing up this patch? I'd like to get this into GDM 2.19 if possible.
I'm on it. But my time is very spare now. Hopefully it is getting better next week.
*ping* Any update?
Created attachment 86692 [details] [review] 0001-Remove-unnecessary-call-to-umask.patch
Created attachment 86693 [details] [review] 0002-Remove-unneccessary-umask-calls.patch
Created attachment 86695 [details] [review] 0003-gdm_safe_open_-gets-a-new-parameter
That's all what I got that far. Not much, but at least a tiny step in the right direction. I rebased the patches to cleanly apply to trunk. PS: gdm didn't compile. There was at least a #include <sys/types.h> in daemon/filecheck.h and in gui/gdmuser.h missing (uid_t wasn't known)
Created attachment 86697 [details] [review] 0003-gdm_safe_open_-gets-a-new-parameter Sorry, I had send the wrong version
I think your build problem with GDM 2.19 was correct via the patch in bug #432686. This patch has been also applied to SVN head, but you can use this patch separately against 2.19.0 if you like.
Thanks, I just reviewed these and committed your 002 and 003 patches. These both look good. I like that gdm_safe_fopen_w and gdm_safe_fopen_ap now takes a perm argument, this removes a bunch of needless umask's. I did not apply patch 1 because it creates a race condition between the file being created and the chmod call. A hacker could probably create a program to watch for the created auth file and do something nasty with it before the chmod call, allowing them access to the auth file, which would be bad. I think in this place it makes sense to grab the old umask and continue to set the umask to 077 before creating the file, then reset the umask to the old umask after the call.
Note, I'd prefer to not depend on g_mkstemp functionality since on some platforms it might not always behave consistantly. It's better to be extra safe here since it is the user authority file we are talking about. But I think it makes sense to just grab the current umask before creating the file and reset it back to that rather than hardcoding it to 022 after the call. Does this make sense?
Thanks for taking the time to report this bug. However, you are using a version that is too old and not supported anymore. GNOME developers are no longer working on that version, so unfortunately there will not be any bug fixes for the version that you use. By upgrading to a newer version of GNOME you could receive bug fixes and new functionality. You may need to upgrade your Linux distribution to obtain a newer version of GNOME. Please feel free to reopen this bug if the problem still occurs with a newer version of GNOME.