[PATCH] sulogin: allow system maintenance login if root password is empty

[Tito]

Hi,
i'm just wondering why root should have an empty password.....

On Monday 20 May 2013 13:07:33 Jonathan Liu wrote:
> The current password checking is unable to distinguish between the user
> entering an empty password or pressing Control-D. As a result, an empty
> password always results in normal startup.
> 
> We modify bb_ask to store the EOF status after the null terminator if
> the password is empty. This allows sulogin to properly check if
> Control-D was pressed.
> 
> Signed-off-by: Jonathan Liu <net147 at gmail.com>
> ---
>  libbb/bb_askpass.c   | 3 +++
>  loginutils/sulogin.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libbb/bb_askpass.c b/libbb/bb_askpass.c
> index fe2b506..4df0901 100644
> --- a/libbb/bb_askpass.c
> +++ b/libbb/bb_askpass.c
> @@ -75,6 +75,9 @@ char* FAST_FUNC bb_ask(const int fd, int timeout, const char *prompt)
>  		 || ++i == sizeof_passwd-1 /* line limit */
>  		) {
>  			ret[i] = '\0';
> +			/* if empty, store EOF status after null terminator */
> +			if (i == 0)
> +				ret[i + 1] = (r == 0);
>  			break;
>  		}
>  	}
> diff --git a/loginutils/sulogin.c b/loginutils/sulogin.c
> index bd2b09e..9fcf530 100644
> --- a/loginutils/sulogin.c
> +++ b/loginutils/sulogin.c
> @@ -84,7 +84,7 @@ int sulogin_main(int argc UNUSED_PARAM, char **argv)
>  				"Give root password for system maintenance\n"
>  				"(or type Control-D for normal startup):");
>  
> -		if (!cp || !*cp) {
> +		if (!cp || (!*cp && cp[1])) {
>  			bb_info_msg("Normal startup");
>  			return 0;
>  		}
> 

Looking at the code there are a few things that make ring my bells:
a) writing stuff after the null termination looks evil
    what will happen if we have a 128 or 127 char long password?
b) reading after the null termination even so and the reason
    is that we don't know what could be in the buffer

Also a comment in the sulogin code made me suspicious:

		/* cp points to a static buffer that is zeroed every time */
		cp = bb_ask(STDIN_FILENO, timeout, "test: ");


but looking at bb_askpass.c there is no static buffer nor is the buffer zeroed every
time as the comment states so or we have to fix the code or the comment,
this is easily demonstrated with this test program:

int sulogin_main(int argc UNUSED_PARAM, char **argv)
{
	char *cp;

	while (1) {
		/* cp points to a static buffer that is zeroed every time */
		cp = bb_ask(STDIN_FILENO, timeout, "test: ");
		if (!cp || !*cp) {
			bb_info_msg("Normal startup");
			return 0;
		}
	}
	return 0;
}

and a slightly modified version of bb_askpass:

	while (1) {
		int r = read(fd, &ret[i], 1);
		if (r < 0) {
			/* read is interrupted by timeout or ^C */
			ret = NULL;
			break;
		}
		if (r == 0 /* EOF */
		 || ret[i] == '\r' || ret[i] == '\n' /* EOL */
		 || ++i == sizeof_passwd-1 /* line limit */
		) {
			//ret[i] = '\0';
+			ret[sizeof_passwd-1] = '\0';
			break;
		}
	}
+	printf("'%s'", ret);

the output in this case is:

./busybox sulogin
sulogin: using fallback suid method
test: 'prova
'
test: '
rova
'
test: '
rova
'
test: '
rova
'
test: '
rova
'
test: 

this proves that the buffer is not zeroed every time.
An alternative solution could be to propagate CTRL-D EOL or \n
to the calling function and do the check there.
Hope this helps.

Ciao,
Tito