[PATCH alternative] sulogin: fix FEATURE_SHADOWPASSWDS sematics

[Tito]

On Saturday 16 November 2013 19:52:27 Rich Felker wrote:
> On Thu, Nov 14, 2013 at 03:43:29PM +0800, Qi.Chen at windriver.com wrote:
> > From: Chen Qi <qi.chen at windriver.com>
> > 
> > In other parts of busybox such as adduser and deluser, FEATURE_SHADOWPASSWDS
> > indicates that the command will check /etc/shadow but it doesn't require
> > it as a necessity.
> > 
> > In sulogin, however, if FEATURE_SHADOWPASSWDS is enabled, the command requires
> > /etc/shadow to be available.
> > 
> > Here's a problem with the above behaviour. In a sysv-based system, if we boot
> > into runlevel S by default, the system will diplay the 'no password for root'
> > error message without a stop at boot time
> > 
> > The patch fixes the above problem by making sulogin to follow the same sematics
> > of FEATURE_SHADOWPASSWDS as in useradd and userdel.
> > 
> > Signed-off-by: Chen Qi <Qi.Chen at windriver.com>
> > ---
> >  loginutils/sulogin.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/loginutils/sulogin.c b/loginutils/sulogin.c
> > index bd2b09e..b03540a 100644
> > --- a/loginutils/sulogin.c
> > +++ b/loginutils/sulogin.c
> > @@ -68,10 +68,9 @@ int sulogin_main(int argc UNUSED_PARAM, char **argv)
> >  		 * At least glibc 2.4 does this. Be extra paranoid here. */
> >  		struct spwd *result = NULL;
> >  		int r = getspnam_r(pwd->pw_name, &spw, buffer, sizeof(buffer), &result);
> > -		if (r || !result) {
> > -			goto auth_error;
> > +		if (!r && result) {
> > +			pwd->pw_passwd = result->sp_pwdp;
> >  		}
> > -		pwd->pw_passwd = result->sp_pwdp;
> >  	}
> >  #endif
> 
> This is definitely not acceptable from a security standpoint.
> getspnam_r could fail for all sorts of reasons such as exhausting file
> descriptors, memory, etc. Falling back to using /etc/passwd in this
> case (which might contain blank password fields since they're not
> intended to be used) would be a huge security hole. Why is getspnam_r
> failing for you in runlevel S?
> 
> Rich

Hi,
usually passwd sets password field to x in /etc/passwd if password
field in /etc/shadow is used so in a correctly managed system
there should not be empty password fields at all.

From man 5 passwd:

The encrypted password field may be blank, in which case no password is required to
       authenticate as the specified login name. However, some applications which read the
       /etc/passwd file may decide not to permit any access at all if the password field is blank.
       If the password field is a lower-case “x”, then the encrypted password is actually stored in
       the shadow(5) file instead; there must be a corresponding line in the /etc/shadow file, or
       else the user account is invalid. If the password field is any other string, then it will be
       treated as an encrypted password, as specified by crypt(3).


I wonder at this point if it could be acceptable for sulogin to rather
act the same way as libbb's correct_passwd function does in login and su
to check in /etc/passwd if the passwd field is set to 'x'
and use /etc/shadow only in that case. 

--- loginutils/sulogin.c.original       2013-06-02 14:00:43.000000000 +0200
+++ loginutils/sulogin.c        2013-11-16 21:49:36.433438048 +0100
@@ -63,7 +63,7 @@
        }
 
 #if ENABLE_FEATURE_SHADOWPASSWDS
-       {
+       if ((pwd->pw_passwd[0] == 'x' || pwd->pw_passwd[0] == '*') && !pwd->pw_passwd[1]) {
                /* getspnam_r may return 0 yet set result to NULL.
                 * At least glibc 2.4 does this. Be extra paranoid here. */
                struct spwd *result = NULL;

This is patch is only compile tested so please Chen Qi could you check if it fixes your problem?

BTW by reading man sulogin I spotted:

"If the root account is locked, no password prompt is displayed and sulogin behaves as if the correct password were entered."

This feature is missing in busybox: is it worth to be implemented?

Ciao,
Tito


-------------- next part --------------
A non-text attachment was scrubbed...
Name: sulogin.patch
Type: text/x-patch
Size: 571 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20131116/990a27ab/attachment.bin>