From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 7721 invoked from network); 11 Oct 2021 17:41:08 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 11 Oct 2021 17:41:08 -0000 Received: (qmail 3103 invoked by uid 550); 11 Oct 2021 17:41:06 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3073 invoked from network); 11 Oct 2021 17:41:06 -0000 X-Virus-Scanned: Debian amavisd-new at disroot.org Mime-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1633974053; bh=jawWVVWa1RpLqNJ3R6bL1K9NBcDMGO2EI3j822/OTGc=; h=To:Subject:From:Date:In-Reply-To; b=bXQ51fM3tAy21eSa/rvR/aUUlNz7RZx0PFrBwbgSPUIqm6rHuoGMyEpJiJe769LU2 4FabK6GfMVyMx6Zw+6xDtCKzPkk0a0lk5hklNoYndEiYy2DNMJeKYW9Tvj7i54NkHe qEQS6Cd1tBjlJHJJxoCYXcMzD3M/QsXfBHRp47OcIZEy+zjSBWPe7FPeNkRmrvJOV+ tXCTp/acxBk3RXVMTfiGKkyTob4BRs5md7eeAZl1gQSCt1QNr0an7hWSsd2Tr6ripA tcy59k4lYOmIJvO5ddi5Bzgi5CSnMV0tRk+T4UMo/ATBwGGuDW883wWoUabm86qBF7 lqNOD5i4bpirQ== Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 To: From: =?utf-8?q?=C3=89rico_Nogueira?= Date: Mon, 11 Oct 2021 14:18:47 -0300 Message-Id: In-Reply-To: <0100017c6f8d9905-d09f62b3-323a-4133-bcad-e27204cfd62c-000000@email.amazonses.com> Subject: Re: [musl] get/set*ent functions and real world applications 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: > =3D=3D=3D > galaxy@musl:~/musl-tests $ cat test-ent.c > #include > #include > #include > #include > > int main() { > struct passwd *pw; > printf("Reading /etc/passwd:\n"); > errno =3D 0; > while ((pw =3D 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 !=3D 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 $ > =3D=3D=3D > > 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 ':'): > =3D=3D=3D > galaxy@musl:~/musl-tests $ cat test-gent.c > #include > #include > #include > #include > > int main() { > struct group *gr; > char **p; > printf("Reading /etc/group:"); > errno =3D 0; > while ((gr =3D getgrent())) { > printf("%s:%s:%u:", > gr->gr_name, gr->gr_passwd, gr->gr_gid); > for (p =3Dgr->gr_mem; *p; p++) { > printf("%s%c", *p, *(p+1) ? ',' : '\n'); > } > if (p =3D=3D gr->gr_mem) printf("\n"); > } > if (errno !=3D 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 $ > =3D=3D=3D > > Glibc, on the other hand, fixes the not-so-broken record (since it is > only > supplemental groups which are missing): > =3D=3D=3D > [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]$ > =3D=3D=3D 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)