[Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[GM.Ijewski]

   Now it calls CryptGenRandom() if is it compiled for windows.

   It might be possible to save the cryptographic provider in between
   invocations, e.g. by making it static -- I have no idea how
   computationally
   intensive that operation actually is.

   Signed-off-by: Geert Martin Ijewski <gm.ijewski@web.de>

   diff --git a/crypto/random-platform.c b/crypto/random-platform.c
   index 82b755a..7aa0476 100644
   --- a/crypto/random-platform.c
   +++ b/crypto/random-platform.c
   @@ -26,6 +26,7 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
                             size_t buflen G_GNUC_UNUSED,
                             Error **errp)
    {
   +#ifndef _WIN32
        int fd;
        int ret = -1;
        int got;
   @@ -61,4 +62,26 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
     cleanup:
        close(fd);
        return ret;
   +#else
   +    HCRYPTPROV   hCryptProv;
   +
   +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
   0)) {
   +        if (NTE_BAD_KEYSET == GetLastError()) {
   +            if (!CryptAcquireContext(&hCryptProv, NULL, NULL,
   +                                     PROV_RSA_FULL, CRYPT_NEWKEYSET))
   {
   +                error_setg_errno(errp, GetLastError(),
   +                         "Unable to create cryptographic provider");
   +            }
   +        }
   +    }
   +
   +    if (!CryptGenRandom(hCryptProv, buflen, buf)) {
   +        error_setg_errno(errp, GetLastError(),
   +                         "Unable to read random bytes");
   +        return -1;
   +    }
   +
   +    CryptReleaseContext(hCryptProv, 0);
   +    return 0;
   +#endif
    }
   diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
   index ff18b23..4a5d908 100644
   --- a/include/sysemu/os-win32.h
   +++ b/include/sysemu/os-win32.h
   @@ -29,6 +29,7 @@
    #include <winsock2.h>
    #include <windows.h>
    #include <ws2tcpip.h>
   +#include <Wincrypt.h>

    #if defined(_WIN64)
    /* On w64, setjmp is implemented by _setjmp which needs a second
   parameter.

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

For subject line, better to describe the change made, rather than
the problem.

On Mon, Apr 24, 2017 at 02:17:56PM +0200, GM.Ijewski@web.de wrote:
> Now it calls CryptGenRandom() if is it compiled for windows.
>  
> It might be possible to save the cryptographic provider in between
> invocations, e.g. by making it static -- I have no idea how computationally
> intensive that operation actually is.

I'd think most people should really just enable gnutls during build. This just
has to provide a fallback that's good enough to be functional. If someone really
cares about performance of this fallback, they can send patches later....

>
> Signed-off-by: Geert Martin Ijewski <gm.ijewski@web.de>
>  
> diff --git a/crypto/random-platform.c b/crypto/random-platform.c
> index 82b755a..7aa0476 100644
> --- a/crypto/random-platform.c
> +++ b/crypto/random-platform.c
> @@ -26,6 +26,7 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
>                           size_t buflen G_GNUC_UNUSED,
>                           Error **errp)
>  {
> +#ifndef _WIN32
>      int fd;
>      int ret = -1;
>      int got;
> @@ -61,4 +62,26 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
>   cleanup:
>      close(fd);
>      return ret;
> +#else
> +    HCRYPTPROV   hCryptProv;
> +
> +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL, 0)) {
> +        if (NTE_BAD_KEYSET == GetLastError()) {
> +            if (!CryptAcquireContext(&hCryptProv, NULL, NULL,
> +                                     PROV_RSA_FULL, CRYPT_NEWKEYSET)) {
> +                error_setg_errno(errp, GetLastError(),
> +                         "Unable to create cryptographic provider");

You forgot to "return -1' here

> +            }
> +        }

You need to have an 'else' branch here that reports an error too,
in cae the first CryptAcquireContext returns err != NTE_BSD_KEYSET.

> +    }
> +
> +    if (!CryptGenRandom(hCryptProv, buflen, buf)) {
> +        error_setg_errno(errp, GetLastError(),
> +                         "Unable to read random bytes");
> +        return -1;
> +    }
> +
> +    CryptReleaseContext(hCryptProv, 0);
> +    return 0;
> +#endif
>  }
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index ff18b23..4a5d908 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -29,6 +29,7 @@
>  #include <winsock2.h>
>  #include <windows.h>
>  #include <ws2tcpip.h>
> +#include <Wincrypt.h>

It would be preferrable to put this in random-platform.c to avoid
polluting the global namespace

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Peter Maydell]

On 24 April 2017 at 13:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> For subject line, better to describe the change made, rather than
> the problem.
>
> On Mon, Apr 24, 2017 at 02:17:56PM +0200, GM.Ijewski@web.de wrote:
>> Now it calls CryptGenRandom() if is it compiled for windows.
>>
>> It might be possible to save the cryptographic provider in between
>> invocations, e.g. by making it static -- I have no idea how computationally
>> intensive that operation actually is.
>
> I'd think most people should really just enable gnutls during build. This just
> has to provide a fallback that's good enough to be functional. If someone really
> cares about performance of this fallback, they can send patches later....

What I think is more worrying is the suggestion in the CryptAcquireContext
docs that this might for instance prompt the user for a password to
decrypt keys. We definitely don't want to do that every time. In
fact we'd rather not do it at all.

Googling suggests that the approach used by web browsers is
to use the not-very-documented RtlGenRandom function:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/win_rand.c#141
https://chromium.googlesource.com/chromium/src.git/+/master/base/rand_util_win.cc

That looks much simpler than trying to use the official crypto
APIs. (It's also what the MS C runtime library does to implement
rand_s().)

thanks
-- PMM

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 02:30:25PM +0100, Peter Maydell wrote:
> On 24 April 2017 at 13:50, Daniel P. Berrange <berrange@redhat.com> wrote:
> > For subject line, better to describe the change made, rather than
> > the problem.
> >
> > On Mon, Apr 24, 2017 at 02:17:56PM +0200, GM.Ijewski@web.de wrote:
> >> Now it calls CryptGenRandom() if is it compiled for windows.
> >>
> >> It might be possible to save the cryptographic provider in between
> >> invocations, e.g. by making it static -- I have no idea how computationally
> >> intensive that operation actually is.
> >
> > I'd think most people should really just enable gnutls during build. This just
> > has to provide a fallback that's good enough to be functional. If someone really
> > cares about performance of this fallback, they can send patches later....
> 
> What I think is more worrying is the suggestion in the CryptAcquireContext
> docs that this might for instance prompt the user for a password to
> decrypt keys. We definitely don't want to do that every time. In
> fact we'd rather not do it at all.
> 
> Googling suggests that the approach used by web browsers is
> to use the not-very-documented RtlGenRandom function:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/win_rand.c#141
> https://chromium.googlesource.com/chromium/src.git/+/master/base/rand_util_win.cc
> 
> That looks much simpler than trying to use the official crypto
> APIs. (It's also what the MS C runtime library does to implement
> rand_s().)

FYI, both gnutls and openssl use these CryptAcquireContext/CryptGenRandom
methods, so I'd prefer to stick with that.

It seems we merely need to set CRYPT_SILENT in the flags to prevent any
chance of interactive prompts.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa379886(v=vs.85).aspx

"CRYPT_SILENT is intended for use with applications for which the UI
 cannot be displayed by the CSP."

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Peter Maydell]

On 24 April 2017 at 14:36, Daniel P. Berrange <berrange@redhat.com> wrote:
> FYI, both gnutls and openssl use these CryptAcquireContext/CryptGenRandom
> methods, so I'd prefer to stick with that.

They probably need the full crypto API anyway, though...

> It seems we merely need to set CRYPT_SILENT in the flags to prevent any
> chance of interactive prompts.
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa379886(v=vs.85).aspx

How about CRYPT_VERIFYCONTEXT? The docs say "in most cases this flag
should be set".

This kind of discussion puts me off the Crypt* APIs though -- they're
a complicated API that can easily be misused. "Please just fill
this buffer with randomness" is a simple API that's hard to call
wrongly...

thanks
-- PMM

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 02:52:30PM +0100, Peter Maydell wrote:
> On 24 April 2017 at 14:36, Daniel P. Berrange <berrange@redhat.com> wrote:
> > FYI, both gnutls and openssl use these CryptAcquireContext/CryptGenRandom
> > methods, so I'd prefer to stick with that.
> 
> They probably need the full crypto API anyway, though...
> 
> > It seems we merely need to set CRYPT_SILENT in the flags to prevent any
> > chance of interactive prompts.
> >
> > https://msdn.microsoft.com/en-us/library/windows/desktop/aa379886(v=vs.85).aspx
> 
> How about CRYPT_VERIFYCONTEXT? The docs say "in most cases this flag
> should be set".
> 
> This kind of discussion puts me off the Crypt* APIs though -- they're
> a complicated API that can easily be misused. "Please just fill
> this buffer with randomness" is a simple API that's hard to call
> wrongly...

This is the extent of gnutls's code in this area

https://gitlab.com/gnutls/gnutls/blob/master/lib/nettle/sysrng-windows.c

Our API has the same usage scenario as this, hence my preference to mirror
what gnutls & other crypto libraries are using.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Peter Maydell]

On 24 April 2017 at 14:57, Daniel P. Berrange <berrange@redhat.com> wrote:
> This is the extent of gnutls's code in this area
>
> https://gitlab.com/gnutls/gnutls/blob/master/lib/nettle/sysrng-windows.c
>
> Our API has the same usage scenario as this, hence my preference to mirror
> what gnutls & other crypto libraries are using.

I see that only calls CryptAcquireContext once, not twice.
I also think we should do what that code does and use a static
variable to avoid calling CryptAcquireContext repeatedly.

If we want to follow gnutls we should just borrow that code
(tweaking the function names etc as appropriate) and credit it:
gnutls is LGPL2.1 so no problem doing that.

thanks
-- PMM

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 03:05:40PM +0100, Peter Maydell wrote:
> On 24 April 2017 at 14:57, Daniel P. Berrange <berrange@redhat.com> wrote:
> > This is the extent of gnutls's code in this area
> >
> > https://gitlab.com/gnutls/gnutls/blob/master/lib/nettle/sysrng-windows.c
> >
> > Our API has the same usage scenario as this, hence my preference to mirror
> > what gnutls & other crypto libraries are using.
> 
> I see that only calls CryptAcquireContext once, not twice.
> I also think we should do what that code does and use a static
> variable to avoid calling CryptAcquireContext repeatedly.

Ok, fair enough.

We can have the existing qcrypto_init() call a qcrypto_random_init()
method to do the one-time initialization task, since that's already
required to run early in order to initialize gnutls when we use it. 

> If we want to follow gnutls we should just borrow that code
> (tweaking the function names etc as appropriate) and credit it:
> gnutls is LGPL2.1 so no problem doing that.

Yep

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Peter Maydell]

On 24 April 2017 at 16:41, Daniel P. Berrange <berrange@redhat.com> wrote:
> We can have the existing qcrypto_init() call a qcrypto_random_init()
> method to do the one-time initialization task, since that's already
> required to run early in order to initialize gnutls when we use it.

Yep, that would work, or initialize-on-first-call.

thanks
-- PMM

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 04:42:38PM +0100, Peter Maydell wrote:
> On 24 April 2017 at 16:41, Daniel P. Berrange <berrange@redhat.com> wrote:
> > We can have the existing qcrypto_init() call a qcrypto_random_init()
> > method to do the one-time initialization task, since that's already
> > required to run early in order to initialize gnutls when we use it.
> 
> Yep, that would work, or initialize-on-first-call.

I avoided suggesting that, to avoid having to worry about thread-safe
initialization :-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Geert Martin Ijewski]

 > We can have the existing qcrypto_init() call a qcrypto_random_init()
 > method to do the one-time initialization task, since that's already
 > required to run early in order to initialize gnutls when we use it.

Wouldn't it make sense to also move the unix initalization to that function?

And what about deinitalization? Though because that handle needs to stay 
valid throughout the whole lifetime of QEMU anyway, that probably can be 
ignored and taken care of by the OS

Regards,
Geert

Re: [Qemu-devel] error: qcrypto_random_bytes() tried to read from /dev/[u]random, even on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 06:33:26PM +0200, Geert Martin Ijewski wrote:
> > We can have the existing qcrypto_init() call a qcrypto_random_init()
> > method to do the one-time initialization task, since that's already
> > required to run early in order to initialize gnutls when we use it.
> 
> Wouldn't it make sense to also move the unix initalization to that function?

Yep, we can do that.

> And what about deinitalization? Though because that handle needs to stay
> valid throughout the whole lifetime of QEMU anyway, that probably can be
> ignored and taken care of by the OS

Correct, there's no need for any de-init if we have a global handle, as it
is needed until QEMU exits.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|