[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Bálint,

On 3/10/23 21:34, Bálint Réczey wrote:
[...]

>> 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,

It's not just setting '\0', but also checking truncation.  As I
said, strncpy(3) is not suited for that, but memcpy(3) could be
used.  However, using memcpy(3) for copying strings with truncation
and detecting truncation is:

memcpy(dst, src, sizeof(dst) - 1)
if (strlen(src) >= sizeof(dst))
goto trunc;
dst[sizeof(dst) - 1] = '\0';

There are a few things I don't like:

-  setting '\0' is in a separate line.  Just a minor thing.
-  Two '-1', which are likely to produce off-by-one errors
   at some point (I've already fixed a few of them, IIRC).  At
   least they didn't seem bad, since we had then on the good
   side (we were just wasting one byte).

But the behavior is indeed what we want.  Here's the definition of
stpecpy(), which basically does that (I call strnlen(3) for optimizing):

$ grepc -tfd stpecpy
./lib/stpecpy.h:67:
inline char *
stpecpy(char *dst, char *end, const char *restrict src)
{
booltrunc;
char*p;
size_t  dsize, dlen, slen;

if (dst == end)
return end;
if (dst == NULL)
return NULL;

dsize = end - dst;
slen = strnlen(src, dsize);
trunc = (slen == dsize);
dlen = slen - trunc;

p = mempcpy(dst, src, dlen);
*p = '\0';

return p + trunc;
}


> but I'd be OK
> with any implementation that's correct and uses only glibc symbols
> including strcpy() and memcpy().

Okay, stpecpy() would be enough.

>> 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

I'm not a fan either; rather the opposite.  I'd vote against doing so.

> 
> readpassphrase.c's ISC license allows that, too, and I think copying
> would not be a ton of work.

Copying it, probably not.  Maintaining it, maybe yes.  I mean, just look
at it:

$ grepc -tfd readpassphrase | wc -l
142


142 lines of a function definition are not something I'd consider easy to
maintain.  Is it a big deal to add another dependency?  I'd say it's a
bigger deal to copy verbatim so many lines of code, and sync them from
time to time from libbsd (or OpenBSD) just to bring in any bugfixes they
apply.  That's exactly the purpose of libbsd, so I think relying on them
should be fine.

Cheers,

Alex
-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Bálint Réczey]

Hi Alejandro,

Alejandro Colomar  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  ezt írta (időpont: 2023.
> > márc. 5., V, 20:44):
> >>
> >> Package: passwd
> >> Source: shadow
> >> Tags: patch
> >> X-Debbugs-CC: Bálint Réczey 
> >> X-Debbugs-CC: Iker Pedrosa 
> >> X-Debbugs-CC: Serge Hallyn 
> >>
> >> These dependencies were added upstream recently.
> >>
> >> Signed-off-by: Alejandro Colomar 
> >> Cc: Iker Pedrosa 
> >> Cc: Serge Hallyn 
> >> ---
> >>  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:
>
> 
> 
>
> 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)