Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/update win crypto apis #8047

Merged
merged 33 commits into from
Sep 25, 2023
Merged

Bugfix/update win crypto apis #8047

Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0ec1e68
Replace Windows APIs that are banned in Windows Store apps
kevinmkane Dec 15, 2016
e068aa7
Fix the build for mingw and CMake + VStudio
simonbutcher Mar 14, 2018
34b8d83
Update ChangeLog for PR #730 for Win32 API fixes
simonbutcher Mar 14, 2018
33425de
Correct check for WIN32 in cmake files for programs
simonbutcher Mar 14, 2018
1c0c5d2
Fix for building programs with mingw
simonbutcher Mar 26, 2018
def90f4
Fix formatting and detail of comments in PR #730
simonbutcher Mar 14, 2018
bcb6cfb
Fix the tests build with mingw for the new Win32 APIs
simonbutcher Mar 14, 2018
35e5dad
Add clarifying comment on use of MultiByteToWideChar() and CP_ACP
simonbutcher Mar 15, 2018
949aa8f
Remove redundant Visual Studio 6 data files
simonbutcher Mar 25, 2018
de573f5
Fix coding style of length_as_int var in x509_crt.c
simonbutcher Jul 5, 2018
769ee65
Fix Visual Studio Release|x64 builds
simonbutcher Dec 10, 2018
b8d6b82
programs: Cleaned up bcrypt linking refererences.
minosgalanakis Aug 9, 2023
a277b21
Code style fixes
minosgalanakis Aug 9, 2023
e13775d
fuzzer Makefile: Added -lbcrypt linkage
minosgalanakis Aug 11, 2023
24a1c16
library Makefile: Moved -lbcrypt to LOCAL_LDFLAGS
minosgalanakis Aug 11, 2023
12b493f
entropy_poll/x509_crt: Added MBEDTLS_POP_TARGET_PRAGMA define guards.
minosgalanakis Aug 11, 2023
8792717
Changelog: Removed entry from root file
minosgalanakis Aug 11, 2023
e960365
ChangeLog.d: Reworded updated_windows_apis.txt.
minosgalanakis Aug 18, 2023
4952f70
Removed unsupported Visual Studio related code in entropy_poll.c and …
minosgalanakis Aug 18, 2023
7afebcc
ChangeLog.d: Added mininum required Windows version.
minosgalanakis Sep 6, 2023
2c6e561
entropy_poll.c: Added looping logic to `mbedtls_platform_entropy_poll…
minosgalanakis Sep 6, 2023
ce33e7b
pkey Cmakelists: Updated the set libs to be consistent with others.
minosgalanakis Sep 6, 2023
7f8e8c5
program-random: Updated Cmake libs variable
minosgalanakis Sep 8, 2023
e8a5d1a
entropy_poll: Updated documentation for entropy_poll loop.
minosgalanakis Sep 8, 2023
a8b02ef
pkey-random: Removed setting mbedtls_target in libs
minosgalanakis Sep 18, 2023
c91d847
ChangeLog: Adjusted the updated_windows_apis log
minosgalanakis Sep 18, 2023
fac45fb
entropy_poll: Removed checks for windows versions < WINXP
minosgalanakis Sep 18, 2023
40995e1
x509_crt: Removed checks for windows versions < WINXP
minosgalanakis Sep 18, 2023
b17410d
vs2013 templates: Set bcrypt to be the sole dependency.
minosgalanakis Sep 22, 2023
08a67cc
x509_crt: Set WideCharToMultiByte to use -1 for length.
minosgalanakis Sep 22, 2023
59108d3
x509_crt: Adjusted the len of lpMultiByteStr arg in WideCharToMultiByte
minosgalanakis Sep 25, 2023
a9bb34c
x509_crt: Removed length_as_int intermediate variable
minosgalanakis Sep 25, 2023
2108775
x509_crt: Removed unused intsafe.h
minosgalanakis Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ChangeLog.d/updated_windows_apis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Requirement changes
* Minimum required Windows version is now Windows Vista, or
Windows Server 2008.

Changes
* Update Windows code to use BCryptGenRandom and wcslen, and
ensure that conversions between size_t, ULONG, and int are
always done safely. Original contribution by Kevin Kane #635, #730
followed by Simon Butcher #1453.
2 changes: 1 addition & 1 deletion library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ if(CMAKE_COMPILER_IS_MSVC)
endif()

if(WIN32)
set(libs ${libs} ws2_32)
set(libs ${libs} ws2_32 bcrypt)
endif(WIN32)

if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
Expand Down
4 changes: 4 additions & 0 deletions library/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ APPLE_BUILD ?= 1
endif
endif

ifdef WINDOWS_BUILD
LOCAL_LDFLAGS += -lbcrypt
endif

# To compile as a shared library:
ifdef SHARED
# all code is position-indep with mingw, avoid warning about useless flag
Expand Down
33 changes: 17 additions & 16 deletions library/entropy_poll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,34 +49,35 @@
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)

#include <windows.h>
#if _WIN32_WINNT >= 0x0501 /* _WIN32_WINNT_WINXP */
#include <wincrypt.h>
#include <bcrypt.h>
#include <intsafe.h>

int mbedtls_platform_entropy_poll(void *data, unsigned char *output, size_t len,
size_t *olen)
{
HCRYPTPROV provider;
((void) data);
*olen = 0;

if (CryptAcquireContext(&provider, NULL, NULL,
PROV_RSA_FULL, CRYPT_VERIFYCONTEXT) == FALSE) {
return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED;
}
/*
* BCryptGenRandom takes ULONG for size, which is smaller than size_t on
* 64-bit Windows platforms. Extract entropy in chunks of len (dependent
* on ULONG_MAX) size.
*/
while (len != 0) {
unsigned long ulong_bytes =
(len > ULONG_MAX) ? ULONG_MAX : (unsigned long) len;

if (!BCRYPT_SUCCESS(BCryptGenRandom(NULL, output, ulong_bytes,
BCRYPT_USE_SYSTEM_PREFERRED_RNG))) {
return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED;
}

if (CryptGenRandom(provider, (DWORD) len, output) == FALSE) {
CryptReleaseContext(provider, 0);
return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED;
*olen += ulong_bytes;
len -= ulong_bytes;
}

CryptReleaseContext(provider, 0);
*olen = len;

return 0;
}
#else /* !_WIN32_WINNT_WINXP */
#error "Entropy not available before Windows XP, use MBEDTLS_NO_PLATFORM_ENTROPY"
#endif /* !_WIN32_WINNT_WINXP */
#else /* _WIN32 && !EFIX64 && !EFI32 */

/*
Expand Down
22 changes: 13 additions & 9 deletions library/x509_crt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <intsafe.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this change :) or can't we bear the thought of another update? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if it clears CI, and I could try to push a last commit to remove that. Re reviewing an include could be simple.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note that the only functional changes to this file now are the #if changes)

#else
#include <time.h>
#endif
Expand Down Expand Up @@ -1535,12 +1536,12 @@ int mbedtls_x509_crt_parse_path(mbedtls_x509_crt *chain, const char *path)
{
int ret = 0;
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)
#if _WIN32_WINNT >= 0x0501 /* _WIN32_WINNT_XP */
int w_ret;
WCHAR szDir[MAX_PATH];
char filename[MAX_PATH];
char *p;
size_t len = strlen(path);
int length_as_int = 0;

WIN32_FIND_DATAW file_data;
HANDLE hFind;
Expand All @@ -1556,7 +1557,16 @@ 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, &length_as_int))) {
return MBEDTLS_ERR_X509_FILE_IO_ERROR;
}

/*
* Note this function uses the code page CP_ACP which is the system default
* ANSI codepage. The input string is always described in BYTES and the
* output length is described in WCHARs.
*/
w_ret = MultiByteToWideChar(CP_ACP, 0, filename, length_as_int, szDir,
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading this change, I'm not sure why we're bothering with the whole length_as_int thing.

This would be a concern if len wouldn't fit in an int, but we actually know that len <= MAX_PATH - 3, where MAX_PATH is 260 or 32,767 (https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry)

So the cast in the (original) line 1559 is totally fine.

We also do a similar cast in (new) line 1588 - admittedly this is now "length remaining in buffer", which is a horrible re-use of variable with opposite meaning, but if we are happy with the cast at 1588 we should be happy with the cast at 1559.

(And fewer changes => better, IMO)

Supporting "Starting in Windows 10, version 1607, MAX_PATH limitations have been removed from common Win32 file and directory functions" would be a bigger, separate, set of changes

MAX_PATH - 3);
if (w_ret == 0) {
return MBEDTLS_ERR_X509_BAD_INPUT_DATA;
Expand All @@ -1574,11 +1584,8 @@ int mbedtls_x509_crt_parse_path(mbedtls_x509_crt *chain, const char *path)
if (file_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
continue;
}

w_ret = WideCharToMultiByte(CP_ACP, 0, file_data.cFileName,
-1,
p, (int) len,
NULL, NULL);
-1, p, (int) len, NULL, NULL);
if (w_ret == 0) {
ret = MBEDTLS_ERR_X509_FILE_IO_ERROR;
goto cleanup;
Expand All @@ -1598,9 +1605,6 @@ int mbedtls_x509_crt_parse_path(mbedtls_x509_crt *chain, const char *path)

cleanup:
FindClose(hFind);
#else /* !_WIN32_WINNT_XP */
#error "mbedtls_x509_crt_parse_path not available before Windows XP"
#endif /* !_WIN32_WINNT_XP */
#else /* _WIN32 */
int t_ret;
int snp_ret;
Expand Down
2 changes: 1 addition & 1 deletion programs/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ endif
ifdef WINDOWS_BUILD
DLEXT=dll
EXEXT=.exe
LOCAL_LDFLAGS += -lws2_32
LOCAL_LDFLAGS += -lws2_32 -lbcrypt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not adding this for cmake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in cmake we can set it once and it propagates throuth the mbedtls_target.

ifdef SHARED
SHARED_SUFFIX=.$(DLEXT)
endif
Expand Down
4 changes: 4 additions & 0 deletions programs/fuzz/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ ifdef FUZZINGENGINE
LOCAL_LDFLAGS += -lFuzzingEngine
endif

ifdef WINDOWS_BUILD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this, under CMake, the fuzz build is explicitly removed for windows builds, and I am presuming this is why you have not added bcrypt to the cmake libs for fuzz.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been removed was not needed for Make either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update. The ci is failing when removing this code

[2023-09-22T17:23:32.311Z] * build_mingw: build: Windows cross build - mingw64, make (Link Library)
.....
[2023-09-22T17:23:52.954Z]   CC    ssl/ssl_client2.c
[2023-09-22T17:23:53.520Z]  i686-w64-mingw32-gcc common.o onefile.o fuzz_x509csr.o ../../tests/src/helpers.o ../../tests/src/asn1_helpers.o ../../tests/src/psa_crypto_helpers.o ../../tests/src/psa_exercise_key.o ../../tests/src/bignum_helpers.o ../../tests/src/threading_helpers.o ../../tests/src/random.o ../../tests/src/fake_external_rng_for_test.o ../../tests/src/certs.o ../../tests/src/drivers/test_driver_aead.o ../../tests/src/drivers/test_driver_asymmetric_encryption.o ../../tests/src/drivers/test_driver_pake.o ../../tests/src/drivers/test_driver_key_agreement.o ../../tests/src/drivers/test_driver_signature.o ../../tests/src/drivers/test_driver_key_management.o ../../tests/src/drivers/test_driver_cipher.o ../../tests/src/drivers/hash.o ../../tests/src/drivers/test_driver_mac.o ../../tests/src/drivers/platform_builtin_keys.o -L../../library -lmbedtls -lmbedx509 -lmbedcrypto  -o fuzz_x509csr
[2023-09-22T17:23:53.521Z] ../../library/libmbedcrypto.a(entropy_poll.o):entropy_poll.c:(.text+0x35): undefined reference to `BCryptGenRandom@16'
[2023-09-22T17:23:53.521Z] collect2: error: ld returned 1 exit status
[2023-09-22T17:23:53.521Z] Makefile:59: recipe for target 'fuzz_x509csr' failed
[2023-09-22T17:23:53.521Z] make[2]: *** [fuzz_x509csr] Error 1
[2023-09-22T17:23:53.521Z] make[2]: *** Waiting for unfinished jobs....
[2023-09-22T17:23:53.521Z] make[1]: *** [fuzz] Error 2
[2023-09-22T17:23:53.521Z] Makefile:167: recipe for target 'fuzz' failed
[2023-09-22T17:23:53.521Z] Makefile:15: recipe for target 'programs' failed
[2023-09-22T17:23:53.521Z] make: *** [programs] Error 2
[2023-09-22T17:23:53.521Z] ^^^^build_mingw: build: Windows cross build - mingw64, make (Link Library): make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 lib programs -> 2^^^^
[2023-09-22T17:23:53.779Z] 
[2023-09-22T17:23:53.779Z] ******************************************************************
[2023-09-22T17:23:53.779Z] * Done, cleaning up 
[2023-09-22T17:23:53.779Z] * Fri Sep 22 17:23:53 UTC 2023
[2023-09-22T17:23:53.779Z] ******************************************************************
[2023-09-22T17:23:54.345Z]

