[weston,v2,2/2] weston-launch: Allow the user to use her login shell

Submitted by Quentin Glidic on June 10, 2016, 2:01 p.m.

Details

Message ID 20160610140122.19670-2-sardemff7+wayland@sardemff7.net
State New
Headers show
Series "weston-launch: Allow the user to use her login shell" ( rev: 3 ) in Wayland

Not browsing as part of any series.

Commit Message

Quentin Glidic June 10, 2016, 2:01 p.m.
From: Quentin Glidic <sardemff7+git@sardemff7.net>

This avoids the need to maintain two parallel shell profile files for
users with a compatible shell (at least bash, sh and zsh are).

There is no major security issue here, as the shell is the one returned
from the password database, and thus is retricted by /etc/shells (or
root override).

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
---

v2: Added some rationale to the commit message

 src/weston-launch.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/weston-launch.c b/src/weston-launch.c
index 140fde1..c206094 100644
--- a/src/weston-launch.c
+++ b/src/weston-launch.c
@@ -108,6 +108,7 @@  struct weston_launch {
 	pid_t child;
 	int verbose;
 	char *new_user;
+	int use_user_shell;
 };
 
 union cmsg_data { unsigned char b[4]; int fd; };
@@ -613,7 +614,10 @@  setup_session(struct weston_launch *wl, char **child_argv)
 	 * We open a new session, so it makes sense
 	 * to run a new login shell
 	 */
-	child_argv[0] = "/bin/sh";
+	if (wl->use_user_shell)
+		child_argv[0] = wl->pw->pw_shell;
+	else
+		child_argv[0] = "/bin/sh";
 	child_argv[1] = "-l";
 	child_argv[2] = "-c";
 	child_argv[3] = BINDIR "/weston \"$@\"";
@@ -675,10 +679,11 @@  static void
 help(const char *name)
 {
 	fprintf(stderr, "Usage: %s [args...] [-- [weston args..]]\n", name);
-	fprintf(stderr, "  -u, --user      Start session as specified username\n");
-	fprintf(stderr, "  -t, --tty       Start session on alternative tty\n");
-	fprintf(stderr, "  -v, --verbose   Be verbose\n");
-	fprintf(stderr, "  -h, --help      Display this help message\n");
+	fprintf(stderr, "  -u, --user              Start session as specified username\n");
+	fprintf(stderr, "  -s, --use-user-shell    Use the user login shell (from PAM) instead of /bin/sh, only has effect with --user\n");
+	fprintf(stderr, "  -t, --tty               Start session on alternative tty\n");
+	fprintf(stderr, "  -v, --verbose           Be verbose\n");
+	fprintf(stderr, "  -h, --help              Display this help message\n");
 }
 
 int
@@ -688,11 +693,12 @@  main(int argc, char *argv[])
 	int i, c;
 	char *tty = NULL;
 	struct option opts[] = {
-		{ "user",    required_argument, NULL, 'u' },
-		{ "tty",     required_argument, NULL, 't' },
-		{ "verbose", no_argument,       NULL, 'v' },
-		{ "help",    no_argument,       NULL, 'h' },
-		{ 0,         0,                 NULL,  0  }
+		{ "user",           required_argument, NULL, 'u' },
+		{ "use-user-shell", no_argument,       NULL, 's' },
+		{ "tty",            required_argument, NULL, 't' },
+		{ "verbose",        no_argument,       NULL, 'v' },
+		{ "help",           no_argument,       NULL, 'h' },
+		{ 0,                0,                 NULL,  0  }
 	};
 
 	memset(&wl, 0, sizeof wl);
@@ -704,6 +710,9 @@  main(int argc, char *argv[])
 			if (getuid() != 0)
 				error(1, 0, "Permission denied. -u allowed for root only");
 			break;
+		case 's':
+			wl.use_user_shell = 1;
+			break;
 		case 't':
 			tty = optarg;
 			break;

Comments

On Fri, 10 Jun 2016 16:01:22 +0200
Quentin Glidic <sardemff7+wayland@sardemff7.net> wrote:

