favor bcrypt over wincrypt for the random generator on Windows

Message ID 20200421074851.9328-1-robux4@ycbcr.xyz
State Superseded
Headers show
Series
  • favor bcrypt over wincrypt for the random generator on Windows
Related show

Commit Message

Steve Lhomme April 21, 2020, 7:48 a.m.
BCrypt is more modern and supported in Universal Apps, Wincrypt is not and
CryptGenRandom is deprecated:
https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom

BCrypt is available since Vista
https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptopenalgorithmprovider

It requires linking with bcrypt rather than advapi32 for wincrypt.
---
 libssp/configure.ac | 16 ++++++++++++++++
 libssp/ssp.c        | 20 ++++++++++++++++++++
 2 files changed, 36 insertions(+)

-- 
2.17.1

Comments

Steve Lhomme May 26, 2020, 6:40 a.m. | #1
Hello,

Any update on this ? This prevents libssp from being usable in UWP apps.

(BTW the name of the old API is not wincrypt, the header, but CryptoAPI 
or CAPI)

On 2020-04-21 9:48, Steve Lhomme wrote:
> BCrypt is more modern and supported in Universal Apps, Wincrypt is not and

> CryptGenRandom is deprecated:

> https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom

> 

> BCrypt is available since Vista

> https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptopenalgorithmprovider

> 

> It requires linking with bcrypt rather than advapi32 for wincrypt.

> ---

>   libssp/configure.ac | 16 ++++++++++++++++

>   libssp/ssp.c        | 20 ++++++++++++++++++++

>   2 files changed, 36 insertions(+)

> 

> diff --git a/libssp/configure.ac b/libssp/configure.ac

> index f30f81c54f6..a39d9e9c992 100644

> --- a/libssp/configure.ac

> +++ b/libssp/configure.ac

> @@ -158,6 +158,22 @@ else

>   fi

>   AC_SUBST(ssp_have_usable_vsnprintf)

>   

> +AC_ARG_ENABLE(bcrypt,

> +AS_HELP_STRING([--disable-bcrypt],

> +  [use bcrypt for random generator on Windows (otherwise wincrypt)]),

> +  use_win_bcrypt=$enableval,

> +  use_win_bcrypt=yes)

> +if test "x$use_win_bcrypt" != xno; then

> +  case "$target_os" in

> +    win32 | pe | mingw32*)

> +      AC_CHECK_TYPES([BCRYPT_ALG_HANDLE],[

> +  LDFLAGS="$LDFLAGS -lbcrypt"

> +],[],[#include <windows.h>

> +#include <bcrypt.h>])

> +    ;;

> +  esac

> +fi

> +

>   AM_PROG_LIBTOOL

>   ACX_LT_HOST_FLAGS

>   AC_SUBST(enable_shared)

> diff --git a/libssp/ssp.c b/libssp/ssp.c

> index 28f3e9cc64a..f07cc41fd4f 100644

> --- a/libssp/ssp.c

> +++ b/libssp/ssp.c

> @@ -56,7 +56,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see

>      to the console using  "CONOUT$"   */

>   #if defined (_WIN32) && !defined (__CYGWIN__)

>   #include <windows.h>

> +#ifdef HAVE_BCRYPT_ALG_HANDLE

> +#include <bcrypt.h>

> +#else

>   #include <wincrypt.h>

> +#endif

>   # define _PATH_TTY "CONOUT$"

>   #else

>   # define _PATH_TTY "/dev/tty"

> @@ -77,6 +81,21 @@ __guard_setup (void)

>       return;

>   

>   #if defined (_WIN32) && !defined (__CYGWIN__)

> +#ifdef HAVE_BCRYPT_ALG_HANDLE

> +  BCRYPT_ALG_HANDLE algo = 0;

> +  NTSTATUS err = BCryptOpenAlgorithmProvider(&algo, BCRYPT_RNG_ALGORITHM,

> +                                             NULL, 0);

> +  if (BCRYPT_SUCCESS(err))

> +    {

> +      if (BCryptGenRandom(algo, (BYTE *)&__stack_chk_guard,

> +                          sizeof (__stack_chk_guard), 0) && __stack_chk_guard != 0)

> +        {

> +           BCryptCloseAlgorithmProvider(algo, 0);

> +           return;

> +        }

> +      BCryptCloseAlgorithmProvider(algo, 0);

> +    }

> +#else /* !HAVE_BCRYPT_ALG_HANDLE */

>     HCRYPTPROV hprovider = 0;

>     if (CryptAcquireContext(&hprovider, NULL, NULL, PROV_RSA_FULL,

>                             CRYPT_VERIFYCONTEXT | CRYPT_SILENT))

> @@ -89,6 +108,7 @@ __guard_setup (void)

>           }

>         CryptReleaseContext(hprovider, 0);

>       }

