From: Florian Obser <florian@openbsd.org>
Subject: Re: doas -l feature
To: tech@openbsd.org
Date: Fri, 06 Jun 2025 13:33:49 +0200

On 2025-06-05 17:17 -06, David Crumpton <david.m.crumpton@gmail.com> wrote:
> I wanted a list option for doas to aid in rule validation for accounts with more rules than the permit wheel rule.  I also added a -m option to show what rule matched on success.
> These options make it easier to debug rules when you have a lot of
> rules for granular control in addition to auditing user rules.
> Example output plus cvs diff follow.

I have no need for this an no opinion if there is still space in doas.c
for this.

But since I read the diff, some comments:

- there is a lot of trailing whitespace in the diff
- knf: if () / for () instead of if() and for() in a bunch of places,


more inline.

[...]

> Index: doas.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/doas/doas.c,v
> retrieving revision 1.99
> diff -u -p -u -p -r1.99 doas.c
> --- doas.c	15 Feb 2024 18:57:58 -0000	1.99
> +++ doas.c	5 Jun 2025 23:02:40 -0000
> @@ -39,7 +39,7 @@
>  static void __dead
>  usage(void)
>  {
> -	fprintf(stderr, "usage: doas [-Lns] [-a style] [-C config] [-u user]"
> +	fprintf(stderr, "usage: doas [-Llmns] [-a style] [-C config] [-u user]"
>  	    " command [arg ...]\n");
>  	exit(1);
>  }
> @@ -114,10 +114,10 @@ match(uid_t uid, gid_t *groups, int ngro
>  	}
>  	if (r->target && uidcheck(r->target, target) != 0)
>  		return 0;
> -	if (r->cmd) {
> +	if (r->cmd && cmd != NULL) {

(I have not checked if doas still does the right thing with this check
or can be tricked into executing something it should not.)

>  		if (strcmp(r->cmd, cmd))
>  			return 0;
> -		if (r->cmdargs) {
> +		if (r->cmdargs && r->cmdargs != NULL) {
>  			/* if arguments were given, they should match explicitly */
>  			for (i = 0; r->cmdargs[i]; i++) {
>  				if (!cmdargs[i])
> @@ -302,6 +302,102 @@ done:
>  	return (unveils);
>  }
>  
> +
> +static void
> +printrule(const struct rule *rule) {
> +	int i;
> +	int group = 0;
> +
> +	if (rule->ident[0] == ':') 
> +		group = 1;
> +	
> +	if(rule->action == PERMIT)
> +		printf("  Permit");
> +	else
> +		printf("    Deny");
> +
> +	if(group) {
> +		printf(" group: %s", rule->ident + 1);
> +	} else {
> +		printf(" user: %s", rule->ident);
> +	}
> +
> +	if (rule->target) 
> +		printf(" (as %s)", rule->target);
> +	else 
> +		printf(" (as root)");
> +
> +	if (rule->cmd) {
> +		printf(" command: %s", rule->cmd);
> +		if(rule->cmdargs)
> +			for(i = 0; rule->cmdargs[i] != NULL; i++)
> +				printf(" %s", rule->cmdargs[i]);
> +	} else
> +		printf(" ALL commands");
> +
> +	if(rule->options || rule->envlist) {
> +		printf(" [");
> +
> +		if (rule->options) {
> +			if (rule->options & NOPASS)
> +				printf(" nopass");
> +
> +			if (rule->options & KEEPENV)
> +				printf(" keepenv");
> +
> +			if (rule->options & PERSIST)
> +				printf(" persist");
> +
> +			if (rule->options & NOLOG)
> +				printf(" nolog");
> +		}
> +
> +		if (rule->envlist)
> +			printf(" setenv");
> +
> +		printf(" ]");
> +	}
> +
> +	if(rule->envlist) {
> +		printf(" {");
> +		for(i = 0; rule->envlist[i] != NULL; i++)
> +			printf(" %s", rule->envlist[i]);
> +		printf(" }");
> +	}
> +	printf("\n");
> +}
> +
> +static void __dead
> +listrules(uid_t uid, gid_t *groups, int ngroups, uid_t target)
> +{
> +	int i;
> +	int found = 0;
> +	struct passwd *pw = getpwuid(uid);

getpwuid() can fail, this needs an error check, otherwise pw->pw_name
will segfault later.

> +	char username[LOGIN_NAME_MAX + 1]; 
> +
> +	if (pledge("stdio rpath", NULL) == -1)

why rpath? I guess parseuid() can be reached? pledge("stdio getpw")
should cover that.

However, this is suddenly a considerable amount of code an
unauthenticated user can run as root. pledge("stdio") would make me much
happier.

-- 
In my defence, I have been left unsupervised.