Update Windows Crypto APIs #1453

simonbutcher · 2018-03-14T16:06:08Z

Description

This is an update on PR #730 to address issues building with mingw and Visual Studio when using cmake generated files.

To quote the original 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 differnt sizes.

Also, removed the obsolete Visual Studio 6 data files which are no longer used.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

Tests
[] Documentation
Changelog updated
[] Backported

gilles-peskine-arm · 2018-03-14T17:09:30Z

library/CMakeLists.txt

@@ -92,7 +92,7 @@ if(CMAKE_COMPILER_IS_CLANG)
 endif(CMAKE_COMPILER_IS_CLANG)

 if(WIN32)
-    set(libs ${libs} ws2_32)
+    set(libs ${libs} ws2_32 bcrypt)


This prevents building for old Windows versions, not just with Visual Studio but with e.g. mingw. Do we want that?

Not necessarily a blocker anyway, we can tell people with special needs (e.g. Windows XP) to edit the build scripts.

This applies to all the places where the build scripts have an added declaration to link with bcrypt.

That's a decision we've already made. Please see the new policy on it.

Basically, this is going into development and the older LTS branches will be maintained for older Windows installations.

@gilles-peskine-arm I do maintain patchsets for recent versions of mbed TLS, as I tend to support Windows 2000/XP as a side effect of linking against the system libc msvcrt.dll. (Of course, depending on the rest of the project, this is not always guaranteed.)

(Yes, this has been discouraged since 1999. This is why import libraries for the recent versioned libc are included with mingw-w64. But GCC continues doing so by default because it assumes a single, systemwide libc.so as on UNIX and Linux)

gilles-peskine-arm · 2018-03-14T17:16:57Z

library/x509_crt.c

+    if ( FAILED ( SizeTToInt( len, &lengthAsInt ) ) )
+        return( MBEDTLS_ERR_X509_FILE_IO_ERROR );
+
+    w_ret = MultiByteToWideChar( CP_ACP, 0, filename, lengthAsInt, szDir,
                                 MAX_PATH - 3 );


Preexisting: at some point someone is going to complain that CP_ACP is obsolescent. We should add a warning to state that this code depends on using a unibyte code page, because the output buffer size is in characters, not bytes. Changing to e.g. CP_UTF8 would require more complex code here. See https://msdn.microsoft.com/en-us/library/windows/desktop/dd319072(v=vs.85).aspx

Given the work @irwir has been doing on PR #1427, I think I'd rather this was raised as a ~~bug~~ issue, rather than a comment in the code.

If you agree, could you please raise the bug?

I don't see any functional bug here, just a maintenance issue that could lead to a bug in the future. This needs to be a comment in the code. What would be the bug? To switch to CP_UTF8? I don't know if it's the right thing. For all I know, CP_ACP is fine because we want to treat the file name as just some string to be passed (semantically) unchanged and we don't care about encoding.

It seems to me that #1427 would bypass this issue. I'd still rather have the comment, even in the backports.

I think I should have said raise it as an issue, not a bug, and that implies a fault.

I'm not sure my point was obvious enough to I'll explain it again. @irwir has created a PR to change the strings that touch the platform from char's to TCHAR's to allow portability between ASCII/UTF-8 and Unicode, including changing interface code so the public interface on Windows can accept TCHAR strings.

I do think we need to consider how we interface with Windows and what we should do about this, and consider if and how we address the issue.

My suggestion of raising an issue was really about that.

Anyway - I'll take your point and add a comment.

I didn't want to change this code more than I had to when I originally added this, but since you've brought it up, it looks like the whole point of these conversions is to call FindFirstFileW and FindNextFileW, and then to convert their results back to narrow character strings to be used.

Another possibility is to dispense with wide character strings entirely, since the mbedTLS API always takes narrow character strings, and use the narrow character string Windows equivalents: the FindFirstFileA and FindNextFileA functions and the WIN32_FIND_DATAA type. Then the platform will do all the string conversions internally and this code can only deal in narrow character strings.

If you decide to move to a TCHAR API approach then all of this is moot, of course, but for however long you stick with narrow character strings, you might consider this.

this code depends on using a unibyte code page

Is 'unibyte' word about one byte being one character?
In that case, ANSI is no more unibyte than UTF-8. Just remember ANSI code pages for Oriental languages.
Whereever needed, Windows provided two APIs for the same function: one ANSI, and one Unicode.
No conversion should be needed when strings and API were both of ANSI kind.