> +#endif /* !HAVE_BCRYPT_ALG_HANDLE */

>   #else

>     int fd = open ("/dev/urandom", O_RDONLY);

>     if (fd != -1)

> -- 

> 2.17.1

>
Richard Sandiford June 1, 2020, 4:30 p.m. | #2
Steve Lhomme <robux4@ycbcr.xyz> writes:
> Hello,

>

> Any update on this ? This prevents libssp from being usable in UWP apps.

>

> (BTW the name of the old API is not wincrypt, the header, but CryptoAPI 

> or CAPI)


Sorry for the slow review.  I fear most global reviewers would have
no idea whether the patch is right or not.  Maybe Jon (cc:ed) could
comment.

Thanks,
Richard

>

> On 2020-04-21 9:48, Steve Lhomme wrote:

>> BCrypt is more modern and supported in Universal Apps, Wincrypt is not and

>> CryptGenRandom is deprecated:

>> https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom

>> 

>> BCrypt is available since Vista

>> https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptopenalgorithmprovider

>> 

>> It requires linking with bcrypt rather than advapi32 for wincrypt.

>> ---

>>   libssp/configure.ac | 16 ++++++++++++++++

>>   libssp/ssp.c        | 20 ++++++++++++++++++++

>>   2 files changed, 36 insertions(+)

>> 

>> diff --git a/libssp/configure.ac b/libssp/configure.ac

>> index f30f81c54f6..a39d9e9c992 100644

>> --- a/libssp/configure.ac

>> +++ b/libssp/configure.ac

>> @@ -158,6 +158,22 @@ else

>>   fi

>>   AC_SUBST(ssp_have_usable_vsnprintf)

>>   

>> +AC_ARG_ENABLE(bcrypt,

>> +AS_HELP_STRING([--disable-bcrypt],

>> +  [use bcrypt for random generator on Windows (otherwise wincrypt)]),

>> +  use_win_bcrypt=$enableval,

>> +  use_win_bcrypt=yes)

>> +if test "x$use_win_bcrypt" != xno; then

>> +  case "$target_os" in

>> +    win32 | pe | mingw32*)

>> +      AC_CHECK_TYPES([BCRYPT_ALG_HANDLE],[

>> +  LDFLAGS="$LDFLAGS -lbcrypt"

>> +],[],[#include <windows.h>

>> +#include <bcrypt.h>])

>> +    ;;

>> +  esac

>> +fi

>> +

>>   AM_PROG_LIBTOOL

>>   ACX_LT_HOST_FLAGS

>>   AC_SUBST(enable_shared)

>> diff --git a/libssp/ssp.c b/libssp/ssp.c

>> index 28f3e9cc64a..f07cc41fd4f 100644

>> --- a/libssp/ssp.c

>> +++ b/libssp/ssp.c

>> @@ -56,7 +56,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see

>>      to the console using  "CONOUT$"   */

>>   #if defined (_WIN32) && !defined (__CYGWIN__)

>>   #include <windows.h>

>> +#ifdef HAVE_BCRYPT_ALG_HANDLE

>> +#include <bcrypt.h>

>> +#else

>>   #include <wincrypt.h>

>> +#endif

>>   # define _PATH_TTY "CONOUT$"

>>   #else

>>   # define _PATH_TTY "/dev/tty"

>> @@ -77,6 +81,21 @@ __guard_setup (void)

>>       return;

>>   

>>   #if defined (_WIN32) && !defined (__CYGWIN__)

>> +#ifdef HAVE_BCRYPT_ALG_HANDLE

>> +  BCRYPT_ALG_HANDLE algo = 0;

>> +  NTSTATUS err = BCryptOpenAlgorithmProvider(&algo, BCRYPT_RNG_ALGORITHM,

>> +                                             NULL, 0);

>> +  if (BCRYPT_SUCCESS(err))

>> +    {

>> +      if (BCryptGenRandom(algo, (BYTE *)&__stack_chk_guard,

>> +                          sizeof (__stack_chk_guard), 0) && __stack_chk_guard != 0)

>> +        {

>> +           BCryptCloseAlgorithmProvider(algo, 0);

>> +           return;

>> +        }

>> +      BCryptCloseAlgorithmProvider(algo, 0);

>> +    }

>> +#else /* !HAVE_BCRYPT_ALG_HANDLE */

>>     HCRYPTPROV hprovider = 0;

>>     if (CryptAcquireContext(&hprovider, NULL, NULL, PROV_RSA_FULL,

>>                             CRYPT_VERIFYCONTEXT | CRYPT_SILENT))

>> @@ -89,6 +108,7 @@ __guard_setup (void)

>>           }

>>         CryptReleaseContext(hprovider, 0);

>>       }

