[Qemu-devel] [PATCH v2] crypto_gen_random() now also works on windows

[Qemu-devel] [PATCH v2] crypto_gen_random() now also works on windows

[Geert Martin Ijewski]

If no crypto library is included in the build QEMU uses 
qcrypto_random_bytes() to generate random data. That function tried to 
open /dev/urandom or /dev/random and if openeing neither file worked it 
errored out.

Those files obviously do not exist on windows, so there the code now 
uses CryptGenRandom().

Furthermore there was some refactoring and a new function 
qcrypto_random_init() was introduced, that initalizes (platform 
specific) handles that are used by qcrypto_random_bytes().

Signed-off-by: Geert Martin Ijewski <gm.ijewski@web.de>
---
  crypto/init.c            |  6 ++++++
  crypto/random-platform.c | 45 
+++++++++++++++++++++++++++++++++++++--------
  include/crypto/random.h  |  9 +++++++++
  3 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/crypto/init.c b/crypto/init.c
index f65207e..f131c42
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -32,6 +32,8 @@
  #include <gcrypt.h>
  #endif

+#include "crypto/random.h"
+
  /* #define DEBUG_GNUTLS */

  /*
@@ -146,5 +148,9 @@ int qcrypto_init(Error **errp)
      gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0);
  #endif

+    if (qcrypto_random_init(errp) < 0) {
+        return -1;
+    }
+
      return 0;
  }
diff --git a/crypto/random-platform.c b/crypto/random-platform.c
index 82b755a..49d7f80
@@ -22,14 +22,23 @@

  #include "crypto/random.h"

-int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
-                         size_t buflen G_GNUC_UNUSED,
-                         Error **errp)
-{
-    int fd;
-    int ret = -1;
-    int got;
+#ifdef _WIN32
+#include <Wincrypt.h>
+HCRYPTPROV hCryptProv;
+#else
+int fd; /* a file handle to either /dev/urandom or /dev/random */
+#endif

+int qcrypto_random_init(Error **errp)
+{
+#ifdef _WIN32
+    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
+                             CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) {
+        error_setg_errno(errp, GetLastError(),
+                         "Unable to create cryptographic provider");
+        return -1;
+    }
+#else
      /* TBD perhaps also add support for BSD getentropy / Linux
       * getrandom syscalls directly */
      fd = open("/dev/urandom", O_RDONLY);
@@ -41,6 +50,18 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
          error_setg(errp, "No /dev/urandom or /dev/random found");
          return -1;
      }
