[PATCH 00/16] Improvements, mainly to user name handling and scp.

Matt Johnston matt at ucc.asn.au
Tue Dec 29 23:03:34 AWST 2015


Hi Michael,

I think the general change of these patches makes sense
(avoiding failure when a local user doesn't exist) but it
needs to be more minimal. scp comes straight from OpenSSH
with some small changes for uClinux etc. I've tried to
avoid additional changes since it really needs updating to a
more recent OpenSSH - the more changes, the larger that work
is. Upstreaming it might be an option.

Cheers,
Matt

On Mon, Dec 07, 2015 at 10:42:59PM -0000, Michael Witten wrote:
> User names have hitherto been handled neither consistently nor well; this
> series alleviates at least some of the issues.
> 
> Fear not the long patch series!
> 
> Most commits involve a fairly small number of changes; while I could have
> consolidated these changes into fewer commits, I think the series as a whole
> provides a better narration for what's going.
> 
> Besides a few small improvements along the way, the main thrust is:
> 
>   * Removing user-name handling from `scp' (in favor of using the
>     handling that is already present in `dropbear'/`dbclient').
>     
>   * Lazily looking up the current user's name.
>   
>   * Removing unused code.
> 
> Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-):
> 
>   cli-main.c       |   2 +-
>   cli-runopts.c    |  32 ++++++--------
>   common-session.c |   1 +
>   runopts.h        |   2 +-
>   scp.c            | 125 ++++++++++---------------------------------------------
>   scpmisc.c        |  31 +-------------
>   scpmisc.h        |   2 -
> 
> This is the series:
> 
>   [01] scp: Insert comma into stderr message
>   [02] scp: Have `fatal()' append a newline to the message
>   [03] scp: only pass `-v' when DEBUG_TRACE is set
>   [04] scp: `-l%s' -> `-l %s'
>   [05] scp: const/static correctness improvements
>   [06] scp: Introduce `get_user_name()'
>   [07] scp: Use "unknown" when the user name cannot be retrieved
>   [08] scp: style: simplify code by using a tertiary operator
>   [09] scp: Use `get_user_name()' more often
>   [10] scp: Simplify code now that the user name is never `NULL'
>   [11] scp: Remove parsing of `[user@]host'
>   [12] scp: Remove unused functions
>   [13] scp: Remove `replacearg()'
>   [14] runopts: Mark `*cli_runopts.own_user' as `const'
>   [15] runopts: There's no reason to make a duplicate of "unknown"
>   [16] runopts: Re-introduce the `get_user_name()' function from `scp' development
> 
> Lastly, for convenience in reviewing the changes, here is the overall patch:
> 
> --- a/cli-main.c
> +++ b/cli-main.c
> @@ -135,7 +135,7 @@
>  static void cli_proxy_cmd(int *sock_in, int *sock_out) {
>  	int ret;
>  
> -	fill_passwd(cli_opts.own_user);
> +	fill_passwd(get_user_name());
>  
>  	ret = spawn_command(exec_proxy_cmd, cli_opts.proxycmd,
>  			sock_out, sock_in, NULL, NULL);
> --- a/cli-runopts.c
> +++ b/cli-runopts.c
> @@ -36,7 +36,6 @@
>  static void printhelp();
>  static void parse_hostname(const char* orighostarg);
>  static void parse_multihop_hostname(const char* orighostarg, const char* argv0);
> -static void fill_own_user();
>  #ifdef ENABLE_CLI_PUBKEY_AUTH
>  static void loadidentityfile(const char* filename, int warnfail);
>  #endif
> @@ -102,6 +101,17 @@
>  					
>  }
>  
> +const char *get_user_name() {
> +	static const char *user_name = NULL;
> +
> +	if (user_name == NULL) {
> +		struct passwd *pwd = getpwuid(getuid());
> +		user_name = pwd ? m_strdup(pwd->pw_name) : "unknown";
> +	}
> +
> +	return user_name;
> +}
> +
>  void cli_getopts(int argc, char ** argv) {
>  	unsigned int i, j;
>  	char ** next = 0;
> @@ -175,8 +185,6 @@
>  	opts.keepalive_secs = DEFAULT_KEEPALIVE;
>  	opts.idle_timeout_secs = DEFAULT_IDLE_TIMEOUT;
>  
> -	fill_own_user();
> -
>  	for (i = 1; i < (unsigned int)argc; i++) {
>  		/* Handle non-flag arguments such as hostname or commands for the remote host */
>  		if (argv[i][0] != '-')
> @@ -640,7 +648,7 @@
>  	}
>  
>  	if (cli_opts.username == NULL) {
> -		cli_opts.username = m_strdup(cli_opts.own_user);
> +		cli_opts.username = m_strdup(get_user_name());
>  	}
>  
>  	port = strchr(cli_opts.remotehost, '^');
> @@ -695,22 +703,6 @@
>  }
>  #endif
>  
> -static void fill_own_user() {
> -	uid_t uid;
> -	struct passwd *pw = NULL; 
> -
> -	uid = getuid();
> -
> -	pw = getpwuid(uid);
> -	if (pw && pw->pw_name != NULL) {
> -		cli_opts.own_user = m_strdup(pw->pw_name);
> -	} else {
> -		dropbear_log(LOG_INFO, "Warning: failed to identify current user. Trying anyway.");
> -		cli_opts.own_user = m_strdup("unknown");
> -	}
> -
> -}
> -
>  #ifdef ENABLE_CLI_ANYTCPFWD
>  /* Turn a "[listenaddr:]listenport:remoteaddr:remoteport" string into into a forwarding
>   * set, and add it to the forwarding list */
> --- a/common-session.c
> +++ b/common-session.c
> @@ -581,6 +581,7 @@
>  		return ses.authstate.pw_shell;
>  	}
>  }
> +
>  void fill_passwd(const char* username) {
>  	struct passwd *pw = NULL;
>  	if (ses.authstate.pw_name)
> --- a/runopts.h
> +++ b/runopts.h
> @@ -127,7 +127,6 @@
>  	char *remotehost;
>  	char *remoteport;
>  
> -	char *own_user;
>  	char *username;
>  
>  	char *cmd;
> @@ -165,6 +164,7 @@
>  
>  extern cli_runopts cli_opts;
>  void cli_getopts(int argc, char ** argv);
> +const char *get_user_name();
>  
>  #ifdef ENABLE_USER_ALGO_LIST
>  void parse_ciphers_macs();
> --- a/scp.c
> +++ b/scp.c
> @@ -172,25 +172,21 @@
>   */
>  
>  static void
> -arg_setup(char *host, char *remuser, char *cmd)
> +arg_setup(const char *user_at_host, const char *cmd)
>  {
> -	replacearg(&args, 0, "%s", ssh_program);
> -	if (remuser != NULL)
> -		addargs(&args, "-l%s", remuser);
> -	addargs(&args, "%s", host);
> +	addargs(&args, "%s", user_at_host);
>  	addargs(&args, "%s", cmd);
>  }
>  
>  int
> -do_cmd(char *host, char *remuser, char *cmd, int *fdin, int *fdout, int argc)
> +do_cmd(const char *user_at_host, const char *cmd, int *fdin, int *fdout, int argc)
>  {
>  	int pin[2], pout[2], reserved[2];
>  
>  	if (verbose_mode)
>  		fprintf(stderr,
> -		    "Executing: program %s host %s, user %s, command %s\n",
> -		    ssh_program, host,
> -		    remuser ? remuser : "(unspecified)", cmd);
> +		    "Executing: program %s, login %s, command %s\n",
> +		    ssh_program, user_at_host, cmd);
>  
>  	/*
>  	 * Reserve two descriptors so that the real pipes won't get
> @@ -211,7 +207,7 @@
>  	/* uClinux needs to build the args here before vforking,
>  	   otherwise we do it later on. */
>  #ifdef USE_VFORK
> -	arg_setup(host, remuser, cmd);
> +	arg_setup(user_at_host, cmd);
>  #endif
>  
>  	/* Fork a child to execute the command on the remote host using ssh. */
> @@ -231,7 +227,7 @@
>  		close(pout[1]);
>  
>  #ifndef USE_VFORK
> -		arg_setup(host, remuser, cmd);
> +		arg_setup(user_at_host, cmd);
>  #endif
>  
>  		execvp(ssh_program, args.list);
> @@ -251,16 +247,10 @@
>  	xfree(args.list[args.num-1]);
>  	args.list[args.num-1]=NULL;
>  	args.num--;
> -	/* pop host */
> +	/* pop user_at_host */
>  	xfree(args.list[args.num-1]);
>  	args.list[args.num-1]=NULL;
>  	args.num--;
> -	/* pop user */
> -	if (remuser != NULL) {
> -		xfree(args.list[args.num-1]);
> -		args.list[args.num-1]=NULL;
> -		args.num--;
> -	}
>  #endif
>  
>  	/* Parent.  Close the other side, and return the local side. */
> @@ -282,12 +272,9 @@
>  BUF *allocbuf(BUF *, int, int);
>  void lostconn(int);
>  void nospace(void);
> -int okname(char *);
>  void run_err(const char *,...);
>  void verifydir(char *);
>  
> -struct passwd *pwd;
> -uid_t userid;
>  int errs, remin, remout;
>  int pflag, iamremote, iamrecursive, targetshouldbedirectory;
>  
> @@ -362,7 +349,9 @@
>  			ssh_program = xstrdup(optarg);
>  			break;
>  		case 'v':
> +#ifdef DEBUG_TRACE
>  			addargs(&args, "-v");
> +#endif
>  			verbose_mode = 1;
>  			break;
>  		case 'q':
> @@ -393,9 +382,6 @@
>  	argc -= optind;
>  	argv += optind;
>  
> -	if ((pwd = getpwuid(userid = getuid())) == NULL)
> -		fatal("unknown user %u", (u_int) userid);
> -
>  	if (!isatty(STDERR_FILENO))
>  		showprogress = 0;
>  
> @@ -459,7 +445,8 @@
>  toremote(char *targ, int argc, char **argv)
>  {
>  	int i, len;
> -	char *bp, *host, *src, *suser, *thost, *tuser, *arg;
> +	const char *targ_user_at_host;
> +	char *bp, *src;
>  	arglist alist;
>  
>  	memset(&alist, '\0', sizeof(alist));
> @@ -469,30 +456,17 @@
>  	if (*targ == 0)
>  		targ = ".";
>  
> -	arg = xstrdup(argv[argc - 1]);
> -	if ((thost = strrchr(arg, '@'))) {
> -		/* user at host */
> -		*thost++ = 0;
> -		tuser = arg;
> -		if (*tuser == '\0')
> -			tuser = NULL;
> -	} else {
> -		thost = arg;
> -		tuser = NULL;
> -	}
> -
> -	if (tuser != NULL && !okname(tuser)) {
> -		xfree(arg);
> -		return;
> -	}
> +	targ_user_at_host = argv[argc - 1];
>  
>  	for (i = 0; i < argc - 1; i++) {
>  		src = colon(argv[i]);
>  		if (src) {	/* remote to remote */
>  			freeargs(&alist);
>  			addargs(&alist, "%s", ssh_program);
> +#ifdef DEBUG_TRACE
>  			if (verbose_mode)
>  				addargs(&alist, "-v");
> +#endif
>  #if 0
>  			/* Disabled since dbclient won't understand them
>  			   and scp works fine without them. */
> @@ -504,27 +478,11 @@
>  			*src++ = 0;
>  			if (*src == 0)
>  				src = ".";
> -			host = strrchr(argv[i], '@');
> -
> -			if (host) {
> -				*host++ = 0;
> -				host = cleanhostname(host);
> -				suser = argv[i];
> -				if (*suser == '\0')
> -					suser = pwd->pw_name;
> -				else if (!okname(suser))
> -					continue;
> -				addargs(&alist, "-l");
> -				addargs(&alist, "%s", suser);
> -			} else {
> -				host = cleanhostname(argv[i]);
> -			}
> -			addargs(&alist, "%s", host);
> +
> +			addargs(&alist, "%s", argv[i]); /* [user@]host */
>  			addargs(&alist, "%s", cmd);
>  			addargs(&alist, "%s", src);
> -			addargs(&alist, "%s%s%s:%s",
> -			    tuser ? tuser : "", tuser ? "@" : "",
> -			    thost, targ);
> +			addargs(&alist, "%s:%s", targ_user_at_host, targ);
>  			if (do_local_cmd(&alist) != 0)
>  				errs = 1;
>  		} else {	/* local to remote */
> @@ -532,8 +490,7 @@
>  				len = strlen(targ) + CMDNEEDS + 20;
>  				bp = xmalloc(len);
>  				(void) snprintf(bp, len, "%s -t %s", cmd, targ);
> -				host = cleanhostname(thost);
> -				if (do_cmd(host, tuser, bp, &remin,
> +				if (do_cmd(targ_user_at_host, bp, &remin,
>  				    &remout, argc) < 0)
>  					exit(1);
>  				if (response() < 0)
> @@ -549,7 +506,7 @@
>  tolocal(int argc, char **argv)
>  {
>  	int i, len;
> -	char *bp, *host, *src, *suser;
> +	char *bp, *src;
>  	arglist alist;
>  
>  	memset(&alist, '\0', sizeof(alist));
> @@ -572,20 +529,10 @@
>  		*src++ = 0;
>  		if (*src == 0)
>  			src = ".";
> -		if ((host = strrchr(argv[i], '@')) == NULL) {
> -			host = argv[i];
> -			suser = NULL;
> -		} else {
> -			*host++ = 0;
> -			suser = argv[i];
> -			if (*suser == '\0')
> -				suser = pwd->pw_name;
> -		}
> -		host = cleanhostname(host);
>  		len = strlen(src) + CMDNEEDS + 20;
>  		bp = xmalloc(len);
>  		(void) snprintf(bp, len, "%s -f %s", cmd, src);
> -		if (do_cmd(host, suser, bp, &remin, &remout, argc) < 0) {
> +		if (do_cmd(argv[i], bp, &remin, &remout, argc) < 0) {
>  			(void) xfree(bp);
>  			++errs;
>  			continue;
> @@ -1190,36 +1137,6 @@
>  	killchild(0);
>  }
>  
> -int
> -okname(char *cp0)
> -{
> -	int c;
> -	char *cp;
> -
> -	cp = cp0;
> -	do {
> -		c = (int)*cp;
> -		if (c & 0200)
> -			goto bad;
> -		if (!isalpha(c) && !isdigit(c)) {
> -			switch (c) {
> -			case '\'':
> -			case '"':
> -			case '`':
> -			case ' ':
> -			case '#':
> -				goto bad;
> -			default:
> -				break;
> -			}
> -		}
> -	} while (*++cp);
> -	return (1);
> -
> -bad:	fprintf(stderr, "%s: invalid user name\n", cp0);
> -	return (0);
> -}
> -
>  BUF *
>  allocbuf(BUF *bp, int fd, int blksize)
>  {
> --- a/scpmisc.c
> +++ b/scpmisc.c
> @@ -104,16 +104,6 @@
>  }
>  
>  char *
> -cleanhostname(char *host)
> -{
> -	if (*host == '[' && host[strlen(host) - 1] == ']') {
> -		host[strlen(host) - 1] = '\0';
> -		return (host + 1);
> -	} else
> -		return host;
> -}
> -
> -char *
>  colon(char *cp)
>  {
>  	int flag = 0;
> @@ -165,26 +155,6 @@
>  }
>  
>  void
> -replacearg(arglist *args, u_int which, char *fmt, ...)
> -{
> -	va_list ap;
> -	char *cp;
> -	int r;
> -
> -	va_start(ap, fmt);
> -	r = vasprintf(&cp, fmt, ap);
> -	va_end(ap);
> -	if (r == -1)
> -		fatal("replacearg: argument too long");
> -
> -	if (which >= args->num)
> -		fatal("replacearg: tried to replace invalid arg %d >= %d",
> -		    which, args->num);
> -	xfree(args->list[which]);
> -	args->list[which] = cp;
> -}
> -
> -void
>  freeargs(arglist *args)
>  {
>  	u_int i;
> @@ -223,6 +193,7 @@
>  	va_start(args, fmt);
>  	vfprintf(stderr, fmt, args);
>  	va_end(args);
> +	fputc('\n', stderr);
>  	exit(255);
>  }
>  
> --- a/scpmisc.h
> +++ b/scpmisc.h
> @@ -21,7 +21,6 @@
>  void	 unset_nonblock(int);
>  void	 set_nodelay(int);
>  int	 a2port(const char *);
> -char	*cleanhostname(char *);
>  char	*colon(char *);
>  long	 convtime(const char *);
>  
> @@ -34,7 +33,6 @@
>  	int     nalloc;
>  };
>  void	 addargs(arglist *, char *, ...);
> -void	 replacearg(arglist *, u_int, char *, ...);
>  void	 freeargs(arglist *);
>  
>  /* from xmalloc.h */


More information about the Dropbear mailing list