[PATCH 3/3] Add _FORTIFY_SOURCE support for inet_ntop

[Frederic Berat]

On Tue, Mar 18, 2025 at 1:08 PM Florian Weimer <fweimer@redhat.com> wrote:

> * Frédéric Bérat:
>
> > - Create the __inet_ntop_chk routine that verifies that the builtin size
> > of the destination buffer is at least as big as the size given by the
> > user.
> > - Redirect calls from inet_ntop to __inet_ntop_chk or __inet_ntop_warn
> > - Update the abilist for this new routine
> > - Update man pages to mention the new fortification
>
> Update [the manual] to mention the new fortification
>
> (Manpages are a separate project.)
>

Noted.


>
> > diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
> > index f8ccc2dff5..128f271c03 100644
> > --- a/debug/tst-fortify.c
> > +++ b/debug/tst-fortify.c
> > @@ -23,6 +23,7 @@
> >
> >  #include <assert.h>
> >  #include <fcntl.h>
> > +#include <arpa/inet.h>
> >  #include <limits.h>
> >  #include <locale.h>
> >  #include <obstack.h>
> > @@ -1832,6 +1833,26 @@ do_test (void)
> >  # endif
> >  #endif
> >
> > +  struct in6_addr addr6;
> > +  struct in_addr addr;
> > +  char addrstr6[INET6_ADDRSTRLEN];
> > +  char addrstr[INET_ADDRSTRLEN];
> > +
> > +  if (inet_ntop (AF_INET6, &addr6, addrstr6, sizeof (addrstr6)) == NULL)
> > +    FAIL ();
> > +  if (inet_ntop (AF_INET, &addr, addrstr, sizeof (addrstr)) == NULL)
> > +    FAIL ();
>
> Doesn't this use uninitialized inputs?  Maybe add an initialization to
> addr, addr6.
>
>
I will.


> > +
> > +#if __USE_FORTIFY_LEVEL >= 1
> > +  CHK_FAIL_START
> > +  inet_ntop (AF_INET6, &addr6, buf, INET6_ADDRSTRLEN);
> > +  CHK_FAIL_END
> > +
> > +  CHK_FAIL_START
> > +  inet_ntop (AF_INET, &addr, buf, INET_ADDRSTRLEN);
> > +  CHK_FAIL_END
> > +#endif
>
> (buf is just 10 bytes, so both INET6_ADDRSTRLEN and INET_ADDRSTRLEN will
> trap.)
>
> > diff --git a/include/arpa/inet.h b/include/arpa/inet.h
> > index f389c28cb2..d8afbf259b 100644
> > --- a/include/arpa/inet.h
> > +++ b/include/arpa/inet.h
> > @@ -3,12 +3,16 @@
> >  #include <inet/arpa/inet.h>
> >
> >  #ifndef _ISOMAC
> > +/* Declare functions with security checks.  */
> > +#include <bits/inet-fortified-decl.h>
>
> Why is this necessary?  Wouldn't this included by <inet/arpa/inet.h>
> above?  Is this to check that the declaration matches the definition?
>
>
The inet-fortified-decl.h is included by inet-fortified.h which itself is
only included if FORITIFY_SOURCE is non-zero while this header needs to be
included unconditionally (otherwise build would fail when fortification is
disabled).


> > diff --git a/inet/arpa/inet.h b/inet/arpa/inet.h
> > index 42d38c330d..d381df7066 100644
> > --- a/inet/arpa/inet.h
> > +++ b/inet/arpa/inet.h
> > @@ -65,7 +65,6 @@ extern const char *inet_ntop (int __af, const void
> *__restrict __cp,
> >                             char *__restrict __buf, socklen_t __len)
> >       __THROW;
> >
> > -
>
> Spurious whitespace change.
>
>
Noted.


> > diff --git a/inet/bits/inet-fortified.h b/inet/bits/inet-fortified.h
> > new file mode 100644
> > index 0000000000..86b6d56d7b
> > --- /dev/null
> > +++ b/inet/bits/inet-fortified.h
>
> > +__fortify_function __attribute_overloadable__ const char *
> > +__NTH (inet_ntop (int __af,
> > +        __fortify_clang_overload_arg (const void *, __restrict, __src),
> > +        char *__restrict __dst, socklen_t __dst_size))
> > +    __fortify_clang_warning_only_if_bos_lt(__dst_size, __dst,
> > +                                        "inet_ntop called with bigger
> length "
> > +                                        "than size of destination
> buffer")
> > +{
> > +  return __glibc_fortify (inet_ntop, __dst_size, sizeof(char),
> > +                       __glibc_objsize(__dst),
> > +                       __af, __src, __dst, __dst_size);
>
> Missing whitespace before various '('.
>

Noted.


>
> Thanks,
> Florian
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250318/8c94148a/attachment.htm>