(In reply to Theodoros Kalamatianos from comment #0)
> On several fresh Mageia 4 installs that I have, /etc/shadow and /etc/gshadow
> are world-readable:
> 
> # ls -l /etc/*shadow
> -r--r----- 1 root shadow 1007 Nov  9 15:32 /etc/gshadow
> -r--r----- 1 root shadow 1343 Nov  9 15:32 /etc/shadow

These files are not world readable. They are only readable by the root user and members of the shadow group.

I presume this is not the starting point with a fresh install off MGA4?

My permissions are stricter than that on my MGA4 installs, but perhaps they are the result of several upgrades over the years - fresh install may be different. Certainly the permissions in the setup rpm package look wrong.

Can someone confirm the status on a fresh install? Keep in mind that although the files may have those permissions on install, once the installer creates the users, the various user modification commands may correct those permissions, so it's probably wise to check the perms in /mnt/etc/{,g}shadow during the install process but before creating users.

If we confim the bad permissions from that method, we can look to updating setup and issuing an update I think.

[also note the bug you referred to regarding permissions getting changed to 0000 (which is pretty much the opposite of "world readable"!) was cauldron only but I believe the permissions on those files in the shipped RPM were indeed defined explicitly]

This is a big security bug...

In Mageia 4 and 5 the rights are: 
-rw-r--r-- 1 root root 1038 nov.  12 22:50 /etc/shadow

But he right value is
-r--r----- 1 root shadow 1077 oct.   7  2013 /etc/shadow

They are two problems:
- the group is root instead of shadow
- the file is world readable.

It should be interesting to know why this happens. If it is a workaround, the good values can make an old problem appear.

CC: (none) => jarillon

Sorry, too little sleep, too little copy.

The output I pasted was from a system that I had manually fixed. That system, by the way, was upgraded to Cauldron using urpmi and still had the issue when I reset the permissions to something sane.

Here's what I get on every other Mageia 4 system that I have access to:

$ ll /etc/*shadow
-rw-r--r-- 1 root root 421 Feb 22  2014 /etc/gshadow
-rw-r--r-- 1 root root 654 Feb 25  2014 /etc/shadow

As for https://bugs.mageia.org/show_bug.cgi?id=14194, there is a comment (#3) by David Walser that I took to mean that the setup package would at some time install /etc/*shadow with 644 permission bits.

In case it matters, I just verified the presense of this issue on a minimal Mageia 4 KVM VM that I have lying around and that has not received any updates in about a month.

In that machine setup-2.7.20-9.mga4 contains the /etc/*shadow files in its file list, but with 644 permissions. This is fixed in setup-2.7.21-3.mga5 that I have on my Cauldron machine. Updating that Mageia 4 VM did *not* fix the issue - seems that the setup package remained at the same version.

What's missing is something that resets the permissions to sanity when they are already broken.


PS: "too little copy" should be "too little coffee" in my previous post - still valid it seems...

So we need to backport the fixes from the Cauldron package where we made it install shadow with 440 permissions and have a %post to fix it for existing files, probably doing chmod 440 /etc/shadow* to make sure to catch any backup copies that may be world-readable as well.

Hardware: x86_64 => All
CC: (none) => dirteat

I would suggest doing:

chmod o-rwx /etc/*shadow*

1. It does not let things become more permissive if e.g. the sysadmin had already chmod'ed /etc/shadow to 400.

2. It also catches /etc/gshadow, along with any backups of both files.

(In reply to David Walser from comment #6)
> So we need to backport the fixes from the Cauldron package where we made it
> install shadow with 440 permissions and have a %post to fix it for existing
> files, probably doing chmod 440 /etc/shadow* to make sure to catch any
> backup copies that may be world-readable as well.

I would maybe use a %postun with a specific check for older versions of setup such that this fixup is done only once.

Then if people want to change things on their systems for other reasons, we won't interfere on every upgrade. /etc is technically administrator territory, so we should be a little careful about changing things too aggressively (tho' totally agree that it's correct to do a fix in this case).

That said, if we do want to continually enforce the permissions on these files, we could simply create tmpfiles snippets that do this - ensuring the correct permissions and ownership, even if the user subsequently changes them, a simple reboot would lock them down again. Admins who want their own permissions can always override the tmpfile snippet in /etc.


(In reply to Theodoros Kalamatianos from comment #7)
> I would suggest doing:
> 
> chmod o-rwx /etc/*shadow*

I've seen then being owned by root.root, so we should also do a chown here in addition to the chmod. I would also recommend we set both u and g elements explicitly to be certain.

> 1. It does not let things become more permissive if e.g. the sysadmin had
> already chmod'ed /etc/shadow to 400.

Yeah, I do half agree.... perhaps we should do a u-wx,g-wx,o-rwx? That way we are only taking away, and never adding? This would still preseve 400 permissions but would also change e.g. 777 down to 440 which is nice.
 
> 2. It also catches /etc/gshadow, along with any backups of both files.

Perhaps over-zealously tho'. I'd prefer to use /etc/{,g}shadow{,-} and not leak it any further (although we have to test when one element does not exist in that case).

Consulting urpmf (urpmf /etc/[^/]*shadow[^/]*$) suggests these will be the only files affected but we should not presume the contents of /etc out of principle, and thus we should rely on wildcards like this.

A %postun will only prevent it from running when the update is initially installed, thus breaking the fix.

systemd-tmpfiles really needs to stop randomly changing permissions of files all over the filesystem.  Its purpose initially was supposed to be for creating things in for-sure tmpfs filesystems that don't already exist.  It should not mess with /etc unless it also is a tmpfs, which maybe it will be someday in the future, but isn't now.

Also, we already have msec for enforcing permission changes for security reasons, and it's not all that helpful if we have multiple different tools doing this sort of thing, it just makes things more difficult for sysadmins.  Unfortunately not everyone has msec (although most Mageia users probably do), so fixing this from the setup package is probably the only good reliable fix.

I do agree that we shouldn't go to crazy with the wildcards.  I agree with the {,g} at the beginning.  I think it should use a * at the end though, because even though we know about the "-" at the end, there could also be ~, .bak, and some other things we may not have anticipated.

(In reply to Colin Guthrie from comment #8)
> That said, if we do want to continually enforce the permissions on these
> files, we could simply create tmpfiles snippets that do this - ensuring the
> correct permissions and ownership, even if the user subsequently changes
> them, a simple reboot would lock them down again. Admins who want their own
> permissions can always override the tmpfile snippet in /etc.

Do these snippets specify exact permissions? Or can we also have tighten-only entries ala "og-w"?

> I've seen then being owned by root.root, so we should also do a chown here
> in addition to the chmod. I would also recommend we set both u and g
> elements explicitly to be certain.

You mean a chown to group shadow? What if an administrator has changed that manually to root for some reason? E.g. to prevent users from using `chage'?

> > 1. It does not let things become more permissive if e.g. the sysadmin had
> > already chmod'ed /etc/shadow to 400.
> 
> Yeah, I do half agree.... perhaps we should do a u-wx,g-wx,o-rwx? That way
> we are only taking away, and never adding? This would still preseve 400
> permissions but would also change e.g. 777 down to 440 which is nice.

I am not too sure about removing the write permission from root. In the traditional Unix permission model it makes no difference, so I find it counter-intuitive to see a missing write permission when I can clearly write to the file.  

Are there any programs or editors that do their own checks? And what happens on systems with an LSM configuration that removes the CAP_DAC_OVERRIDE capability from root (not that I have seen any...)?

> > 2. It also catches /etc/gshadow, along with any backups of both files.
> 
> Perhaps over-zealously tho'. I'd prefer to use /etc/{,g}shadow{,-} and not
> leak it any further (although we have to test when one element does not
> exist in that case).
> 
> Consulting urpmf (urpmf /etc/[^/]*shadow[^/]*$) suggests these will be the
> only files affected but we should not presume the contents of /etc out of
> principle, and thus we should rely on wildcards like this.

Agreed, although there could always be the odd editor-generated backup with a different suffix.

(In reply to Colin Guthrie from comment #8)
> (In reply to David Walser from comment #6)
> > So we need to backport the fixes from the Cauldron package where we made it
> > install shadow with 440 permissions and have a %post to fix it for existing
> > files, probably doing chmod 440 /etc/shadow* to make sure to catch any
> > backup copies that may be world-readable as well.
> 
> I would maybe use a %postun with a specific check for older versions of
> setup such that this fixup is done only once.
> 

IMHO, %postun won't work here; it should be either a %posttrans, or a %triggerpostun if we want to do only once.

But we should be careful, IIRC, setup is in the first transaction at install, so we must be careful to not introduce a circular dependancy (eg using a sh script in %post).
A safe way would be to use lua.

add Thierry in CC to have his opinion.

CC: (none) => lmenut, thierry.vignaud

(In reply to David Walser from comment #9)
> A %postun will only prevent it from running when the update is initially
> installed, thus breaking the fix.

Oops, sorry I mistyped - I meant a %triggerun, not %postun.

Something like:

%triggerun -- %{name} < 2.7.20-9.1

We could instead use a value of 2.7.21-4 so that all cauldron and beta1 upgraders are also caught but this does mean that the fix could be applied twice for some users - once when updating MGA4 and a second time when updating their updated MGA4 to MGA5.


> systemd-tmpfiles really needs to stop randomly changing permissions of files
> all over the filesystem.  Its purpose initially was supposed to be for
> creating things in for-sure tmpfs filesystems that don't already exist.  It
> should not mess with /etc unless it also is a tmpfs, which maybe it will be
> someday in the future, but isn't now.

Generally speaking there are not many rules where tmpfiles do "mess with permissions" and when they do it's typically only in the tmpfs /run (or /var/run which is the same thing)

/etc on tmpfs will likely only happen for containers.

But anyway, doing "grep -E "[0-7]{4}" /usr/lib/tmpfiles.d/*.conf" doesn't throw up much that changes perms "all over the files system". The only one that even really comes close to that definition is var.conf (which is indeed related to the factory reset stuff, and if it gets things different to our packaging for /var then we should change either the packaging or the tmpfile snippet to match).

Indeed checking on my system:

grep -E "^[^#].*[0-7]{4}" /usr/lib/tmpfiles.d/*.conf | cut -d':' -f2| cut -d/ -f2 | cut -d' ' -f1 | sort -u

suggests only /dev, /run, /tmp and /var are touched.

So I'm not really sure of the basis for your above comment and I can't say I really agree with it completely! Keep in mind that it was sysusers that had previously set shadow file perms to 0000 (and has now been fixed upstream), not tmpfiles just in case the two have been conflated.

> Also, we already have msec for enforcing permission changes for security
> reasons, and it's not all that helpful if we have multiple different tools
> doing this sort of thing, it just makes things more difficult for sysadmins.
> Unfortunately not everyone has msec (although most Mageia users probably
> do), so fixing this from the setup package is probably the only good
> reliable fix.

I'm happy with that. Was just providing alternatives.

FWIW, we will eventually have to add tmpfile snippets for our permissions on these files if we ever want to support factory reset of /etc as otherwise they will be created with the compiled in defaults in systemd-sysusers, hence why I thought I'd mention it. Might make sense to do it now rather than have to solve this problem again in the future.

> I do agree that we shouldn't go to crazy with the wildcards.  I agree with
> the {,g} at the beginning.  I think it should use a * at the end though,
> because even though we know about the "-" at the end, there could also be ~,
> .bak, and some other things we may not have anticipated.

Yeah fair enough :)




(In reply to Theodoros Kalamatianos from comment #10)
> (In reply to Colin Guthrie from comment #8)
> > That said, if we do want to continually enforce the permissions on these
> > files, we could simply create tmpfiles snippets that do this - ensuring the
> > correct permissions and ownership, even if the user subsequently changes
> > them, a simple reboot would lock them down again. Admins who want their own
> > permissions can always override the tmpfile snippet in /etc.
> 
> Do these snippets specify exact permissions? Or can we also have
> tighten-only entries ala "og-w"?

See "man tmpfiles.d" and search for "Mode". You can set them as a mask against existing files by using a ~ prefix, but I'd suggest we want to be explicit here. If admins (or other tools) want to lock this down, they can drop an overriding tmpfile snippet in the /etc/ tree to override the vendor choice.

> > I've seen then being owned by root.root, so we should also do a chown here
> > in addition to the chmod. I would also recommend we set both u and g
> > elements explicitly to be certain.
> 
> You mean a chown to group shadow? What if an administrator has changed that
> manually to root for some reason? E.g. to prevent users from using `chage'?

Yeah, but by the same token it's also not correct as to what we intended. So I suspect we have to go with what we think is mostly likely. I think it's more likely that people did a fresh install of MGA4 and got themselves accidentally into this position than someone specifically did this deliberately.

This is one of the reasons I suggested our fixup be in a %triggerun rather than a %post - so that it's a one-time fix, not an "on every update" fix.

But I do see your point too, so won't argue the case too hard :)

> > > 1. It does not let things become more permissive if e.g. the sysadmin had
> > > already chmod'ed /etc/shadow to 400.
> > 
> > Yeah, I do half agree.... perhaps we should do a u-wx,g-wx,o-rwx? That way
> > we are only taking away, and never adding? This would still preseve 400
> > permissions but would also change e.g. 777 down to 440 which is nice.
> 
> I am not too sure about removing the write permission from root. In the
> traditional Unix permission model it makes no difference, so I find it
> counter-intuitive to see a missing write permission when I can clearly write
> to the file.  
> 
> Are there any programs or editors that do their own checks? And what happens
> on systems with an LSM configuration that removes the CAP_DAC_OVERRIDE
> capability from root (not that I have seen any...)?

Well, this is another debate really. The permissions in Cauldron on both files in the package are 440. If we want to change it to 660 then we can do so, but I thought that had already been discussed. I would have thought that removing CAP_DAC_OVERRIDE would be someone someone did to specifically lock things down, and I guess you could argue that changing passwords is one of those things that should apply (I'm sure there are plenty arguments to the contrary tho'!)

> Agreed, although there could always be the odd editor-generated backup with
> a different suffix.

Yeah, David said the same. On balance, I'd be happy enough with a wildcard ending, but I'd defer to David's opinion here anyway.

(In reply to Luc Menut from comment #11)
> IMHO, %postun won't work here; it should be either a %posttrans, or a
> %triggerpostun if we want to do only once.

Yeah I meant a %triggerun in my head but wrote it down wrong :s
 
> But we should be careful, IIRC, setup is in the first transaction at
> install, so we must be careful to not introduce a circular dependancy (eg
> using a sh script in %post).
> A safe way would be to use lua.
> 
> add Thierry in CC to have his opinion.

I think %triggerun would avoid any problems here as it won't apply on first install.

(In reply to Colin Guthrie from comment #13)
> 
> I think %triggerun would avoid any problems here as it won't apply on first
> install.

yep, but rpm requires are commons to install and upgrade, so we have to check that the added script doesn't add a requires that introduce a circular dependency.

Notice also that the group is wrong on mga4, it should be shadow, not root!

You're scaring me with scriplets changing permissions of such a crucial file; I hope we will not regret it later :)

I can't think of a good reason why any system administrator would choose to have both /etc/shadow and /etc/gshadow world-readable.

For me, the important issue to be fixed here is to remove the read access for "other".

To my way of thinking, msec should be checking and fixing this in it's regular checks.

I tried chmod o-r /etc/shadow /etc/gshadow on two of my mga 4.1 systems.

The next thing I found was that I could not login through screen lock.
I am using mate DE and lxdm (to allow switching DE).

As soon as I restored /etc/shadow /etc/gshadow to world readable
then I could authenticate in the screen lock.

So, there seems to be some dependency on having these files at o+r.

@last comment:
/etc/shadow must have group id shadow:
chgrp shadow /etc/{,g}shadow
chmod 440 /etc/{,g}shadow

Then unlocking should work without world-readable shadow files.

CC: (none) => cjw

This works:

files="/etc/shadow /etc/gshadow"; chgrp shadow ${files} && chmod o-r ${files}

Ping...

CC: (none) => mageia

Could this not be done with task-meta?

Nope, setup package needs to fix its own mess.... I will fix it up

Assignee: bugsquad => tmb

(In reply to Thomas Backlund from comment #22)
> Nope, setup package needs to fix its own mess

Sure, and we did fix something that was incorrect in the setup package in Cauldron, but it was also wrong well before that, but the issue didn't crop up until Mageia 4.  It's still not clear what it was that used to fix the permissions of /etc/shadow and no longer does.  Maybe it's not important, as the issue could just be fixed in setup, but it'd be nice to know.

It broke with the removal of old tcb conversion code as that always ensured permissions and ownership:

http://svnweb.mageia.org/packages/updates/4/setup/current/SPECS/setup.spec?r1=391543&r2=407081

(In reply to Thomas Backlund from comment #22)
> Nope, setup package needs to fix its own mess.... I will fix it up

Taking too long (: - please review my changes. Feel free to submit to mga4 updates/testing.

(In reply to Christiaan Welvaart from comment #25)
> (In reply to Thomas Backlund from comment #22)
> > Nope, setup package needs to fix its own mess.... I will fix it up
> 
> Taking too long (: - please review my changes. Feel free to submit to mga4
> updates/testing.

Looks insufficient.  There's plenty of commentary in previous comments in this bug about considerations that should go into the fix.

(In reply to David Walser from comment #26)
> (In reply to Christiaan Welvaart from comment #25)
> > (In reply to Thomas Backlund from comment #22)
> > > Nope, setup package needs to fix its own mess.... I will fix it up
> > 
> > Taking too long (: - please review my changes. Feel free to submit to mga4
> > updates/testing.
> 
> Looks insufficient.  There's plenty of commentary in previous comments in
> this bug about considerations that should go into the fix.

From earlier comments I gather the following improvements could be made:
- use lua instead of a shell script
- use a versioned triggerun script instead of %post
- handle the files with a - suffix as well

The spec file is updated in svn.

Ideally it'd also catch backup files, but this is probably sufficient, assuming the triggerun thing works as expected.  We should get this out before mga5 is released if possible, since the fix doesn't exist there.

CC: (none) => luigiwalser

We need to add the same fix in mga5 to cover all upgraders

(In reply to Thomas Backlund from comment #29)
> We need to add the same fix in mga5 to cover all upgraders

That's a good idea.  Would the triggerun part be the same?

yep

hum, same triggerun but with different version, adjusted to mga5 version.

(In reply to Luc Menut from comment #32)
> hum, same triggerun but with different version, adjusted to mga5 version.

No, the version specified should be the current MGA4 version (prior to update). Remember that it's triggering on "uninstall" of a package, therefore whether you got from $mga4 to $mga4.1 or from $mga4 to $mga5, you still just match on $mga4.

(In reply to Colin Guthrie from comment #33)
> (In reply to Luc Menut from comment #32)
> > hum, same triggerun but with different version, adjusted to mga5 version.
> 
> No, the version specified should be the current MGA4 version (prior to
> update). Remember that it's triggering on "uninstall" of a package,
> therefore whether you got from $mga4 to $mga4.1 or from $mga4 to $mga5, you
> still just match on $mga4.

I don't think so.
This bug was fixed for new installs only with setup-2.7.21-3.mga5 (Oct 20 2014),
http://svnweb.mageia.org/packages?view=revision&revision=737458
so if we want to fix :
- mga5 new installs until 2014-10-20 (mga5 alpha1 & alpha2),
- all old installs updated to cauldron,
in all these cases, the uninstalled version will be a mga5 version (2.7.21-3, if fully updated). We need to have last setup version from cauldron in the triggerun version in order to fix them.

s/all old installs updated to cauldron/all old installs already updated to cauldron/

From the dev list, it looks like this isn't yet fixed.

My system was a fresh install of mga4.
All /etc/*shadow* files were owned by root/root
This would exclude shadow.lock, which had permissions rw-------
The others (gshadow, gshadow-, shadow, shadow-) had permissions rw-r--r--
(Much as suggested by comments above.)

So based on the above comments, it looks like the solution is :

chmod a-wx,o-r {,g}shadow{,?}
chown :shadow {,g}shadow{,?}

in a %triggerun for versions <=2.7.21-3

If this looks right, could someone implement it ?

(The x is probably overkill, the ? probably could be -, and maybe a warning could be made for advanced users that changed permissions in an incompatible way for some reason.)

CC: (none) => andre999mga

The files need to be write by root so modified chmod below

I think to add to spec file

%post
chmod g-wx,o-rwx {,g}shadow{,?}
chown :shadow {,g}shadow{,?}


is t rught ?

CC: (none) => contact

No, it does not need to be root writable, and the chmod should set the exact correct permissions.  What cjw added in Mageia 4 SVN should be good and just needs to be copied to Cauldron SVN.

(In reply to Daniel Tartavel from comment #37)
> The files need to be write by root so modified chmod below
> 
> I think to add to spec file
> 
> %post
> chmod g-wx,o-rwx {,g}shadow{,?}
> chown :shadow {,g}shadow{,?}
> 
> 
> is t right ?

You have to use %triggerun and not %post since a user might skip an upgrade or 2, thus the code has to be left in the spec file for a while, and you don't want to redo this correction for users who (appropriately) update every time.
%triggerun is only run when the previous version (less than the specified level) is uninstalled, thus only done once.

Update candidate pushed to Mageia 4:
setup-2.7.20-9.1.mga4.noarch

Advisory and instructions to come.

Assignee: tmb => qa-bugs

Suggested advisory:
===================

Updated setup package fixes security vulnerability

  A security vulnerability has been identified in Mageia 4's setup package
  where the /etc/shadow and /etc/gshadow files containing encrypted passwords
  had wrong ownership and permissions, making them world-readable (mga#14516).

  This update fixes this issue by enforcing that those files are owned by root
  and the shadow group, and are only readable by those two entities (0440
  permissions).


References:
 - https://bugs.mageia.org/show_bug.cgi?id=14516


SRPM:
 - setup-2.7.20-9.1.mga4


RPMs:
setup-2.7.20-9.1.mga4.noarch

Let's try revising it a little.

Suggested advisory:
===================

Updated setup package fixes security issue

  An issue has been identified in Mageia 4's setup package where the
  /etc/shadow and /etc/gshadow files containing password hashes were created
  with incorrect permissions, making them world-readable (mga#14516).

  This update fixes this issue by enforcing that those files are owned by
  the root user and shadow group, and are only readable by those two entities.

  Note that this issue only affected new Mageia 4 installations.  Systems that
  were updated from previous Mageia versions were not affected.


References:
 - https://bugs.mageia.org/show_bug.cgi?id=14516


SRPM:
 - setup-2.7.20-9.1.mga4


RPMs:
setup-2.7.20-9.1.mga4.noarch

Testing procedure:
==================

Before the update, check the permissions of your /etc/shadow, /etc/gshadow and potentially /etc/shadow- and /etc/gshadow- files, e.g. with:

# ls -l /etc/*shadow*
-rw-r--r-- 1 root shadow 876 mars  20 10:26 /etc/gshadow
-rw-r--r-- 1 root shadow 864 mars  20 10:26 /etc/gshadow-
-rw-r--r-- 1 root shadow 918 mars  20 10:26 /etc/shadow
-rw-r--r-- 1 root shadow 896 mars  20 10:26 /etc/shadow-
-rw------- 1 root root     0 mars  10 15:33 /etc/shadow.lock

After the update, check the permissions again:

$ ls -l /etc/*shadow*
-r--r----- 1 root shadow 876 mars  20 10:26 /etc/gshadow
-r--r----- 1 root shadow 864 mars  20 10:26 /etc/gshadow-
-r--r----- 1 root shadow 918 mars  20 10:26 /etc/shadow
-r--r----- 1 root shadow 896 mars  20 10:26 /etc/shadow-
-rw------- 1 root root     0 mars  10 15:33 /etc/shadow.lock

(Don't mind the dates being the same as above, my "before" example is made up since I already have a fixed system).

before they are also probably root:root

Indeed it was root:root 644 before.  I had fixed /etc/shadow manually already on the one machine I have that's affected, but I hadn't bothered with /etc/gshadow yet.

Confirmed that everything is correct after installing the update on Mageia 4 i586.

Whiteboard: has_procedure => has_procedure MGA4-32-OK

Testing on Mageia4x64 real hardware,

From current package :
--------------------
setup-2.7.20-9.mga4

# ls -l /etc/*shadow*
-rw-r--r-- 1 root root 589 mars  21 11:19 /etc/gshadow
-rw------- 1 root root 576 mars  21 10:42 /etc/gshadow-
-rw-r--r-- 1 root root 877 mars  21 11:19 /etc/shadow
-rw------- 1 root root 854 mars  21 10:42 /etc/shadow-
-rw------- 1 root root   0 juin   6  2014 /etc/shadow.lock

To updated testing package :
--------------------------
setup-2.7.20-9.1.mga4

-r--r----- 1 root shadow 589 mars  21 11:19 /etc/gshadow
-r--r----- 1 root shadow 576 mars  21 10:42 /etc/gshadow-
-r--r----- 1 root shadow 877 mars  21 11:19 /etc/shadow
-r--r----- 1 root shadow 854 mars  21 10:42 /etc/shadow-
-rw------- 1 root root     0 juin   6  2014 /etc/shadow.lock
-r--r----- 1 root shadow 317 mars  20 19:00 /etc/shadow.rpmnew

OK

CC: (none) => olchal
Whiteboard: has_procedure MGA4-32-OK => has_procedure MGA4-32-OK MGA4-64-OK

Confirmed the fix mga4 64

Before
------
$ ls -l /etc/*shadow*
-rw-r--r-- 1 root root 1053 Feb 21 13:07 /etc/gshadow
-rw-r--r-- 1 root root 1043 Feb  4 14:38 /etc/gshadow-
-rw-r--r-- 1 root root 1493 Feb 21 13:07 /etc/shadow
-rw-r--r-- 1 root root 1473 Feb  4 14:38 /etc/shadow-
-rw------- 1 root root    0 Feb 26  2014 /etc/shadow.lock

After
-----
$ ls -l /etc/*shadow*
-r--r----- 1 root shadow 1053 Feb 21 13:07 /etc/gshadow
-r--r----- 1 root shadow 1043 Feb  4 14:38 /etc/gshadow-
-r--r----- 1 root shadow 1493 Feb 21 13:07 /etc/shadow
-r--r----- 1 root shadow 1473 Feb  4 14:38 /etc/shadow-
-rw------- 1 root root      0 Feb 26  2014 /etc/shadow.lock
-r--r----- 1 root shadow  317 Mar 20 18:00 /etc/shadow.rpmnew

Noted that /etc/shadow.rpmnew was created, leaving /etc/shadow intact. Checked content of all affected files for possible overwrite and all appears ok.

We should test the fix in general use also.

It would also be nice to test at least once those two Mageia 5 upgrade cases:
- upgrade from a non-updated Mageia 4 with setup-2.7.20-9.mga4 (the Mageia 5 setup package should then handle the fix)
- upgrade from an up-to-date Mageia 4 with setup-2.7.20-9.1.mga4 (the Mageia 5 setup package should then create the .rpmnew file without touching the old files permissions).

Hm, I'm a bit concerned about this update actually. It works fine, but I fear that unaware users might break their systems if they update via MageiaUpdate, and click "Use the .rpmnew" when prompted with what they want to do of the new /etc/fstab and new /etc/shadow.

There's already one user who made the mistake on Cauldron it seems: http://www.mageialinux-online.org/forum/topic-19953+attention-a-la-derniere-mise-a-jour.php (in French)

We should maybe fix bug 14266 before we push this update.

(In reply to Rémi Verschelde from comment #49)
> Hm, I'm a bit concerned about this update actually. It works fine, but I
> fear that unaware users might break their systems if they update via
> MageiaUpdate, and click "Use the .rpmnew" when prompted with what they want
> to do of the new /etc/fstab and new /etc/shadow.
> 
> There's already one user who made the mistake on Cauldron it seems:
> http://www.mageialinux-online.org/forum/topic-19953+attention-a-la-derniere-
> mise-a-jour.php (in French)

And one more (if it isn't the same user with a different user name):
 bug 15548

(In reply to Marja van Waes from comment #51)
> And one more (if it isn't the same user with a different user name):
>  bug 15548

Haha, no, I don't use aliases. He is someone else. :)

How much longer do we want to hold up this update?  I do agree that the MageiaUpdate issue really needs to be fixed, but this has been simmering for quite a while...

IMO we can't push it until bug 14266 is fixed. There has already been quite an uproar following the cauldron freeze push, where unaware users chose to use the .rpmnew (most likely because it has "new" in the name, and people tend to think "new = better").

So I don't think we want to take the risk to break even only 2% of existing Mageia 4 installation because of this MageiaUpdate bug.

I agree with Rémi. 

If we push as-is and give users a choice between, with one click of a button, keeping a working system and inadvertently breaking it then the likelihood is that many will break it.

Maybe I'm missing something, but why not ensure that no rpmnew is generated for fstab, at least for updates to setup ?
(That is, no fstab generated in case of an update.)
It should never be useful in the case of an update.

Is there some reason why that wouldn't be possible (or wanted) ?

shadow and fstab are config(noreplace) files, and rpm generates .rpmnew files for all such files when they are modified from what the package installed.  AFAIK rpm doesn't have something that means config(noreplace,don't-create-rpmnew-files).  The only other alternative is just config, which means on package update the existing file will be renamed as a .rpmsave and the packaged file will replace it.  Obviously that wouldn't work here.  Ideally, these files wouldn't be packaged in the first place; it really makes no sense for them to be owned by a package.  Their initial contents should probably be generated by a %pretrans script or something in new installs, and after that those files should be left alone by the package manager.

Also, config(noreplace) files in general need to go away, but that's a whole different discussion.  The systemd developers are working on that, but it's going to take time.  Package defaults and local system customizations should be stored in different files, perhaps in different directories.  Upstream developers of all software will have to get on board with that idea though.

Thanks for the explanation.
Hopefully that evolution comes fairly soon.
Maybe we should be using a special package to fix the permissions/ownership ?
One that doesn't include shadow and fstab ?

Thanks to Martin Whitaker, bug 14266 should soon be fixed, so afterwards we'll be able to push this one as an update.

Removing the OKs, I'd like this one to be tested against the new rpmdrake (so please try the update with MageiaUpdate).

Whiteboard: has_procedure MGA4-32-OK MGA4-64-OK feedback => has_procedure

One thing to check is to confirm priority upgrade works...

with both rpmdrake and setup in updates_testing, set updates_testing as updates media, and confirm it will automatically install rpmdrake before installing the setup rpm (like it will/should happend for normal users when available in updates)

(In reply to Thomas Backlund from comment #61)
> One thing to check is to confirm priority upgrade works...
> 
Indeed, I've confirmed it works on cauldron, but it's worth checking on mga4 too.

This package has already been tested and hasn't been rebuilt or anything, so the validation is still valid.  The bot that pushes updates still will not push this update yet because it is dependent on another bug that's not closed.  So, no need to worry about this being pushed before rpmdrake because it won't be.  Also, it won't be pushed until the advisory is uploaded and an ID is assigned.

Testing how the updated rpmdrake handles being updated with the setup package is already a required part of the testing procedure for the other bug, so that's the one that still needs to be tested and validated.

CC: (none) => sysadmin-bugs
Keywords: (none) => validated_update
Whiteboard: has_procedure => has_procedure MGA4-32-OK MGA4-64-OK

An update for this issue has been pushed to Mageia Updates repository.

http://advisories.mageia.org/MGASA-2015-0116.html

Status: NEW => RESOLVED
Resolution: (none) => FIXED

Update on a Mageia 4 is perfectly correct.
Tested on 2 desktops and a laptop. Congratulations.

Reopening as the setup update was nuked since it was not 100% foolproof. A new update will be issued that should normally _never_ propose to use empty .rpmnew files.

Status: RESOLVED => REOPENED
Resolution: FIXED => (none)

Removing validated tag, advisory will also need to be updated.

Keywords: validated_update => (none)
Whiteboard: has_procedure MGA4-32-OK MGA4-64-OK advisory => has_procedure

This one has been pushed and advisory ID assigned and MGAA which makes it difficult to re-use.

It's probably best to work on the better fix and then create a new bug for the update, linked to this one so that both are closed when the update is then pushed.

Ok, I'll open another BR then.

Keywords: (none) => validated_update
Blocks: 14069 => (none)
Whiteboard: has_procedure => has_procedure MGA4-32-OK MGA4-64-OK advisory

We shouldn't be creating new bugs because of a broken update process.  We need to be able to support reopening a bug when an issue isn't actually fixed, especially in a case like this where the update was simply deleted.

We had a case recently where we were able to reuse the same bug, it just took a little manual work on the sysadmin side.  Ideally, that should be fixed on the sysadmin side.

*** Bug 15644 has been marked as a duplicate of this bug. ***

Advisory:
=========

Updated setup package fixes security issue

  An issue has been identified in Mageia 4's setup package where the
  /etc/shadow and /etc/gshadow files containing password hashes were created
  with incorrect permissions, making them world-readable (mga#14516).

  This update fixes this issue by enforcing that those files are owned by
  the root user and shadow group, and are only readable by those two entities.

  Note that this issue only affected new Mageia 4 installations. Systems that
  were updated from previous Mageia versions were not affected.

  This update was already issued as MGASA-2015-0116, but the latter was withdrawn
  as it generated .rpmnew files for critical configuration files, and rpmdrake
  might propose the user to use those basically empty files, thus leading to
  loss of passwords or partition table. This new update ensures that such .rpmnew
  files are not kept after the update.


References:
 - https://bugs.mageia.org/show_bug.cgi?id=14516
 - http://advisories.mageia.org/MGASA-2015-0116.html
 - https://ml.mageia.org/l/arc/qa-discuss/2015-03/msg00399.html


SRPM:
 - setup-2.7.20-9.2.mga4


RPMs:
setup-2.7.20-9.2.mga4.noarch

Testing procedure in comment 43. Please test updates with the following initial configurations:
- updated setup (2.7.20-9.1.mga4, in case you updated before it was nuked) with updated rpmdrake
- non-updated setup (urpmi --downgrade --searchmedia "Core Release" setup) with updated rpmdrake (6.10.5-1.mga4)
- non-updated setup with non-updated rpmdrake (6.10-1.mga4)

Actually I need to change the %post script to use lua instead of shell.

Whiteboard: has_procedure => has_procedure feedback

One of you had better upload the advisory then as you're ignoring my request

(In reply to Rémi Verschelde from comment #74)
> Actually I need to change the %post script to use lua instead of shell.

Done with:


SRPM:
 - setup-2.7.20-9.3.mga4

RPMs:
setup-2.7.20-9.3.mga4.noarch

Whiteboard: has_procedure feedback => has_procedure

And... make that -9.4.mga4 :-) This time I should be really done :-)

SRPM:
 - setup-2.7.20-9.4.mga4

RPMs:
setup-2.7.20-9.4.mga4.noarch

Testing complete x86_64. I downgraded rpmdrake, then activated the updates testing media as update media, and updated rpmdrake itself then setup via MageiaUpdate. No rpmnew was ever proposed to me, and the rights of the shadow files are ok (readable only to root and group shadow).

I'm going to test again, this time downgrading both rpmdrake and setup (since I already had setup from Core Updates before it was nuked). Consider it ok if I don't comment within an hour, I'll report if anything bad happens after reboot.

CC: (none) => stormi
Whiteboard: has_procedure => has_procedure MGA4-64-OK

Bug 15644 will be used for the new update as per QA meeting.

Closing this one again.

Keywords: (none) => validated_update
Status: REOPENED => RESOLVED
Resolution: (none) => FIXED
Whiteboard: has_procedure MGA4-64-OK => has_procedure

This was never fixed.  Closing with an appropriate resolution.

*** This bug has been marked as a duplicate of bug 15644 ***

Resolution: FIXED => DUPLICATE

(In reply to David Walser from comment #80)
> This was never fixed.  Closing with an appropriate resolution.
> 
> *** This bug has been marked as a duplicate of bug 15644 ***

Looks wrong to me. It has been fixed and update was issued. Then we chose to make another update to fix the problem that the update caused. As a precaution measure we removed the update that has been issued. This doesn't the update, that has been installed by thousands of users, non-existent.

Resolution: DUPLICATE => FIXED

By closing it as duplicate, we would be re-writing the past.

No, the update was pulled, so this was not fixed.  It's bad enough that we have to use another bug because of procedural issues.  Let's not make this any more illogical than it needs to be.

*** This bug has been marked as a duplicate of bug 15644 ***

Resolution: FIXED => DUPLICATE