The test snippet from all.sh is set as:

component_build_mingw () {
    msg "build: Windows cross build - mingw64, make (Link Library)" # ~ 30s
    scripts/config.py unset MBEDTLS_AESNI_C # AESNI depends on cpu modifiers
    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 lib programs

    # note Make tests only builds the tests, but doesn't run them
    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror' WINDOWS_BUILD=1 tests
    make WINDOWS_BUILD=1 clean

    msg "build: Windows cross build - mingw64, make (DLL)" # ~ 30s
    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 SHARED=1 lib programs
    make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 SHARED=1 tests
    make WINDOWS_BUILD=1 clean
}

So linking against the new library is required at least for make. I will be rebasing and removing commit 800223c .

We could address the different behaviour between make and cmake in a follow up pr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could address the different behaviour between make and cmake in a follow up pr?

The purpose of this PR is to update to later versions of Windows APIs, so let's not make changes to behaviour unless necessary when updating to the later APIs.

(i.e. changing different Make vs CMake behaviour would require design review - specifically, why was it done in the first place? and is that reason still relevant?)

LOCAL_LDFLAGS += -lbcrypt
endif

# A test application is built for each suites/test_suite_*.data file.
# Application name is same as .data file's base name and can be
# constructed by stripping path 'suites/' and extension .data.
Expand Down
8 changes: 4 additions & 4 deletions scripts/data_files/vs2013-app-template.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ INCLUDE_DIRECTORIES
<Link>
<SubSystem>Console</SubSystem>
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalDependencies>kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalLibraryDirectories>Debug</AdditionalLibraryDirectories>
</Link>
<ProjectReference>
Expand All @@ -118,7 +118,7 @@ INCLUDE_DIRECTORIES
<Link>
<SubSystem>Console</SubSystem>
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalDependencies>kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalLibraryDirectories>Debug</AdditionalLibraryDirectories>
</Link>
<ProjectReference>
Expand All @@ -142,7 +142,7 @@ INCLUDE_DIRECTORIES
<EnableCOMDATFolding>true</EnableCOMDATFolding>
<OptimizeReferences>true</OptimizeReferences>
<AdditionalLibraryDirectories>Release</AdditionalLibraryDirectories>
<AdditionalDependencies>kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
Expand All @@ -162,7 +162,7 @@ INCLUDE_DIRECTORIES
<EnableCOMDATFolding>true</EnableCOMDATFolding>
<OptimizeReferences>true</OptimizeReferences>
<AdditionalLibraryDirectories>Release</AdditionalLibraryDirectories>
<AdditionalDependencies>%(AdditionalDependencies);</AdditionalDependencies>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
Expand Down
3 changes: 3 additions & 0 deletions scripts/data_files/vs2013-main-template.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ INCLUDE_DIRECTORIES
<Link>
<SubSystem>Windows</SubSystem>
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
Expand All @@ -106,6 +107,7 @@ INCLUDE_DIRECTORIES
<Link>
<SubSystem>Windows</SubSystem>
<GenerateDebugInformation>true</GenerateDebugInformation>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
Expand All @@ -124,6 +126,7 @@ INCLUDE_DIRECTORIES
<GenerateDebugInformation>true</GenerateDebugInformation>
<EnableCOMDATFolding>true</EnableCOMDATFolding>
<OptimizeReferences>true</OptimizeReferences>
<AdditionalDependencies>bcrypt.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
Expand Down
101 changes: 0 additions & 101 deletions scripts/data_files/vs6-app-template.dsp
View file
Open in desktop

This file was deleted.

Loading