Re: [musl] get/set*ent functions and real world applications

["Érico Nogueira" <ericonr@disroot.org>]

On Mon Oct 11, 2021 at 10:32 AM -03, (GalaxyMaster) wrote:
> Hello,
>
> I really enjoy the consise language used in musl, however, I think I
> found one
> place where albeit the code is corect and elegant it does not produce
> secure
> outcome. I am talking about getpwent(), getgrent(), and their
> counterparts:
> putpwent() and putgrent().
>
> All these functions behave well, when the input is well defined.
> However,
> given the nature of the files these functions are working with and the
> sensitivity of the information contained within, it is a point of
> potential
> abuse.
>
> On a Glibc-based system, if an erronneous entry like
> "user:x::1000::/home/user:/bin/bash"
> it will stay that way as an erroneous entry, but will be ignored, hence
> being harmless.
>
> On musl-based system, on the other hand, that entry would result in a
> root
> account due to the way atou() works in getpwent_a.c:
> ===
> galaxy@musl:~/musl-tests $ cat test-ent.c
> #include <sys/types.h>
> #include <pwd.h>
> #include <stdio.h>
> #include <errno.h>
>
> int main() {
> struct passwd *pw;
> printf("Reading /etc/passwd:\n");
> errno = 0;
> while ((pw = getpwent())) {
> printf("%s:%s:%u:%u:%s:%s:%s\n",
> pw->pw_name, pw->pw_passwd, pw->pw_uid,
> pw->pw_gid, pw->pw_gecos, pw->pw_dir,
> pw->pw_shell);
> }
> if (errno != 0) return errno;
> return 0;
> }
> galaxy@musl:~/musl-tests $ tail -1 /etc/passwd
> user:x::1000::/home/user/:
> galaxy@musl:~/musl-tests $ ./test-ent | tail -1
> user:x:0:1000::/home/user/:
> galaxy@musl:~/musl-tests $
> ===
>
> It is the same story with any numeric field in both /etc/passwd and
> /etc/group,
> empty fields magically turn into 0, which has a significant meaning on
> Un*x
> systems.
>
> The whole parssing of password and group entires is quite "dumb", in
> terms that
> it always expects perfectly valid input and produces always correct
> output, but
> I would argue that these functions should be a bit smarter and should
> not make
> assumptions about the validity of the input, i.e treat it as untrusted
> user
> input.

There's a reason it's recommended that one only make changes to these
files using tools like the ones from the shadow suite. Things in /etc
can, theoretically, only be written to by root or at least trusted
users, so treating as entirely untrusted seems a bit over the top...

That said, erroring out/skipping such entries sounds reasonable either
way.

>
> Another example, where musl exhibits a different behaviour to Glibc is
> parsing
> of erroneous group entries (below, "group" misses the final ':'):
> ===
> galaxy@musl:~/musl-tests $ cat test-gent.c
> #include <sys/types.h>
> #include <grp.h>
> #include <stdio.h>
> #include <errno.h>
>
> int main() {
> struct group *gr;
> char **p;
> printf("Reading /etc/group:");
> errno = 0;
> while ((gr = getgrent())) {
> printf("%s:%s:%u:",
> gr->gr_name, gr->gr_passwd, gr->gr_gid);
> for (p =gr->gr_mem; *p; p++) {
> printf("%s%c", *p, *(p+1) ? ',' : '\n');
> }
> if (p == gr->gr_mem) printf("\n");
> }
> if (errno != 0) return errno;
> return 0;
> }
> galaxy@musl:~/musl-tests $ tail -2 /etc/group
> galaxy:x:502:group1,group2,group3
> group:x:1000
> galaxy@musl:~/musl-tests $ ./test-gent | tail -2
> users:x:999:
> galaxy:x:502:group1,group2,group3
> galaxy@musl:~/musl-tests $
> ===
>
> Glibc, on the other hand, fixes the not-so-broken record (since it is
> only
> supplemental groups which are missing):
> ===
> [galaxy@archlinux musl-tests]$ tail -2 /etc/group
> pkgbuilder:x:1001:linux-is-fun,musl-rulez
> group:x:1000
> [galaxy@archlinux musl-tests]$ ./test-gent | tail -2
> pkgbuilder:x:1001:linux-is-fun,musl-rulez
> group:x:1000:
> [galaxy@archlinux musl-tests]$
> ===

It seems like both libraries are inconsistent in their own ways. glibc
skips malformed entries when some fields are missing, but fixes a
missing supplemental group entry. musl skips a missing supplemental
group entry, but "fixes" malformed entries with fields missing.

Maybe striving for consistency by either always skipping or always
fixing entries seems like a more reasonable choice to me, maybe?

>
> With put*() functions the situation a bit better, but still could be
> improved to achieve better compatibility. putpwent() will output
> '(null)' for any NULL pointer passed for the string arguments (due to
> direct call to fprintf() and the UB for that situation), while Glibc
> would output an empty string instead.

It would seem the function returns EINVAL instead of outputting anything
at all, from my look at the code. I think that's reasonable behavior for
musl to implement, given how badly specified the function is.

> Moreover, putpwent() is inconsistent with putgrent() -- the latter
> locks the file before writing and unlocks afterwards, while the former
> is just going ahead with fprintf(). I know these funnctions are thread
> unsafe, but this lack of locking makes putpwent() plainly dangerous on
> a multiuser system.

I'm pretty sure these functions are always dangerous on multiuser
systems; flockfile(3) is FILE level locking, it just protects from other
threads touching a given FILE object. If you want to protect yourself
from multiple programs handling the file simultaneously, you need file
locking, such as done with fcntl(2).

Regarding the inconsistency, it seems to have been this way since their
introduction in ddfb267b0e72499f6022981733264a063ec881f0. Thinking about
it more, it's necessary because putgrent(3) needs to print the first
part of the record and then loop through the supplemental group list,
which are multiple separate fprintf calls. Since putpwent(3) is a single
fprintf call, which will lock the FILE object itself, there's no risk of
corrupting the entries.

>
> Rich, would it be acceptable to align musl behaviour with Glibc's in
> this
> regard? I mean, consider missing uid/gid fields to be a critical error,
> so the
> record is dropped, and consider missing supplemental groups (together
> with the
> last separator) to be a minor, fixable error and preserve the record?
>
> This would make (at least my) life a bit easier in porting applications
> to a
> musl-based system, but more imprtantly, it will be less dangerous if an
> erroneous record crawls into one of the identity files.
>
> P.S. I also think that my version (in the example above) of going
> through
> supplemental groups looks nicer than the one in putgrent() :)
>
> --
> (GM)