Yes, “unibyte” means that 1 character = 1 byte. Thanks @irwir, I didn't know that ANSI could be multibyte (I'm not familiar with Windows APIs at all). Given that szDir is a WCHAR array, is the size correct? In other words, does an ANSI character always fit in a WCHAR?

does an ANSI character always fit in a WCHAR?

That could be expected in most cases, but not guaranteed because WCHAR implies Unicode with its peculiarities.
MAX_PATH dedermines the number of array elements, not the number of characters (see description of WIN32_FIND_DATA structure). And, as said, conversion to wide character is not needed.

Another possibility is to dispense with wide character strings entirely, since the mbedTLS API always takes narrow character strings

The cryptographic part actually uses bytes and does not care about characters.
Characters appear when API refers to path/file names. As I was told, the library was supposed to use UTF-8 encoding, and that is not what 'narrow' strings mean in Windows code.

gilles-peskine-arm · 2018-03-14T17:23:30Z

library/Makefile

@@ -155,7 +155,7 @@ libmbedcrypto.dylib: $(OBJS_CRYPTO)

 libmbedcrypto.dll: $(OBJS_CRYPTO)
 	echo "  LD    $@"
-	$(CC) -shared -Wl,-soname,$@ -Wl,--out-implib,[email protected] -o $@ $(OBJS_CRYPTO) -lws2_32 -lwinmm -lgdi32 -static-libgcc $(LOCAL_LDFLAGS) $(LDFLAGS)
+	$(CC) -shared -Wl,-soname,$@ -Wl,--out-implib,[email protected] -o $@ $(OBJS_CRYPTO) -lws2_32 -lbcrypt -lwinmm -lgdi32 -static-libgcc $(LOCAL_LDFLAGS) $(LDFLAGS)


Blocker: you need to update something for the non-shared build as well. Under Ubuntu 17.10:

