[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <0100017c6f8d9905-d09f62b3-323a-4133-bcad-e27204cfd62c-000000@email.amazonses.com>
Date: Mon, 11 Oct 2021 13:32:03 +0000
From: "(GalaxyMaster)" <galaxy@...nwall.com.au>
To: musl@...ts.openwall.com
Subject: get/set*ent functions and real world applications
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@...l:~/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@...l:~/musl-tests $ tail -1 /etc/passwd
user:x::1000::/home/user/:
galaxy@...l:~/musl-tests $ ./test-ent | tail -1
user:x:0:1000::/home/user/:
galaxy@...l:~/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.
Another example, where musl exhibits a different behaviour to Glibc is parsing
of erroneous group entries (below, "group" misses the final ':'):
===
galaxy@...l:~/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@...l:~/musl-tests $ tail -2 /etc/group
galaxy:x:502:group1,group2,group3
group:x:1000
galaxy@...l:~/musl-tests $ ./test-gent | tail -2
users:x:999:
galaxy:x:502:group1,group2,group3
galaxy@...l:~/musl-tests $
===
Glibc, on the other hand, fixes the not-so-broken record (since it is only
supplemental groups which are missing):
===
[galaxy@...hlinux musl-tests]$ tail -2 /etc/group
pkgbuilder:x:1001:linux-is-fun,musl-rulez
group:x:1000
[galaxy@...hlinux musl-tests]$ ./test-gent | tail -2
pkgbuilder:x:1001:linux-is-fun,musl-rulez
group:x:1000:
[galaxy@...hlinux musl-tests]$
===
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. 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.
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)
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.
