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.

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

How to use macro for mbedtls_platform_zeroize() in Windows? #8490

Open
irwir opened this issue Nov 7, 2023 · 12 comments
Open

How to use macro for mbedtls_platform_zeroize() in Windows? #8490

irwir opened this issue Nov 7, 2023 · 12 comments
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)

Comments

@irwir
Copy link
Contributor

irwir commented Nov 7, 2023

* if your platform has explicit_bzero() but `platform_util.c` does not
* detect its presence, define `MBEDTLS_PLATFORM_ZEROIZE_ALT` to be
* `explicit_bzero` to use that function as mbedtls_platform_zeroize().

Suppose the goal is to replace mbedtls_platform_zeroize with RtlSecureZeroMemory in MSVC.
The quoted text hints it is sufficient to write:
#define MBEDTLS_PLATFORM_ZEROIZE_ALT RtlSecureZeroMemory
Is this correct?

@tom-cosgrove-arm
Copy link
Contributor

What's wrong with the SecureZeroMemory() it already uses on Windows? (Which is exactly the same, if I read the documentation correctly)

@irwir
Copy link
Contributor Author

irwir commented Nov 7, 2023

SecureZeroMemory is just a macro that expands to RtlSecureZeroMemory.
The idea is to avoid enveloped calls to the function and thus allow the compiler to make smaller and faster code.

@gilles-peskine-arm
Copy link
Contributor

it is sufficient to write:
#define MBEDTLS_PLATFORM_ZEROIZE_ALT RtlSecureZeroMemory
Is this correct?

Yes, with one subtlety: MBEDTLS_PLATFORM_ZEROIZE_ALT(NULL, 0) must be valid (and do nothing). I'm not sure if we actually need that at the moment on platforms where calloc(0, 0) doesn't return NULL, but there's a lot of code in the library that accepts a null pointer for a zero-sized buffer.

By default, on Windows, mbedtls_platform_zeroize calls SecureZeroMemory, but only if the size is nonzero.

@irwir
Copy link
Contributor Author

irwir commented Nov 7, 2023

Yes, with one subtlety: MBEDTLS_PLATFORM_ZEROIZE_ALT(NULL, 0) must be valid (and do nothing).

With this definition the library compiles:
#define MBEDTLS_PLATFORM_ZEROIZE_ALT RtlSecureZeroMemory
Then building ssl_mail_client gives 46 link errors: unresolved external symbol mbedtls_platform_zeroize.

@gilles-peskine-arm
Copy link
Contributor

Then building ssl_mail_client gives 46 link errors

That's odd. Is it just this program and no other? What build system are you using?

We do one test build with MBEDTLS_PLATFORM_ZEROIZE_ALT defined. It's on linux, with make.

@irwir
Copy link
Contributor Author

irwir commented Nov 7, 2023

Windows 11, Visual Studio 2022, library 3.5.0.

Edit. The symbol is undefined in the library, hence any program that needs to zeroize memory cannot be built.

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Nov 7, 2023

Hmm, the doc says "Uncomment the macro to let Mbed TLS use your alternate implementation of mbedtls_platform_zeroize()", so - unlike many of the others - it's not #defined to the name of the function to use, but instead it gets #defined and then a separate mbedtls_platform_zeroize() function must be provided.

While the documentation goes on to say "For example, define MBEDTLS_PLATFORM_ZEROIZE_ALT to be explicit_bzero", that part is incorrect.

To actually use it as a #define, you'd have to do like we do in user-config-zeroize-memset.h, something like

#define mbedtls_platform_zeroize(buf, len) RtlSecureZeroMemory(buf, len)

and ensure the header needed for RtlSecureZeroMemory() is included everywhere it is needed

@irwir
Copy link
Contributor Author

irwir commented Nov 7, 2023

Has it ever been tested in Windows?

@irwir
Copy link
Contributor Author

irwir commented Nov 8, 2023

Yes, with one subtlety: MBEDTLS_PLATFORM_ZEROIZE_ALT(NULL, 0) must be valid (and do nothing). I'm not sure if we actually need that at the moment on platforms where calloc(0, 0) doesn't return NULL, but there's a lot of code in the library that accepts a null pointer for a zero-sized buffer.

Too much time passed since experiments with a pointer to memset trick, but probably this additional check for a null pointer was required for defeating optimizers - VS C++ compiler still was willing to skip memory cleaning otherwise.

@irwir
Copy link
Contributor Author

irwir commented Nov 8, 2023

While the documentation goes on to say "For example, define MBEDTLS_PLATFORM_ZEROIZE_ALT to be explicit_bzero", that part is incorrect.

Agreed.

To actually use it as a #define, you'd have to do like we do in user-config-zeroize-memset.h, something like

#define mbedtls_platform_zeroize(buf, len) RtlSecureZeroMemory(buf, len)

Also this part should be taken into account:

#if !defined(MBEDTLS_TEST_DEFINES_ZEROIZE) //no-check-names

If mbedtls_platform_zeroize macro was defined, then this block either declares an undefined external symbol or redeclares the target cleaner - RtlSecureZeroMemory in this case.

and ensure the header needed for RtlSecureZeroMemory() is included everywhere it is needed

https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlsecurezeromemory
Including Windows headers in random places and/or in wrong order leads to failed compilation.

@gilles-peskine-arm gilles-peskine-arm added bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) labels Nov 8, 2023
@gilles-peskine-arm
Copy link
Contributor

I'm labeling this [bug] with the objective of fixing the documentation. I want to make the platform interfaces consistent, but that's an API change which has to wait for a major version. Hopefully we'll do this in 4.0.

@irwir
Copy link
Contributor Author

irwir commented Nov 8, 2023

I'm labeling this [bug] with the objective of fixing the documentation.

Should the code be made at least working in more than a trivial test case? This might be beneficial even when the drastic future changes were applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)
Projects
Status: No status
Development

No branches or pull requests

3 participants