[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config
Alejandro Colomar
alx.manpages at gmail.com
Wed Mar 8 12:55:15 GMT 2023
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.manpages at 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 <balint at balintreczey.hu>
>> X-Debbugs-CC: Iker Pedrosa <ipedrosa at redhat.com>
>> X-Debbugs-CC: Serge Hallyn <serge at hallyn.com>
>>
>> These dependencies were added upstream recently.
>>
>> Signed-off-by: Alejandro Colomar <alx at kernel.org>
>> Cc: Iker Pedrosa <ipedrosa at redhat.com>
>> Cc: Serge Hallyn <serge at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-shadow-devel/attachments/20230308/074d046d/attachment.sig>
More information about the Pkg-shadow-devel
mailing list