Re: [PATCH v2] glob: implement GLOB_TILDE

[Rich Felker <dalias@libc.org>]

On Thu, Jul 25, 2019 at 12:33:33PM +0200, Ismael Luceno wrote:
> 
> 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.

Up to now I've tried to avoid just being "the maintainer's job is to
say no", but here the maintainer's job is to say no. There are 3 (or
4, if you count the dynamic linker) places in musl where libc.secure
is used:

1. preopening /dev/null on fds 0-2 if they start closed
2. disallowing arbitrary user-provided timezone files
3. disallowing arbitrary user-provided locale files
4. disallowing LD_PRELOAD/LD_LIBRARY_PATH

Here, (1) is explicitly allowed by POSIX, and the environment with
them initially closed is deemed a non-conforming execution environment
anyway. (2) and (3) are somewhat unfortunate, but necessary in order
to prevent invoking-user-controlled state from causing undefined
behavior; I actually hope to remove (2) at some point by doing a
read-and-validate instead of mmap in the suid case but it's low
priority. (4) is obviously necessary.

There's nowhere in musl where we violate the contract of an interface
with the application in suid mode because there's some conceivable way
the application could do something dangerous/wrong with the result. I
was likewise adamant about _FORTIFY_SOURCE only catching actual
overflows/UB, not things that are just "likely misusage".

~ should produce $HOME. It's arguable that it should instead lookup
the homedir based on getlogin like glibc documents, but (1) this
contradicts the Shell Command Language definition, and (2) it's not
viable in musl to begin with since getlogin() just gives you
getenv("LOGNAME").

> > > +	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*.

This seems consistent with POSIX sh ~:

    If the login name is null (that is, the tilde-prefix contains only
    the tilde), the tilde-prefix is replaced by the value of the
    variable HOME. If HOME is unset, the results are unspecified.
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I'm not sure if it's the best behavior but I don't have any strong
opinion on it.

> > > +		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.

At first I thought I'd prefer that, and still might, but POSIX doesn't
seem to mandate anything here for the shell:

    If the system does not recognize the login name, the results are
    undefined.

But in any case we should probably define and support both flags, even
if both have the same behavior. This way applications that care will
continue to get the "check" behavior if in the future there turns out
to be reason to make GLOB_TILDE do the "non-check" thing. This will
also improve glibc ABI-compat in case a caller is using the "check"
version of the flag. (BTW I didn't check, but I assume your flag value
matches the glibc one?)

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

None of the three error codes are really good, but NOMATCH is probably
best. GLOB_ABORTED may be more appropriate with GLOB_ERR set (to allow
application to distinguish between an error processing the glob and
simple nonexistence of matches). Thoughts?

> > > +		}
> > > +		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

On the last iteration of the above loop that terminates due to length
limit, i<PATH_MAX-2, so i==PATH_MAX-3. The ++ in the loop body results
in i==PATH_MAX-2.

You're right that it's not an overflow, because you only write the '/'
and null terminator below, but the user's homedir pathname has been
silently truncated (unless you happened to hit !*home on the same
iteration). I think a test like "if (*home) error-out.." would work
here.

> > > +		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".

Assuming ~ expands to something that fits.

Rich