+#endif
+
+    return 0;
+}
+
+int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
+                         size_t buflen G_GNUC_UNUSED,
+                         Error **errp)
+{
+#ifndef _WIN32
+    int ret = -1;
+    int got;

      while (buflen > 0) {
          got = read(fd, buf, buflen);
@@ -59,6 +80,14 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,

      ret = 0;
   cleanup:
-    close(fd);
      return ret;
+#else
+    if (!CryptGenRandom(hCryptProv, buflen, buf)) {
+        error_setg_errno(errp, GetLastError(),
+                         "Unable to read random bytes");
+        return -1;
+    }
+
+    return 0;
+#endif
  }
diff --git a/include/crypto/random.h b/include/crypto/random.h
index a101353..82a3209
--- a/include/crypto/random.h
+++ b/include/crypto/random.h
@@ -40,5 +40,14 @@ int qcrypto_random_bytes(uint8_t *buf,
                           size_t buflen,
                           Error **errp);

+/**
+ * qcrypto_random_init:
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Initalizes the handles used by qcrypto_random_bytes
+ *
+ * Returns 0 on success, -1 on error
+ */
+int qcrypto_random_init(Error **errp);

  #endif /* QCRYPTO_RANDOM_H */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] crypto_gen_random() now also works on windows

[Daniel P. Berrange]

On Mon, Apr 24, 2017 at 07:51:49PM +0200, Geert Martin Ijewski wrote:
> If no crypto library is included in the build QEMU uses
> qcrypto_random_bytes() to generate random data. That function tried to open
> /dev/urandom or /dev/random and if openeing neither file worked it errored
> out.
> 
> Those files obviously do not exist on windows, so there the code now uses
> CryptGenRandom().
> 
> Furthermore there was some refactoring and a new function
> qcrypto_random_init() was introduced, that initalizes (platform specific)
> handles that are used by qcrypto_random_bytes().
> 
> Signed-off-by: Geert Martin Ijewski <gm.ijewski@web.de>
> ---
>  crypto/init.c            |  6 ++++++
>  crypto/random-platform.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  include/crypto/random.h  |  9 +++++++++
>  3 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/init.c b/crypto/init.c
> index f65207e..f131c42
> --- a/crypto/init.c
> +++ b/crypto/init.c
> @@ -32,6 +32,8 @@
>  #include <gcrypt.h>
>  #endif
> 
> +#include "crypto/random.h"
> +
>  /* #define DEBUG_GNUTLS */
> 
>  /*
> @@ -146,5 +148,9 @@ int qcrypto_init(Error **errp)
>      gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0);
>  #endif
> 
> +    if (qcrypto_random_init(errp) < 0) {
> +        return -1;
> +    }
> +
>      return 0;
>  }
> diff --git a/crypto/random-platform.c b/crypto/random-platform.c
> index 82b755a..49d7f80
> @@ -22,14 +22,23 @@
> 
>  #include "crypto/random.h"
> 
> -int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
> -                         size_t buflen G_GNUC_UNUSED,
> -                         Error **errp)
> -{
> -    int fd;
> -    int ret = -1;
> -    int got;
> +#ifdef _WIN32
> +#include <Wincrypt.h>
> +HCRYPTPROV hCryptProv;
> +#else
> +int fd; /* a file handle to either /dev/urandom or /dev/random */
> +#endif


Lets mark both these vars 'static'


> 
> +int qcrypto_random_init(Error **errp)
> +{
> +#ifdef _WIN32
> +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
> +                             CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) {
> +        error_setg_errno(errp, GetLastError(),
> +                         "Unable to create cryptographic provider");

Unfortunately the return value of 'GetLastError()' isn't an errno
so we can't use error_setg_errno here.

Just use error_setg, and report the error value with a '(code=%u)'
substitution at the end of the error message.


> +        return -1;
> +    }
> +#else
>      /* TBD perhaps also add support for BSD getentropy / Linux
>       * getrandom syscalls directly */
>      fd = open("/dev/urandom", O_RDONLY);
> @@ -41,6 +50,18 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
>          error_setg(errp, "No /dev/urandom or /dev/random found");
>          return -1;
>      }
> +#endif
> +
> +    return 0;
> +}
> +
> +int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
> +                         size_t buflen G_GNUC_UNUSED,
> +                         Error **errp)
> +{
> +#ifndef _WIN32
> +    int ret = -1;
> +    int got;
> 
>      while (buflen > 0) {
>          got = read(fd, buf, buflen);
> @@ -59,6 +80,14 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED,
> 
>      ret = 0;
>   cleanup:
> -    close(fd);
>      return ret;
> +#else
> +    if (!CryptGenRandom(hCryptProv, buflen, buf)) {
> +        error_setg_errno(errp, GetLastError(),
> +                         "Unable to read random bytes");
> +        return -1;
> +    }
> +
> +    return 0;
> +#endif
>  }
> diff --git a/include/crypto/random.h b/include/crypto/random.h
> index a101353..82a3209
> --- a/include/crypto/random.h
> +++ b/include/crypto/random.h
> @@ -40,5 +40,14 @@ int qcrypto_random_bytes(uint8_t *buf,
>                           size_t buflen,
>                           Error **errp);
> 
> +/**
> + * qcrypto_random_init:
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Initalizes the handles used by qcrypto_random_bytes
> + *
> + * Returns 0 on success, -1 on error
> + */
> +int qcrypto_random_init(Error **errp);

We need to add a stub

  int qcrypto_random_init(Error **errp G_GNUC_UNUSED) { return 0; }

in the random-gcrypt.c and random-gnutls.c files

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] [PATCH v2] crypto_gen_random() now also works on windows

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On 04/25/2017 08:42 AM, Daniel P. Berrange wrote:
> On Mon, Apr 24, 2017 at 07:51:49PM +0200, Geert Martin Ijewski wrote:
>> If no crypto library is included in the build QEMU uses
>> qcrypto_random_bytes() to generate random data. That function tried to open
>> /dev/urandom or /dev/random and if openeing neither file worked it errored
>> out.
>>

