[PATCH] sulogin: fix FEATURE_SHADOWPASSWDS sematics

[Rich Felker]

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