Replace Windows APIs that are banned in Windows Store apps #730
Conversation
|
ping. What's holding this back? |
|
One week has gone by. ping again. |
|
Hi @thiagomacieira and thanks for the interest in mbed TLS. It's nice to see Intel following the project. If you look have a look, you'll see we currently have a large number of open pull requests from the community. As a security and cryptography library, we carefully review all contributions, and we only have so much capacity to make such reviews. That means a far slower adoption and take up of contributions than we'd like, which is frustrating both for us and the community. We're currently expanding the team, and revising both how we accept contributions and our CI system to make it easier and faster to accept contributions, which I hope will resolve the problem in the near future, but in the meantime it means that we're less responsive in accepting contributions than we'd like. |
|
Hi, it's been one month. Are there any updates? |
|
Hi @thiagomacieira Thanks again for the reminder |
|
Hi @kevinmkane, |
|
Hi @RonEld, Those definitions between stdint.h and intsafe.h will always be the same per a comment in Visual Studio's version of stdint.h. According to https://connect.microsoft.com/VisualStudio/feedback/details/621653 this issue existed in VS2010 but was fixed as of VS2012. Is it acceptable to suppress the warning when intsafe.h is included with an explanatory comment? The alternative would be to copy the definitions of the macros and inline functions I'm introducing directly into the code and not include intsafe.h at all. |
|
@kevinmkane Thank you for your explanation
I think this would be the lesser of two evils. If possible, surround the pragma with |
|
I have now pushed a follow-up commit to suppress the warning as we discussed. |
library/entropy_poll.c
Outdated
| @@ -54,28 +54,41 @@ | |||
| #define _WIN32_WINNT 0x0400 | |||
| #endif | |||
| #include <windows.h> | |||
| #include <wincrypt.h> | |||
| #include <bcrypt.h> | |||
| #if defined(_MSC_VER) | |||
There was a problem hiding this comment.
Shouldn't it be #if _MSC_VER <= 1600?
I believe visual studio 2010 _MSC_VER value is 1600
There was a problem hiding this comment.
Yes, that's a good idea. That way we'll catch any legitimate instances of duplicated definitions with later versions of the compiler. I've overwritten the last commit with this change.
kevinmkane
force-pushed
the
bannedapis
branch
from
01ac0f0 to
c43f40b
Compare
|
@kevinmkane Thank you for considering my comments. |
There was a problem hiding this comment.
In the commit message, please fix spelling of different
- these types are differnt
+ these types are differentExcept for minor trailing white space issue and misspelling in the commit message, this looks good to me.
Gatekeeper Note: this adds a requirement on bcrypt, which is available since Windows Vista.
library/entropy_poll.c
Outdated
| ((void) data); | ||
| *olen = 0; | ||
|
|
||
| if( CryptAcquireContext( &provider, NULL, NULL, | ||
| PROV_RSA_FULL, CRYPT_VERIFYCONTEXT ) == FALSE ) | ||
| /* |
There was a problem hiding this comment.
Please don't add trailing white space in any commits of this PR.
CryptGenRandom and lstrlenW are not permitted in Windows Store apps, meaning apps that use mbedTLS can't ship in the Windows Store. Instead, use BCryptGenRandom and wcslen, respectively, which are permitted. Also make sure conversions between size_t, ULONG, and int are always done safely; on a 64-bit platform, these types are different sizes. Also suppress macro redefinition warning for intsafe.h: Visual Studio 2010 and earlier generates C4005 when including both <intsafe.h> and <stdint.h> because a number of <TYPE>_MAX constants are redefined. This is fixed in later versions of Visual Studio. The constants are guaranteed to be the same between both files, however, so we can safely suppress the warning when including intsafe.h.
|
@RonEld I did use generate_visualc_files.pl. |
|
Automatic CI verification build not done, please verify manually. |
|
@Patater Both commits squashed into one with this latest update, which includes your requested corrections. |
|
Thanks! |
|
This breaks mingw64 on centos 7.4. |
|
As per the published policy on Windows API support, this is now approved for the |
| * BCryptGenRandom takes ULONG for size, which is smaller than size_t on 64-bit platforms. | ||
| * Ensure len's value can be safely converted into a ULONG. | ||
| */ | ||
| if ( FAILED( SizeTToULong( len, &len_as_ulong ) ) ) |
There was a problem hiding this comment.
SizeTToUlong doesn't exist in MinGW 5.3 so either the code using it must be under #ifdef _MSC_VER guards or an implementation must be provided. Do newer versions of MinGW support it? It could be acceptable to require a more recent version going forward.
| @@ -1122,7 +1135,10 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) | |||
| p = filename + len; | |||
| filename[len++] = '*'; | |||
|
|
|||
| w_ret = MultiByteToWideChar( CP_ACP, 0, filename, (int)len, szDir, | |||
| if ( FAILED ( SizeTToInt( len, &lengthAsInt ) ) ) | |||
There was a problem hiding this comment.
Fix merge mistakes introduced when rebasing the PR Mbed-TLS#730.
|
Opened up a new PR, #1453, to address the issues building with This PR can now be closed as development will continue there. |
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
Signed-off-by: Simon Butcher <[email protected]>
CryptGenRandom and lstrlenW are not permitted in Windows Store apps,
meaning apps that use mbedTLS can't ship in the Windows Store.
Instead, use BCryptGenRandom and wcslen, respectively, which are
permitted.
Also make sure conversions between size_t, ULONG, and int are
always done safely; on a 64-bit platform, these types are differnt
sizes.