>> +#endif /* !HAVE_BCRYPT_ALG_HANDLE */

>>   #else

>>     int fd = open ("/dev/urandom", O_RDONLY);

>>     if (fd != -1)

>> -- 

>> 2.17.1

>>
Steve Lhomme June 2, 2020, 7:46 a.m. | #3
Actually I found a but in the patch. The BCryptGenRandom() doesn't 
return a BOOL like CryptGenRandom so the success needs to be checked 
differently.

I'm sending an updated patch with the wincrypt name change as well.

On 2020-06-01 18:30, Richard Sandiford wrote:
> Steve Lhomme <robux4@ycbcr.xyz> writes:

>> Hello,

>>

>> Any update on this ? This prevents libssp from being usable in UWP apps.

>>

>> (BTW the name of the old API is not wincrypt, the header, but CryptoAPI

>> or CAPI)

> 

> Sorry for the slow review.  I fear most global reviewers would have

> no idea whether the patch is right or not.  Maybe Jon (cc:ed) could

> comment.

> 

> Thanks,

> Richard

> 

>>

>> On 2020-04-21 9:48, Steve Lhomme wrote:

>>> BCrypt is more modern and supported in Universal Apps, Wincrypt is not and

>>> CryptGenRandom is deprecated:

>>> https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom

>>>

>>> BCrypt is available since Vista

>>> https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptopenalgorithmprovider

>>>

>>> It requires linking with bcrypt rather than advapi32 for wincrypt.

>>> ---

>>>    libssp/configure.ac | 16 ++++++++++++++++

>>>    libssp/ssp.c        | 20 ++++++++++++++++++++

>>>    2 files changed, 36 insertions(+)

>>>

>>> diff --git a/libssp/configure.ac b/libssp/configure.ac

>>> index f30f81c54f6..a39d9e9c992 100644

>>> --- a/libssp/configure.ac

>>> +++ b/libssp/configure.ac

>>> @@ -158,6 +158,22 @@ else

>>>    fi

>>>    AC_SUBST(ssp_have_usable_vsnprintf)

>>>    

>>> +AC_ARG_ENABLE(bcrypt,

>>> +AS_HELP_STRING([--disable-bcrypt],

>>> +  [use bcrypt for random generator on Windows (otherwise wincrypt)]),

>>> +  use_win_bcrypt=$enableval,

>>> +  use_win_bcrypt=yes)

>>> +if test "x$use_win_bcrypt" != xno; then

>>> +  case "$target_os" in

>>> +    win32 | pe | mingw32*)

>>> +      AC_CHECK_TYPES([BCRYPT_ALG_HANDLE],[

>>> +  LDFLAGS="$LDFLAGS -lbcrypt"

>>> +],[],[#include <windows.h>

>>> +#include <bcrypt.h>])

>>> +    ;;

>>> +  esac

>>> +fi

>>> +

>>>    AM_PROG_LIBTOOL

>>>    ACX_LT_HOST_FLAGS

>>>    AC_SUBST(enable_shared)

>>> diff --git a/libssp/ssp.c b/libssp/ssp.c

>>> index 28f3e9cc64a..f07cc41fd4f 100644

>>> --- a/libssp/ssp.c

>>> +++ b/libssp/ssp.c

>>> @@ -56,7 +56,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see

>>>       to the console using  "CONOUT$"   */

>>>    #if defined (_WIN32) && !defined (__CYGWIN__)

>>>    #include <windows.h>

>>> +#ifdef HAVE_BCRYPT_ALG_HANDLE

>>> +#include <bcrypt.h>

>>> +#else

>>>    #include <wincrypt.h>

>>> +#endif

>>>    # define _PATH_TTY "CONOUT$"

>>>    #else

>>>    # define _PATH_TTY "/dev/tty"

>>> @@ -77,6 +81,21 @@ __guard_setup (void)

>>>        return;

>>>    

>>>    #if defined (_WIN32) && !defined (__CYGWIN__)

>>> +#ifdef HAVE_BCRYPT_ALG_HANDLE

>>> +  BCRYPT_ALG_HANDLE algo = 0;

>>> +  NTSTATUS err = BCryptOpenAlgorithmProvider(&algo, BCRYPT_RNG_ALGORITHM,

>>> +                                             NULL, 0);

>>> +  if (BCRYPT_SUCCESS(err))

>>> +    {

>>> +      if (BCryptGenRandom(algo, (BYTE *)&__stack_chk_guard,

>>> +                          sizeof (__stack_chk_guard), 0) && __stack_chk_guard != 0)

>>> +        {

>>> +           BCryptCloseAlgorithmProvider(algo, 0);

>>> +           return;

>>> +        }

>>> +      BCryptCloseAlgorithmProvider(algo, 0);

>>> +    }

>>> +#else /* !HAVE_BCRYPT_ALG_HANDLE */

>>>      HCRYPTPROV hprovider = 0;

>>>      if (CryptAcquireContext(&hprovider, NULL, NULL, PROV_RSA_FULL,

>>>                              CRYPT_VERIFYCONTEXT | CRYPT_SILENT))

>>> @@ -89,6 +108,7 @@ __guard_setup (void)

>>>            }

>>>          CryptReleaseContext(hprovider, 0);

>>>        }