>> +int qcrypto_random_init(Error **errp)
>> +{
>> +#ifdef _WIN32
>> +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
>> +                             CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) {
>> +        error_setg_errno(errp, GetLastError(),
>> +                         "Unable to create cryptographic provider");
> 
> Unfortunately the return value of 'GetLastError()' isn't an errno
> so we can't use error_setg_errno here.
> 
> Just use error_setg, and report the error value with a '(code=%u)'
> substitution at the end of the error message.
> 

Or, use error_setg_win32(), which is designed for this purpose.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] crypto_gen_random() now also works on windows

[Daniel P. Berrange]

On Tue, Apr 25, 2017 at 08:52:58AM -0500, Eric Blake wrote:
> On 04/25/2017 08:42 AM, Daniel P. Berrange wrote:
> > On Mon, Apr 24, 2017 at 07:51:49PM +0200, Geert Martin Ijewski wrote:
> >> If no crypto library is included in the build QEMU uses
> >> qcrypto_random_bytes() to generate random data. That function tried to open
> >> /dev/urandom or /dev/random and if openeing neither file worked it errored
> >> out.
> >>
> 
> >> +int qcrypto_random_init(Error **errp)
> >> +{
> >> +#ifdef _WIN32
> >> +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
> >> +                             CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) {
> >> +        error_setg_errno(errp, GetLastError(),
> >> +                         "Unable to create cryptographic provider");
> > 
> > Unfortunately the return value of 'GetLastError()' isn't an errno
> > so we can't use error_setg_errno here.
> > 
> > Just use error_setg, and report the error value with a '(code=%u)'
> > substitution at the end of the error message.
> > 
> 
> Or, use error_setg_win32(), which is designed for this purpose.

Oh nice, didn't know that existed !

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] [PATCH v2] crypto_gen_random() now also works on windows

[Geert Martin Ijewski]

Am 25.04.2017 um 15:42 schrieb Daniel P. Berrange:
> On Mon, Apr 24, 2017 at 07:51:49PM +0200, Geert Martin Ijewski wrote:
>> +#ifdef _WIN32
>> +#include <Wincrypt.h>
>> +HCRYPTPROV hCryptProv;
>> +#else
>> +int fd; /* a file handle to either /dev/urandom or /dev/random */
>> +#endif
>
>
> Lets mark both these vars 'static'

Ah yes, of course - that was a stupid error.

>>
>> +int qcrypto_random_init(Error **errp)
>> +{
>> +#ifdef _WIN32
>> +    if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL,
>> +                             CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) {
>> +        error_setg_errno(errp, GetLastError(),
>> +                         "Unable to create cryptographic provider");
>
> Unfortunately the return value of 'GetLastError()' isn't an errno
> so we can't use error_setg_errno here.
>
> Just use error_setg, and report the error value with a '(code=%u)'
> substitution at the end of the error message.
>
>

I'll use Eric's suggestion here, thanks to the both of you.

>> +/**
>> + * qcrypto_random_init:
>> + * @errp: pointer to a NULL-initialized error object
>> + *
>> + * Initalizes the handles used by qcrypto_random_bytes
>> + *
>> + * Returns 0 on success, -1 on error
>> + */
>> +int qcrypto_random_init(Error **errp);
>
> We need to add a stub
>
>   int qcrypto_random_init(Error **errp G_GNUC_UNUSED) { return 0; }
>
> in the random-gcrypt.c and random-gnutls.c files

Oh, I didn't think to look into the makefile... Yes, sure I'll do that too