From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14451 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Ismael Luceno Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v2] glob: implement GLOB_TILDE Date: Thu, 25 Jul 2019 12:33:33 +0200 Message-ID: <20190725103333.GA30296@pirotess.home> References: <20190724213338.27138-1-ismael@iodev.co.uk> <20190725034814.GI1506@brightrain.aerifal.cx> 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="146654"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.12.1 (2019-06-15) To: musl@lists.openwall.com Original-X-From: musl-return-14467-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jul 25 12:33:52 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 1hqb4K-000c4H-J4 for gllmg-musl@m.gmane.org; Thu, 25 Jul 2019 12:33:52 +0200 Original-Received: (qmail 1787 invoked by uid 550); 25 Jul 2019 10:33:49 -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 1757 invoked from network); 25 Jul 2019 10:33:48 -0000 Content-Disposition: inline In-Reply-To: <20190725034814.GI1506@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14451 Archived-At: 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".