Hi Alejandro, Alejandro Colomar <alx.manpa...@gmail.com> ezt írta (időpont: 2023. márc. 8., Sze, 13:55): > > 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).
I did not find setting the last '\0' that cumbersome, but I'd be OK with any implementation that's correct and uses only glibc symbols including strcpy() and memcpy(). > BTW, since POSIX will be adding strlcpy(3) soon, glibc will also > add it soon (maybe Paul can update us on that). Then it will be OK to use strlcpy() as well, of course. > > > > 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. Indeed, readpassphrase() is the most problematic, but looking at its implementation in libbsd it could be just copied to shadow. I'm not a fan of such copies, but it seems this function has been copied extensively already: https://codesearch.debian.net/search?q=readpassphrase%28const+char&literal=1 readpassphrase.c's ISC license allows that, too, and I think copying would not be a ton of work. Cheers, Balint > > 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