> From: Quentin Glidic <sardemff7+git@sardemff7.net>
> 
> This avoids the need to maintain two parallel shell profile files for
> users with a compatible shell (at least bash, sh and zsh are).
> 
> There is no major security issue here, as the shell is the one returned
> from the password database, and thus is retricted by /etc/shells (or
> root override).
> 
> Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>
> ---
> 
> v2: Added some rationale to the commit message

Hi Quentin,

thanks for that, this patch is now:
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

However, I would like to have someone say they would benefit from this
patch. Then it would be easy to land this. Otherwise I'm a bit torn.
Yes, it's a simple addition and it looks safe, but it's still adding a
new optional path to a setuid-root binary, so it's not with zero cost.


Thanks,
pq

>  src/weston-launch.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/src/weston-launch.c b/src/weston-launch.c
> index 140fde1..c206094 100644
> --- a/src/weston-launch.c
> +++ b/src/weston-launch.c
> @@ -108,6 +108,7 @@ struct weston_launch {
>  	pid_t child;
>  	int verbose;
>  	char *new_user;
> +	int use_user_shell;
>  };
>  
>  union cmsg_data { unsigned char b[4]; int fd; };
> @@ -613,7 +614,10 @@ setup_session(struct weston_launch *wl, char **child_argv)
>  	 * We open a new session, so it makes sense
>  	 * to run a new login shell
>  	 */
> -	child_argv[0] = "/bin/sh";
> +	if (wl->use_user_shell)
> +		child_argv[0] = wl->pw->pw_shell;
> +	else
> +		child_argv[0] = "/bin/sh";
>  	child_argv[1] = "-l";
>  	child_argv[2] = "-c";
>  	child_argv[3] = BINDIR "/weston \"$@\"";
> @@ -675,10 +679,11 @@ static void
>  help(const char *name)
>  {
>  	fprintf(stderr, "Usage: %s [args...] [-- [weston args..]]\n", name);
> -	fprintf(stderr, "  -u, --user      Start session as specified username\n");
> -	fprintf(stderr, "  -t, --tty       Start session on alternative tty\n");
> -	fprintf(stderr, "  -v, --verbose   Be verbose\n");
> -	fprintf(stderr, "  -h, --help      Display this help message\n");
> +	fprintf(stderr, "  -u, --user              Start session as specified username\n");
> +	fprintf(stderr, "  -s, --use-user-shell    Use the user login shell (from PAM) instead of /bin/sh, only has effect with --user\n");
> +	fprintf(stderr, "  -t, --tty               Start session on alternative tty\n");
> +	fprintf(stderr, "  -v, --verbose           Be verbose\n");
> +	fprintf(stderr, "  -h, --help              Display this help message\n");
>  }
>  
>  int
> @@ -688,11 +693,12 @@ main(int argc, char *argv[])
>  	int i, c;
>  	char *tty = NULL;
>  	struct option opts[] = {
> -		{ "user",    required_argument, NULL, 'u' },
> -		{ "tty",     required_argument, NULL, 't' },
> -		{ "verbose", no_argument,       NULL, 'v' },
> -		{ "help",    no_argument,       NULL, 'h' },
> -		{ 0,         0,                 NULL,  0  }
> +		{ "user",           required_argument, NULL, 'u' },
> +		{ "use-user-shell", no_argument,       NULL, 's' },
> +		{ "tty",            required_argument, NULL, 't' },
> +		{ "verbose",        no_argument,       NULL, 'v' },
> +		{ "help",           no_argument,       NULL, 'h' },
> +		{ 0,                0,                 NULL,  0  }
>  	};
>  
>  	memset(&wl, 0, sizeof wl);
> @@ -704,6 +710,9 @@ main(int argc, char *argv[])
>  			if (getuid() != 0)
>  				error(1, 0, "Permission denied. -u allowed for root only");
>  			break;
> +		case 's':
> +			wl.use_user_shell = 1;
> +			break;
>  		case 't':
>  			tty = optarg;
>  			break;