[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190725103333.GA30296@pirotess.home>
Date: Thu, 25 Jul 2019 12:33:33 +0200
From: Ismael Luceno <ismael@...ev.co.uk>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v2] glob: implement GLOB_TILDE
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".
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
