From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14452 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v2] glob: implement GLOB_TILDE Date: Thu, 25 Jul 2019 10:30:24 -0400 Message-ID: <20190725143024.GL1506@brightrain.aerifal.cx> References: <20190724213338.27138-1-ismael@iodev.co.uk> <20190725034814.GI1506@brightrain.aerifal.cx> <20190725103333.GA30296@pirotess.home> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="110960"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-14468-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jul 25 16:30:48 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hqelW-000Sdj-LY for gllmg-musl@m.gmane.org; Thu, 25 Jul 2019 16:30:42 +0200 Original-Received: (qmail 32288 invoked by uid 550); 25 Jul 2019 14:30:37 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 32182 invoked from network); 25 Jul 2019 14:30:36 -0000 Content-Disposition: inline In-Reply-To: <20190725103333.GA30296@pirotess.home> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14452 Archived-At: 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 > > + 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