>>> +#endif /* !HAVE_BCRYPT_ALG_HANDLE */

>>>    #else

>>>      int fd = open ("/dev/urandom", O_RDONLY);

>>>      if (fd != -1)

>>> -- 

>>> 2.17.1

>>>

Patch

diff --git a/libssp/configure.ac b/libssp/configure.ac
index f30f81c54f6..a39d9e9c992 100644
--- a/libssp/configure.ac
+++ b/libssp/configure.ac
@@ -158,6 +158,22 @@  else
 fi
 AC_SUBST(ssp_have_usable_vsnprintf)
 
+AC_ARG_ENABLE(bcrypt,
+AS_HELP_STRING([--disable-bcrypt],
+  [use bcrypt for random generator on Windows (otherwise wincrypt)]),
+  use_win_bcrypt=$enableval,
+  use_win_bcrypt=yes)
+if test "x$use_win_bcrypt" != xno; then
+  case "$target_os" in
+    win32 | pe | mingw32*)
+      AC_CHECK_TYPES([BCRYPT_ALG_HANDLE],[
+  LDFLAGS="$LDFLAGS -lbcrypt"
+],[],[#include <windows.h>
+#include <bcrypt.h>])
+    ;;
+  esac
+fi
+
 AM_PROG_LIBTOOL
 ACX_LT_HOST_FLAGS
 AC_SUBST(enable_shared)
diff --git a/libssp/ssp.c b/libssp/ssp.c
index 28f3e9cc64a..f07cc41fd4f 100644
--- a/libssp/ssp.c
+++ b/libssp/ssp.c
@@ -56,7 +56,11 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    to the console using  "CONOUT$"   */
 #if defined (_WIN32) && !defined (__CYGWIN__)
 #include <windows.h>
+#ifdef HAVE_BCRYPT_ALG_HANDLE
+#include <bcrypt.h>
+#else
 #include <wincrypt.h>
+#endif
 # define _PATH_TTY "CONOUT$"
 #else
 # define _PATH_TTY "/dev/tty"
@@ -77,6 +81,21 @@  __guard_setup (void)
     return;
 
 #if defined (_WIN32) && !defined (__CYGWIN__)
+#ifdef HAVE_BCRYPT_ALG_HANDLE
+  BCRYPT_ALG_HANDLE algo = 0;
+  NTSTATUS err = BCryptOpenAlgorithmProvider(&algo, BCRYPT_RNG_ALGORITHM, 
+                                             NULL, 0);
+  if (BCRYPT_SUCCESS(err))
+    {
+      if (BCryptGenRandom(algo, (BYTE *)&__stack_chk_guard,
+                          sizeof (__stack_chk_guard), 0) && __stack_chk_guard != 0)
+        {
+           BCryptCloseAlgorithmProvider(algo, 0);
+           return;
+        }
+      BCryptCloseAlgorithmProvider(algo, 0);
+    }
+#else /* !HAVE_BCRYPT_ALG_HANDLE */
   HCRYPTPROV hprovider = 0;
   if (CryptAcquireContext(&hprovider, NULL, NULL, PROV_RSA_FULL,
                           CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
@@ -89,6 +108,7 @@  __guard_setup (void)
         }
       CryptReleaseContext(hprovider, 0);
     }
+#endif /* !HAVE_BCRYPT_ALG_HANDLE */
 #else
   int fd = open ("/dev/urandom", O_RDONLY);
   if (fd != -1)