Hi Bálint, [I reordered some quotes for my reply] [CC Paul, since he's been mentioned, and I'm curious to know if he has any comments]
On 3/8/23 11:59, Bálint Réczey wrote: > Hi Alejandro, > > Alejandro Colomar <alx.manpa...@gmail.com> ezt írta (időpont: 2023. > márc. 5., V, 20:44): >> >> Package: passwd >> Source: shadow >> Tags: patch >> X-Debbugs-CC: Bálint Réczey <bal...@balintreczey.hu> >> X-Debbugs-CC: Iker Pedrosa <ipedr...@redhat.com> >> X-Debbugs-CC: Serge Hallyn <se...@hallyn.com> >> >> These dependencies were added upstream recently. >> >> Signed-off-by: Alejandro Colomar <a...@kernel.org> >> Cc: Iker Pedrosa <ipedr...@redhat.com> >> Cc: Serge Hallyn <se...@hallyn.com> >> --- >> debian/control | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/debian/control b/debian/control >> index 3cc66f8d..75015c35 100644 >> --- a/debian/control >> +++ b/debian/control >> @@ -11,11 +11,13 @@ Build-Depends: bison, >> gettext, >> itstool, >> libaudit-dev [linux-any], >> + libbsd-dev, > > I checked out recent changes in shadow's master and I'm very happy > about many of the fixes for memory allocation problems, Thanks! :-) > but wearing my maintainer hat I believe linking to a new library for a > few functions which are not very different from their glibc > counterpart is not worth it. We added it with strlcpy(3) in mind, but I agree with you that it's not a critical reason, and we could live without it; in fact I introduced a similar (and IMO superior) function, stpecpy(), which could replace strlcpy(3) in all 6 calls. But we didn't really add it for it; we already had the libbsd-dev dependency before adding strlcpy(3). libbsd-dev was added for readpassphrase(3bsd), which has nothing similar in glibc, and I don't want to be rewriting it in terms of glibc stuff, since it's not trivial. It was added in this patch set: * 2a5b8810 - Mon, 21 Nov 2022 14:00:13 +0100 (4 months ago) | agetpass: Hook into build-system - Guillem Jover * ab91ec10 - Wed, 28 Sep 2022 23:09:19 +0200 (5 months ago) | Hide [[gnu::malloc(deallocator)]] in a macro - Alejandro Colomar * 554f86ba - Tue, 27 Sep 2022 21:21:35 +0200 (5 months ago) | Replace the deprecated getpass(3) by our agetpass() - Alejandro Colomar * 155c9421 - Mon, 26 Sep 2022 22:22:24 +0200 (5 months ago) | libmisc: agetpass(), erase_pass(): Add functions for getting passwords safely - Alex Colomar * 8cce4557 - Wed, 28 Sep 2022 00:03:46 +0200 (5 months ago) | Don't 'else' after a 'noreturn' call - Alex Colomar * 99ce21a3 - Tue, 22 Nov 2022 14:35:06 +0100 (4 months ago) | CI: add libbsd and pkg-config dependencies - Iker Pedrosa > > Freezero() also provides little extra benefit over memset() and free() > and is used only 4 times in the code. Use of freezero(3bsd) was added later to erase_pass() for one reason: that API pair --agetpass(), erase_pass()-- already strongly depends on a libbsd-dev function --readpassphrase(3bsd)--, so depending on two of them doesn't add any issues. Anyway, freezero(3) is easy to implement if we had a need. > There are reasons for strlcpy() not being provided by glibc [1]: > > "Reactions among core glibc contributors on the topic of including > strlcpy() and strlcat() have been varied over the years. Christoph > Hellwig's early patch was rejected in the then-primary maintainer's > inimitable style (1 and 2). But reactions from other glibc developers > have been more nuanced, indicating, for example, some willingness to > accept the functions. Perhaps most insightfully, Paul Eggert notes > that even when these functions are provided (as an add-on packaged > with the application), projects such as OpenSSH, where security is of > paramount concern, still manage to either misuse the functions > (silently truncating data) or use them unnecessarily (i.e., the > traditional strcpy() and strcat() could equally have been used without > harm); such a state of affairs does not constitute a strong argument > for including the functions in glibc. " > > I agree with their position and the 6 cases where strlcpy() is used in > shadow's current master could be implemented with strncpy() as safely > as with strlcpy(). I would strongly advise against that. strncpy(3) doesn't produce strings. See the following manual pages: <https://mirrors.edge.kernel.org/pub/linux/docs/man-pages/book/man-pages-6.03.pdf#strncpy_3> <https://mirrors.edge.kernel.org/pub/linux/docs/man-pages/book/man-pages-6.03.pdf#string_copying_7> My main concerns with strncpy(3) are: - It zeroes the rest of the buffer, which isn't bad per se, but then when reading code it's hard to tell if that was necessary or if it was just some inessential side effect of calling strncpy(3). Which was exactly what happened when I did the migration from strncpy(3) to strlcpy(3): I had a hard time telling for sure at every call site if I could do it, or if I was doing something wrong by removing that zeroing. - It doesn't terminate the string, so you still need to manually write a '\0' after the call. At this point, and assuming most calls don't need zeroing, it would be simpler to just call memcpy(3) --which BTW was Ulrich's recommendation in the old glibc discussions you mentioned--. Remember that, as I wrote in the strncpy(3) manual page recently, strncpy(3) is just: char * strncpy(char *restrict dst, const char *restrict src, size_t sz) { bzero(dst, sz); memcpy(dst, src, strnlen(src, sz)); return dst; } Which shows that strncpy(3) is really worse than strcpy(3), as Eggert would probably say, in the same sense that a misused strlcpy(3) is worse than strcpy(3). strncpy(3) silently truncates the string in the call to memcpy(3), so you avoid overrunning the dst buffer by transforming the bug into a silent truncation, which can be similarly bad. Of course, I can't forget that our 6 uses of strlcpy(3) are still possibly-bogus in that sense: they don't check truncation. $ grep -rn strlcpy src/su.c:661: strlcpy (name, tmp_name, sizeof(name)); lib/gshadow.c:131: strlcpy (sgrbuf, string, len); lib/fields.c:103: strlcpy (buf, cp, maxsize); libmisc/console.c:47: strlcpy (buf, cons, sizeof (buf)); libmisc/utmp.c:46: (void) strlcpy (tmptty, tname, sizeof tmptty); libmisc/date_to_str.c:42: (void) strlcpy (buf, "never", size); I didn't have the time to look into that, but we should really check if we need to add some error checking. With strlcpy(3), at least we can do it, contrary to strncpy(3), which doesn't really help detecting truncation (except that you can check the last byte _before_ overwriting it with the '\0', which is really cumbersome). BTW, since POSIX will be adding strlcpy(3) soon, glibc will also add it soon (maybe Paul can update us on that). > > Could you please return to using functions provided by glibc instead > of pulling in libbsd in the next upstream release? We would rather replace strlcpy(3) with stpecpy(), which also copies a string with truncation, and has a better interface (carries truncation detection over chained calls; don't need to recalculate the remaining size after each call --nor use strlcat(3)--). We could also easily replace freezero(3bsd). reallocf(3bsd) would also be easy to reimplement as reallocarrayf(p, 1, size), which we already implement in <./lib/alloc.h>. But we can't trivially replace readpassphrase(3bsd). We could try to reimplement it ourselves, but I don't see avoiding libbsd-dev as a strong-enough reason. > That way there would be no need for pkg-config or pkgconf either. > > Cheers, > Balint > > [1] https://lwn.net/Articles/507319/ Cheers, Alex -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
OpenPGP_signature
Description: OpenPGP digital signature
