mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] get/set*ent functions and real world applications
@ 2021-10-11 13:32 (GalaxyMaster)
  2021-10-11 17:18 ` Érico Nogueira
  0 siblings, 1 reply; 7+ messages in thread
From: (GalaxyMaster) @ 2021-10-11 13:32 UTC (permalink / raw)
  To: musl

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.

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]$
===

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)

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-13 13:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 13:32 [musl] get/set*ent functions and real world applications (GalaxyMaster)
2021-10-11 17:18 ` Érico Nogueira
2021-10-12  0:38   ` (GalaxyMaster)
2021-10-12 15:58     ` Érico Nogueira
2021-10-12 17:38       ` Érico Nogueira
2021-10-13  6:16   ` A. Wilcox
2021-10-13 13:56     ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).