[PATCH libXdmcp] Use getrandom() syscall if available

[Benjamin Tissoires]

On Mon, Apr 3, 2017 at 9:17 PM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> OpenBSD does not have a getrandom(3) function in libc.  The OpenBSD
> postion is that the arc4random(3) family of functions is always the
> preferred way to get proper random numbers as all the other interfaces
> have nasty corner cases that people shouldn't have to deal with.  In
> your case for example, getrandom(2) may block and therefore could
> return EINTR, a case which your patch doesn't handle.

I see. So if arc4random_buf(3) is available, it should always be
preferred. When it is not available, I guess I should go for
getrandom/getentropy and probably don't care about the syscall at all.

>
> So I'd argue that from a Linux distro point of view, making sure that
> a proper arc4random_buf(3) implementation is available would be the
> best way to fix this vulnerability.  Either by improving the version
> provided in libbsd (feel free to take the code from libressl) or by
> lobbying for its inclusion in glibc.

TBH, the issue with the libbsd implementation is that it's not shipped
with RHEL (where I try to provide a fix). For various reasons we do
not ship libbsd and adding it there only for one PRNG is something
that will be costly in term of maintenance.
I don't think we ship libressl either, but the problem is not that
much with RHEL (we can ship custom fixes if required), but more the
fact that there is no true fix besides "use libbsd".

Which is why I wanted to provide an alternative for Linux that we can
point customers at.

Regarding adding arc4random_buf(3) in glibc, I am not sure I
personally want to go this path. It took a little bit more than 2
years to have getrandom() in glibc, when it was pushed by far more
knowledgeable people than me :(


>  Adding more #ifdef spaghetti
> everywhere isn't going to help.

True. But the current situation where the code might be vulnerable or
not is not a good situation either. Having a CVE out there without a
fix is not good. Users will want it to be fixed and we can't guarantee
that the code they are running is safe. I am not pointing fingers at
anybody (I was not in the security discussion), I am just trying to
justify the fact that we still need a patch that we can point people
at.

How about (in pseudo-c):

---
#ifndef HAVE_ARC4RANDOM_BUF
void arc4random_buf(char *auth, int len)
{
#if HAVE_GET_ENTROPY
    getentropy(auth, len);
    if (failed)
#endif
    {
      /* use the older, unsafe code */
    }
}
#endif

void
XdmcpGenerateKey (XdmAuthKeyPtr key)
{
    arc4random_buf(key->data, 8)
}

---

This should make the code cleaner, remove a little bit of spaghetti,
and will allow for future libc to implement arc4random_buf in the
missing architectures.

Cheers,
Benjamin

>
> Cheers,
>
> Mark
>
>> >> diff --git a/configure.ac b/configure.ac
>> >> index 2288502..d0d4d05 100644
>> >> --- a/configure.ac
>> >> +++ b/configure.ac
>> >> @@ -63,6 +63,9 @@ case $host_os in
>> >>          ;;
>> >>  esac
>> >>
>> >> +# Checks for syscalls
>> >> +AC_CHECK_DECLS([SYS_getrandom], [], [], [[#include <sys/syscall.h>]])
>> >> +
>> >>  # Checks for library functions.
>> >>  AC_CHECK_LIB([bsd], [arc4random_buf])
>> >>  AC_CHECK_FUNCS([srand48 lrand48 arc4random_buf])
>> >>
>> >
>> > Could you move that check up into the case $host_os section above it
>> > under a new case for *linux* perhaps?
>>
>> Sure.
>>
>> Cheers,
>> Benjamin
>> _______________________________________________
>> xorg-devel at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>
>>