Description of problem: xlock does not unlock anymore The pb can be traced back to the permission of /etc/shadow that reads: ll /etc/shadow ---------- 1 root root 1490 Sep 28 11:53 /etc/shadow This is fixed as soon as they are changed to: ls -trl /etc/shadow -r--r----- 1 root shadow 1490 Sep 28 11:53 /etc/shadow It seems to be a regression from our old mother somehow: https://qa.mandriva.com/show_bug.cgi?id=23481 Dunno what really happens in the setup package. Cheers, Chris. Reproducible: Steps to Reproduce:
(In reply to Chris Denice from comment #0) > Description of problem: > > xlock does not unlock anymore > > The pb can be traced back to the permission of /etc/shadow that reads: > > ll /etc/shadow > ---------- 1 root root 1490 Sep 28 11:53 /etc/shadow > > This is fixed as soon as they are changed to: > > ls -trl /etc/shadow > -r--r----- 1 root shadow 1490 Sep 28 11:53 /etc/shadow > > > It seems to be a regression from our old mother somehow: > https://qa.mandriva.com/show_bug.cgi?id=23481 > > Dunno what really happens in the setup package. > > Cheers, > Chris. > > Reproducible: > > Steps to Reproduce: I have exact same problem with mate-screensaver. Same with https://qa.mandriva.com/show_bug.cgi?id=48590 For me this is fixed as soon as they are changed to: ls -trl /etc/shadow -r--r--r-- 1 root root 1073 Eyl 16 20:35 /etc/shadow
CC: (none) => tarakbumba
CC: (none) => doktor5000
CC: (none) => mageia
And same problem with /etc/gshadow
The only thing I can guess is because setup installs shadow with the wrong permissions, something is going behind its back trying to fix it (maybe msec) and is doing so incorrectly. I don't know or really thing this will fix the issue, but it's probably at least a change that should be made in setup. Index: SOURCES/setup-2.7.20-install-shadow.diff =================================================================== --- SOURCES/setup-2.7.20-install-shadow.diff (revision 733040) +++ SOURCES/setup-2.7.20-install-shadow.diff (working copy) @@ -5,5 +5,5 @@ install: install -D -m 644 group $(DESTDIR)/etc/group install -D -m 644 passwd $(DESTDIR)/etc/passwd -+ install -D -m 644 shadow $(DESTDIR)/etc/ -+ install -D -m 644 gshadow $(DESTDIR)/etc/ ++ install -D -m 440 shadow $(DESTDIR)/etc/ ++ install -D -m 440 gshadow $(DESTDIR)/etc/ Index: SPECS/setup.spec =================================================================== --- SPECS/setup.spec (revision 733040) +++ SPECS/setup.spec (working copy) @@ -1,7 +1,7 @@ Summary: A set of system configuration and setup files Name: setup Version: 2.7.20 -Release: %mkrel 10 +Release: %mkrel 11 License: public domain Group: System/Base Url: http://www.mageia.org/ @@ -42,7 +42,6 @@ %install %makeinstall_std -cp -a passgrp/{shadow,gshadow} %{buildroot}/etc/ %files %doc NEWS
CC: (none) => pterjan, thierry.vignaud
I would change from 644 to 640 (instead of 440)
I have tested that xlock works with 640 (or 440) provided that the group is shadow, but would fail for the group "root" with 640 and 440. I can change the spec if everybody agrees with this?
*** Bug 14214 has been marked as a duplicate of this bug. ***
CC: (none) => nicolas.salguero
Priority: Normal => release_blocker
Changing the spec is probably not enough, something rewrites the file
chattr +i /etc/shadow would be a quick&dirty workaround :) But it's probably better to use something like inotifywatch or audit subsystem to check what's changing the file.
440 are the correct permissions, not 640. I believe the patch I posted in Comment 3 is correct. I don't know if it'll fix this issue. It doesn't look like msec messes with shadow files permissions.
CC: (none) => luigiwalser
Thanks, I have uploaded on the repos: setup-2.7.21-2.mga5 which is your patch David, plus I specified explicitly the attr(440, root, shadow) in the %files section. I tested and the installed files get the correct permissions. It remains to see if some other progs affect them afterwards. If someone see something obviously wrong, feel free to fix it. The package is under freeze-push constrain (Remi bumped to 21-1.mga5 for another reason) cheers, chris.
was this pushed ?
not yet, I guess "pushers" are a bit overloaded... :-/
*** Bug 14329 has been marked as a duplicate of this bug. ***
CC: (none) => linux
setup 2.7.21-3.mga5 Same error like in comment 1 with LXQt
*** Bug 14380 has been marked as a duplicate of this bug. ***
CC: (none) => loginov_alex
Hi there, Setup-2.7.21-3.mga5 should be fixed for a fresh installation only; or upgrade, but it will not fix previously wrong permissions. I think the permissions should really be: 440 root shadow So if problems persist with these permission we should look to each package individually now. The other relevant question is if some package different than setup are changing these permissions; or may be that the mcc GUI installer ? Post it here! thanks, cheers.
I suspect the problem is coming from systemd-sysusers command which is often triggered on package installs. I notice two lines: if (fchmod(fileno(passwd), 0644) < 0) { and if (fchmod(fileno(shadow), 0000) < 0) { that look to match the values seen in this bug report. It seems that the shadow files in particular are defined as being 0000 in Fedora's setup package: http://pkgs.fedoraproject.org/cgit/setup.git/tree/setup.spec#n67 Does anyone know why ours are not restricted down in the same way? It seems that xlockmore package has a setuid binary... is this really how we want to work? I'd suggest we try and restrict it down (less setuid stuff the better). Again, seems Fedora disables this: http://pkgs.fedoraproject.org/cgit/xlockmore.git/tree/xlockmore.spec#n47 I'm not sure how the xlock stuff works but I'd expect a conversation via PAM at the very least. We do ship a pam file: http://svnweb.mageia.org/packages/cauldron/xlockmore/current/SOURCES/xlock.pamd?view=markup but this is a little different to Fedora's: http://pkgs.fedoraproject.org/cgit/xlockmore.git/tree/xlockmore.spec#n61 While I'd be happy enough to change the permissions in systemd via a patch, I'd really need to be convinced it's the right approach - someone needs to explain to me the security implications and why it's OK to change them. As I don't have any problems unlocking things with my files the way they are (restricted perms), I would be of the general opinion that anything that does require more relaxed permissions is not doing it right :p Hopefully this gives folk enough info as to look further into it - especially to see if the same problems exist in the components mentioned (mate-screensaver and xlockmore/xscreensaver) in Fedora assuming the permissions remain locked down there.
If something is going behind our backs and screwing with the /etc/shadow permissions, it needs to stop, period. I don't know what kind of funny business Fedora is doing, but 0000 permissions doesn't make any sense and I don't know of any other system that does this. If systemd is expecting that, it needs to be fixed. As far as the setuid thing in xlockmore, if it's not needed, then yes it'd be nice to disable that.
Well, the change in Fedora was done in: http://pkgs.fedoraproject.org/cgit/setup.git/commit/?id=178a2ad9f228c55d2ec0f2eb1df66aba73a38824 Which refers to: https://bugzilla.redhat.com/show_bug.cgi?id=517577 Basically it's all about lowering capabilities when it's not needed (I see several similar bug reports). I'll ask upstream and see whether or not these permissions need to be configurable or whether it would make more sense generally to harden them at our side too (I don't generally like the idea of them being read/writable by non-root).
Looking a little more closely.... It seems that by being group-writeable by "shadow", utilities like chage can run without being setuid (although they still need to be setgid). This seems at least a little more secure than on fedora where tools like chage appear to be setuid. Anyway, regardless of musing on this, I would argue that it is incorrect to set the file permissions if the files already exist. The sysusers code is partly for initialising things after a factory reset, although for MGA6 I will start pushing for us to adopt it generally as a replacement for the %_pre_useradd stuff. I'd argue that setting it up properly in a factory reset setup it would make sense to chmod/chown as needed, but when the file already exists, it should be left well alone. For us, I would suggest we ship a tmpfiles snippet with our desired permissions such that the files are properly owned after any such factory reset (assuming of course that I patch out any chmod'ing when the file already exists and this is upstreamed)
Yeah I'm all for hardening and making things more secure. That capabilities stuff is interesting, basically separating out types of actions that only root can do into difference pieces that can be granted by themselves, so you don't need setuid stuff (polkit being another good way to avoid needing setuid stuff). I don't know how much infrastructure needs to be in place for all the capabilities stuff to work. Like with SELinux, Fedora has patches involving capabilities stuff in various places that we don't have, so that concerns me. If it does require lots of patches and things, I don't know that we can realistically support it (just as it is with SELinux), but like I said, it may not be as bad, I just don't know enough to say. As you know I'm very much in favor of things being such that people don't need to ever edit existing files in /etc and merge changes into later versions of those files, and the whole factory reset idea. It'll be exciting if those kinds of changes take hold. As far as shadow permissions, it certainly shouldn't be world-readable (and actually is on my mga4 system here...WTF) nor writable by other-than-root, unless the chage thing needs it g+w like you said. Solaris doesn't have any write permissions set on it, so I wonder how it works there.
I don't have it writeable on my mga4; it is: -r--r----- 1 root shadow so something is actually changing the permissions of that file and we have to find it... :(
(In reply to Chris Denice from comment #23) > I don't have it writeable on my mga4; it is: > -r--r----- 1 root shadow Yeah but does e.g. chage run as a regular user allow you to edit that? Probably not (and arguably it shouldn't and thus if it really needs root rights, why do we even bother doing set*id at all on this utility - it all smells like some voodoo the reasoning behind which has long since forgotten! This (as far as I can tell) is the mode that is desirable for /etc/shadow. > so something is actually changing the permissions of that file and we have > to find it... :( On cauldron, I've poseded above what that something is - namely the sysusers command which creates system users. It has an unconditional chmod of the /etc/[g]shadow files to 0000. Now we just have to decide how best to fix it and I've asked upstream what patches make sense here and will implement the most appropriate method or add a hacky patch if nothing is decided in time.
Ok, thanks for this. I don't know what is sysusers, I guess it is systemd-sysusers and I never used chage. But, whatever you think is convenient, this file should be at the very least readable for solving the multiple bugs 0000 is triggering. For the rest, I am definitely not competent and I really don't understand the meaning and purpose of having 0000 as permission from the very beginning... Although it makes sense, the most secure system is indeed the one that cannot be used :)
I'll have a closer look to what happens with xlockmore wrt /etc/shadow, but it is compiled with: --disable-setuid in any case! cheers.
Blocks: (none) => 14069
(In reply to Colin Guthrie from comment #24) > On cauldron, I've poseded above what that something is - namely the sysusers > command which creates system users. It has an unconditional chmod of the > /etc/[g]shadow files to 0000. > > Now we just have to decide how best to fix it and I've asked upstream what > patches make sense here and will implement the most appropriate method or > add a hacky patch if nothing is decided in time. Well, thats easy ... revert that damage. It should _never_ override the permissions specified by the setup rpm. And I dont care what systemd upstream thinks about this. Allowing systemd-whatever to override packages is a design flaw we cant allow. This need to be fixed and fast, we need to get beta1 isos out.
CC: (none) => tmb
(In reply to Thomas Backlund from comment #27) > This need to be fixed and fast, we need to get beta1 isos out. I've committed systemd 217 and a fix for this issue. I had already updated to 217 locally prior to writing the patch otherwise I'd have just pushed it in 216, but I didn't have much time this evening to fiddle around and test the older version. No upstream comments yet but I only posted the patches this afternoon, so not really expecting too much yet. Either way, the patch looks pretty sane to me and I can't spot any obvious races etc. (always paranoid about such things, especially with user/passwd stuff). Looking through systemd 217 changelog it looks fine to me and there are a lot of bugfixes we likely want going forward. If you prefer I can take some time tomorrow to revert this back to 216 and just add this patch (but there will likely be a whole bunch of other cherry picks I'll have to do prior to final release anyway, so IMO this is the better approach just to ship 217, but feel free to veto that!).
(In reply to Colin Guthrie from comment #28) > (In reply to Thomas Backlund from comment #27) > > This need to be fixed and fast, we need to get beta1 isos out. > > I've committed systemd 217 and a fix for this issue. I had already updated > to 217 locally prior to writing the patch otherwise I'd have just pushed it > in 216, but I didn't have much time this evening to fiddle around and test > the older version. > > No upstream comments yet but I only posted the patches this afternoon, so > not really expecting too much yet. Either way, the patch looks pretty sane > to me and I can't spot any obvious races etc. (always paranoid about such > things, especially with user/passwd stuff). > > Looking through systemd 217 changelog it looks fine to me and there are a > lot of bugfixes we likely want going forward. This will break Mageia: If the word "rescue" is specified on the kernel command line the system will now boot into rescue mode (aka rescue.target), which was previously available only by specifying "1" or "systemd.unit=rescue.target" on the kernel command line. This new kernel command line option nicely mirrors the already existing "emergency" kernel command line option. "rescue" has been a reserved work for ages... since mdk/v times... We use it atleast to load rescue mode instead of stage2
(In reply to Thomas Backlund from comment #29) > This will break Mageia: > > If the word "rescue" is specified on the kernel command line > the system will now boot into rescue mode (aka > rescue.target), which was previously available only by > specifying "1" or "systemd.unit=rescue.target" on the kernel > command line. This new kernel command line option nicely > mirrors the already existing "emergency" kernel command line > option. > > "rescue" has been a reserved work for ages... since mdk/v times... > We use it atleast to load rescue mode instead of stage2 Will that actually break anything? I did see that change but I didn't think it would break things as our rescue system already uses rescue.target since I converted it to systemd (I figured that was most sensible at the time based on it's name alone!). I do specifically link /usr/lib/systemd/system/default.target to rescue.target in the rescue system and I guess that this would now be unnecessary, but it should all still work fine. If you have doubts however, I can easily patch it out - we already have a patch that adds "failsafe" as another alias for this mode as it was a long-time Mageia keyword.
(In reply to Colin Guthrie from comment #30) > > Will that actually break anything? I did see that change but I didn't think > it would break things as our rescue system already uses rescue.target since > I converted it to systemd (I figured that was most sensible at the time > based on it's name alone!). I do specifically link > /usr/lib/systemd/system/default.target to rescue.target in the rescue system > and I guess that this would now be unnecessary, but it should all still work > fine. > Hm, I had totally forgotten about that :/ > If you have doubts however, I can easily patch it out - we already have a > patch that adds "failsafe" as another alias for this mode as it was a > long-time Mageia keyword. Well, I pushed systemd last night and rebuilt rescue and stage2 so lets see how it works out
Status: NEW => RESOLVEDResolution: (none) => FIXED
https://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg24357.html
CC: (none) => oe
May I suggest a fix for mga4?
(In reply to Oden Eriksson from comment #32) > https://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg24357. > html I know... this was the patch I wrote as a direct result of this bug! But thank you for the reference - it's good to include it here :) (In reply to Oden Eriksson from comment #33) > May I suggest a fix for mga4? Please feel free! As noted elsewhere, the patch you linked in comment #32 does not apply in Mageia 4 (it's for a component that didn't exist then, so it only applies to Cauldron). In addition that was to address the permissions being set to 0000, NOT about them being set to world readable, so pretty much the opposite problem :D Please make any comments about fixing MGA4 on bug #14516. I suspect the problem is just the setup packages default permissions and we need to ensure they are locked down in the spec (as is done now in cauldron's setup spec) and that a %post - or perhaps better would be a %postun triggered on the removal of an older version of setup so this is a one-time fixup - is in place to fix up the perms manually with chown and chmod (it would probably make sense to make this same change in the cauldron spec too in case people upgrade from vanilla MGA4 to MGA5). Thanks!