Re: [PATCH v2] glob: implement GLOB_TILDE

[Ismael Luceno <ismael@iodev.co.uk>]

On 24/Jul/2019 23:48, Rich Felker wrote:
<...>
> > +	char *p = *pat + 1;
> > +	size_t i = 0;
> > +
> > +	char *name_end = strchrnul(p, '/');
> > +	if (*name_end) *name_end++ = 0;
> > +	*pat = name_end;
> > +
> > +	char *home = (*p || issetugid()) ? NULL : getenv("HOME");
> 
> I still don't see any justification for violating the semantics of ~
> because the program is suid.

I could only find a couple of implementations which don't check for
privileges:
- 4.3BSD
  + NetBSD
- uClibc
  + uClibc-ng

Almost everyone else does:
- FreeBSD
  + MidnightBSD
  + DragonFlyBSD
  + Android (Bionic)
- GNU
- Newlib
- OpenBSD
  + MirBSD
- Solaris
  + Illumos


I think most users/software would expect it.

> Also, issetugid isn't in the namespace we can use here. Rather than
> poking directly at libc.secure, if we needed this I think it would be
> better to make a namespace-protected alias, but I don't think it's the
> right behavior anyway.

I would go with that.

> > +	if (!home) {
> > +		struct passwd pw, *res;
> > +		switch (*p ? getpwnam_r(p, &pw, buf, PATH_MAX, &res)
> > +			   : getpwuid_r(getuid(), &pw, buf, PATH_MAX, &res)) {
> 
> Same here. I'm not actually sure what should happen if $HOME is unset,
> but it's probably whatever the Shell Command Language specification
> says.

All the implementations I could find fall back to getpw*.

> > +		default:
> > +			return GLOB_ABORTED;
> > +		case ERANGE: case ENOMEM:
> > +			return GLOB_NOSPACE;
> > +		case 0:
> > +			if (!res) return GLOB_NOMATCH;
> 
> The man page says:
> 
>     If the username is invalid, or the home directory cannot be
>     determined, then no substitution is performed.
> 
> and the glibc manual says:
> 
>     If the user name is not valid or the home directory cannot be
>     determined for some reason the pattern is left untouched and
>     itself used as the result
>
> So I think this should not be an error. I'm not entirely clear if it's
> supposed to suppress all further glob expansion and just return a
> single literal result, or treat "~" or "~nosuchuser" as a literal
> component and search it.

It's supposed to continue; but IMO that's neither intuitive nor safe.

I guess that's why glibc added GLOB_TILDE_CHECK, but it should be the
only possibility.

However, if you feel strongly about it, I can add _CHECK instead.

I think:
- ENOMEM should map to NOSPACE.
- a closer look reveals that all other errors should map to NOMATCH.

> > +		}
> > +		home = pw.pw_dir;
> > +	}
> > +	while (i < PATH_MAX - 2 && *home)
> > +		buf[i++] = *home++;
> 
> This could be strnlen+memmove, but perhaps just open coding it like
> you've done is simpler and lighter.
> 
> > +	if (i > PATH_MAX - 2)
> 
> Off-by-one. If it stopped due to reaching the limit in the above loop,
> you'll have equality not greater-than, then overflow below. Perhaps
> strnlen+memmove would be easier to ensure the safety of.

No overflow:

i++ => PATH_MAX - 2
i   => PATH_MAX - 1

> > +		return GLOB_NOSPACE;
> 
> GLOB_NOSPACE is for allocation errors.

Right.

> This is a no-match case, since there can be no matches for a pattern
> PATH_MAX or longer.

It must work for "~\0".