$ make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 … CC test_suite_entropy.c ../library/libmbedcrypto.a(entropy_poll.o):entropy_poll.c:(.text+0x78): undefined reference to `BCryptGenRandom@16' collect2: error: ld returned 1 exit status Makefile:309: recipe for target 'test_suite_entropy.exe' failed

Fixed in 63450c3.

gilles-peskine-arm · 2018-03-14T17:27:46Z

We should document the minimum mingw version in README. If not, at least on the website (update the KB article https://tls.mbed.org/kb/compiling-and-building/compiling-mbedtls-in-mingw).

simonbutcher · 2018-03-14T18:04:19Z

@gilles-peskine-arm - We already have an open task in JIRA (IOTSSL-1042) to update the mingw documentation.

simonbutcher · 2018-03-14T18:23:18Z

Review comments from @gilles-peskine-arm addressed. Please review my responses and respond or approve.

Thanks!

gilles-peskine-arm

Looks ok to me but I have no Windows competence. I confirm that the build with make under Ubuntu 17.10 with gcc-mingw-w64-i686 6.3 (with and without SHARED=1) works and the resulting executables run. I haven't tested other build methods or other compilers.

simonbutcher · 2018-03-15T15:21:19Z

Updated with a comment on the codepageCP_ACP.

@gilles-peskine-arm / @Patater - please re-review and approve.

Patater · 2018-03-23T15:04:49Z

Testing done (in addition to CI checks):
Built benchmark.exe with Makefile and mingw. Ran benchmark.exe on wine; it worked.

Patater

LGTM

Patater · 2018-03-23T17:48:58Z

The provided solution files for x64 fail to build. This is either a regression introduced by this PR or because the PR is based on old versions of those files. If the latter, please consider rebasing on a more recent point in history to get our recent fixes to those solution files.

simonbutcher · 2018-03-25T14:37:28Z

@Patater - You're right. It was a regression introduced by the PR, which is now fixed. The PR is now passing the Windows CI and builds for Win32 and x64 with Visual Studio 2015 on my development machine.

@gilles-peskine-arm / @Patater - please review and approve as you see fit.

Please note I've also removed the obsolete Visual Studio 6 project files.

gilles-peskine-arm

Blocker: scripts/generate_visualc_files.pl modifies some files, the VS files are not aligned with template. The culprit is commit b862f4c which should have included the template as well.

I also don't understand the reason for b862f4c. In the new commit message, please explain why the change was done. Feel free to amend b862f4c.

Patater · 2018-03-27T09:22:48Z

@gilles-peskine-arm b862f4c is basically a fixup to "Replace Windows APIs that are banned in Windows Store apps" (e241eb0). "Replace Windows APIs that are banned in Windows Store apps" introduces a regression by incorrectly re-adding mbedTLS.lib to the vcproj files, which we had removed to fix bug #1347. The commit history would be simpler and easier to understand if b862f4c were applied as a fixup to e241eb0, but that makes reviewing with GitHub more difficult.

gilles-peskine-arm · 2018-03-27T09:35:33Z

Thanks @Patater . Rather than change an old commit, which will make the history inaccurate and makes it harder to diagnose problems later, I'd rather have a fix-up commit that says ”do <such-and-such>. This reverts the incorrect change made in commit 1234 which caused <problem>.”

simonbutcher · 2018-12-24T15:38:05Z

This PR failed in my attempts to merge it causing the CI release job to fail. I've rebased the PR (will need re-approval - sorry), and set off a new CI release job #894.

Demoting to 'needs: work'.

simonbutcher · 2018-12-25T00:25:18Z

The CI job #894 all.sh failed after the rebase with the following errors:

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
FAILED: 4
build: Windows cross build - mingw64, make (Link Library): command 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
build: Windows cross build - mingw64, make (Link Library): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror WINDOWS_BUILD=1 tests -> 2
build: Windows cross build - mingw64, make (DLL): command 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 -> 2
build: Windows cross build - mingw64, make (DLL): command 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 -> 2
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

This passed before the rebase.

simonbutcher · 2018-12-27T12:15:56Z

I've tested with the Docker image in the CI, and the issue is the version of mingw is too old, being i686-w64-mingw32-gcc (GCC) 5.3.1 20160211.

This is too old, and even Ubuntu 17.10 currently has a package version of i686-w64-mingw32-gcc (GCC) 6.3.0 20170415.

This issue is with the CI and needs to be updated to merge this PR.

simonbutcher · 2019-02-14T16:25:54Z

retest

joycepg · 2019-06-07T05:13:39Z

i'm sad this PR hasn't been merged yet, but since it hasn't, i thought i'd point out an alternative that causes less changed files.
the change to bcrypt api in entropy_poll.c required many changes in the makefiles, project files, cmake files, etc. instead of all these explicit reference to link against bcrypt.lib, adding this one line:

#pragma comment(lib,"bcrypt.lib")

into entropy_poll.c (eg just after #include <bcrypt.h>) causes the dependency on bcrypt.lib to be noted inside entropy_poll.obj, and therefore it is pulled in by the linker automatically.

ackh · 2019-11-16T16:57:00Z

Is there any chance that this change ends up in a release any time soon? What is blocking it? Can I do something to help out?

simonbutcher · 2020-01-06T18:14:17Z

I'll pick this PR up again, rebase it and resubmit it. Hopefully we can get it in this time.

despair86 · 2020-01-06T18:19:10Z

@joycepg that trick only works for Microsoft C.

mpg · 2020-03-26T10:43:12Z

Note: there was a PR on the crypto side that may or may not contain changes that need to be picked up here now: ARMmbed/mbed-crypto#341

Zopolis4 · 2022-07-22T12:04:23Z

Any news on this? I'm trying to build a project that uses mbedtls for UWP.

akien-mga · 2022-07-22T12:23:44Z

For the record, we've been using that PR downstream in Godot Engine for a few years for our UWP build. Here's a diff of this PR compatible with mbedTLS 2.28.1: https://github.com/godotengine/godot/blob/master/thirdparty/mbedtls/patches/1453.diff

Our UWP port doesn't see a lot of use though so I can't attest for how well this solves the issue, and whether mbedTLS works properly on UWP.

akien-mga · 2023-04-18T08:35:39Z

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.

The lstrlenW part seems to have been resolved by #7081 (in 3.4.0) and #7086 (in 2.28.3).

Not sure what's the state with CryptGenRandom on UWP nowadays, for now I'll keep that part of the patch but I'll see if it can be dropped too.

Edit: A workaround for CryptGenRandom is still needed, doesn't link with it:

modules.uwp.opt.32.x86.lib(entropy_poll.uwp.opt.32.x86.obj) : error LNK2001: unresolved external symbol _CryptAcquireContext
modules.uwp.opt.32.x86.lib(entropy_poll.uwp.opt.32.x86.obj) : error LNK2001: unresolved external symbol _CryptGenRandom

robUx4 · 2023-06-26T04:45:06Z

For the record, CryptGenRandom is allowed in UWP since Windows 19H1.

akien-mga · 2023-10-27T08:29:48Z

Superseded by #8047 I believe?

simonbutcher mentioned this pull request Mar 14, 2018

Replace Windows APIs that are banned in Windows Store apps #730

Closed

simonbutcher added CLA valid needs-review Every commit must be reviewed by at least two team members, labels Mar 14, 2018

simonbutcher requested a review from Patater March 14, 2018 17:04

gilles-peskine-arm self-requested a review March 14, 2018 17:07

gilles-peskine-arm reviewed Mar 14, 2018

View reviewed changes

gilles-peskine-arm approved these changes Mar 14, 2018

View reviewed changes

gilles-peskine-arm approved these changes Mar 15, 2018

View reviewed changes

simonbutcher self-assigned this Mar 21, 2018

mazimkhan mentioned this pull request Mar 21, 2018

replace win32 functions that are not allowed in windows 10 apps #635

Closed

simonbutcher requested review from Patater and removed request for Patater March 22, 2018 13:13

Patater approved these changes Mar 23, 2018

View reviewed changes

simonbutcher added the needs-work label Mar 23, 2018

simonbutcher removed the needs-work label Mar 25, 2018

Patater previously approved these changes Mar 26, 2018

View reviewed changes

gilles-peskine-arm previously requested changes Mar 26, 2018

View reviewed changes

simonbutcher dismissed Patater’s stale review via a587c92 December 24, 2018 15:32

simonbutcher force-pushed the win32-crypto-api-fix branch from cbbc28e to a587c92 Compare December 24, 2018 15:32

simonbutcher added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Dec 24, 2018

RonEld added the component-crypto Crypto primitives and low-level interfaces label Feb 18, 2019

Patater mentioned this pull request Apr 17, 2019

all.sh: Require i686-w64-mingw32-gcc version >= 6 #2586

Merged

simonbutcher mentioned this pull request Jan 6, 2020

Update Windows Crypto APIs ARMmbed/mbed-crypto#341

Closed

1 task

This was referenced Aug 11, 2020

UWP has no lstrlenW function #2994

Closed

Linker error on Windows UWP #1227

Closed

simonbutcher removed their assignment Nov 9, 2020

daverodgman removed approved for design labels May 13, 2022

akien-mga mentioned this pull request Apr 18, 2023

mbedtls: Update to upstream version 2.28.3 godotengine/godot#76200

Merged

davidhorstmann-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label May 18, 2023

minosgalanakis mentioned this pull request Aug 9, 2023

Bugfix/update win crypto apis #8047

Merged

3 tasks

Update Windows Crypto APIs #1453

Are you sure you want to change the base?

Update Windows Crypto APIs #1453

Conversation

simonbutcher commented Mar 14, 2018 • edited Loading

Description

Status

Requires Backporting

Migrations

Todos

gilles-peskine-arm Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

simonbutcher Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irwir Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irwir Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Mar 14, 2018 • edited Loading

simonbutcher commented Mar 14, 2018

simonbutcher commented Mar 14, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

simonbutcher commented Mar 15, 2018

Patater commented Mar 23, 2018

Patater left a comment

Choose a reason for hiding this comment

Patater commented Mar 23, 2018 • edited Loading

simonbutcher commented Mar 25, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Patater commented Mar 27, 2018

gilles-peskine-arm commented Mar 27, 2018

simonbutcher commented Dec 24, 2018 • edited Loading

simonbutcher commented Dec 25, 2018 • edited Loading

simonbutcher commented Dec 27, 2018

simonbutcher commented Feb 14, 2019

joycepg commented Jun 7, 2019

ackh commented Nov 16, 2019

simonbutcher commented Jan 6, 2020

despair86 commented Jan 6, 2020

mpg commented Mar 26, 2020

Zopolis4 commented Jul 22, 2022

akien-mga commented Jul 22, 2022

akien-mga commented Apr 18, 2023 • edited Loading

robUx4 commented Jun 26, 2023

akien-mga commented Oct 27, 2023

simonbutcher commented Mar 14, 2018 •

edited

Loading

gilles-peskine-arm Mar 14, 2018 •

edited

Loading

simonbutcher Mar 14, 2018 •

edited

Loading

simonbutcher Mar 14, 2018 •

edited

Loading

irwir Mar 15, 2018 •

edited

Loading

irwir Mar 16, 2018 •

edited

Loading

gilles-peskine-arm commented Mar 14, 2018 •

edited

Loading

Patater commented Mar 23, 2018 •

edited

Loading

simonbutcher commented Dec 24, 2018 •

edited

Loading

simonbutcher commented Dec 25, 2018 •

edited

Loading

akien-mga commented Apr 18, 2023 •

edited

Loading