Opened 4 years ago

Closed 4 years ago

#17986 closed enhancement (implemented)

Use SecureZeroMemory in memwipe on Windows

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: windows
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #7419, we use explicit_bzero/memset_s in memwipe when available.

Windows has SecureZeroMemory with the same semantics in Windows.h.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx

As long as we build our Windows binaries with headers that contain SecureZeroMemory, the code will work on any Windows version:

"This function is defined as the RtlSecureZeroMemory function (see WinBase.h). The implementation of RtlSecureZeroMemory is provided inline and can be used on any version of Windows (see WinNT.h.)"

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by teor

Summary: Use SecureZeroMemory for WindUse SecureZeroMemory in memwipe on Windows

comment:3 Changed 4 years ago by teor

Looks great, I don't have Windows so can't compile it, but it seems simple enough, let's get it tested, then merged.

This is going to require a manual merge with #7419 that preserves all the different cases.

(Edit: explain *why* I didn't compile it)

Last edited 4 years ago by teor (previous) (diff)

comment:4 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged into master after #7419, with a tiny amount of cleanup.

comment:5 Changed 4 years ago by teor

Resolution: fixed
Status: closedreopened

Jenkins says:

../tor/src/common/crypto.c: In function 'memwipe':
22:31:43 ../tor/src/common/crypto.c:2965:3: error: implicit declaration of function 'SecureZeroMemory' [-Werror=implicit-function-declaration]
22:31:43 ../tor/src/common/crypto.c:2965:3: error: nested extern declaration of 'SecureZeroMemory' [-Werror=nested-externs]

https://jenkins.torproject.org/job/tor-ci-windows/1795/consoleFull

Microsoft says we need to include Windows.h (which includes WinBase.h) to get this function.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx

comment:6 Changed 4 years ago by nickm

Strange! Aren't we already including windows.h ?

comment:7 in reply to:  6 Changed 4 years ago by teor

Replying to nickm:

Strange! Aren't we already including windows.h ?

MinGW's WinBase.h doesn't contain SecureZeroMemory.
(Their other memory setting functions #define down to memset, so this is probably just as well.)

We'll need to AC_CHECK_FUNC for SecureZeroMemory in configure, like in #7419. (Do we have to include windows.h explicitly, or will configure pick it up automatically?)

comment:8 Changed 4 years ago by nickm

Watch out: AC_CHECK_FUNC doesn't check whether the function is declared at all; it just checks whether it's linkable. We might also need AC_CHECK_FUNC_DECL. Either way we should probably include windows.h.

comment:9 Changed 4 years ago by teor

The function is defined inline in the header, there's no linker symbol for it. So we need AC_CHECK_FUNC_DECL and *not* AC_CHECK_FUNC.

comment:10 Changed 4 years ago by nickm

Resolution: implemented
Status: reopenedclosed

I've tried to fix it with 1d6dd288e1c084a5118785899cca910e8c69fbb1; let's see if that works

Note: See TracTickets for help on using tickets.