Closed deunlee closed 4 years ago
I'm sorry for missing commit log. If you want, I'll change it and rewrite pr.
Update util.c -> Fix zero fill function for Visual Studio
Thanks for your PR. However, because unless dealing with hardware volatile
essentially never means what the programmer intended, I googled around a bit to refresh my memory.
In particular this article concludes that there's no way to do this with standard code.
C11 defines memset_s()
, but as optional. Does your visual studio support that? Otherwise there's SecureZeroMemory()
.
Looks to me like the best solution is something like:
void
explicit_bzero(…)
{
#ifdef HAVE_MEMSET_S
memset_s(…);
#elif defined(HAVE_SECUREZEROMEMORY)
SecureZeroMemory(…)
#else
asm …
#endif
}
plus of course includes and configure.ac
changes.
Visual Studio has several optimization options.
I tested this simple code with each option.
void memset_volatile(volatile void *s, size_t len)
{
volatile char *p = (char*)s;
while (len--) {
*p++ = '\0';
}
}
int copy_and_sum(int arr[], int size)
{
int i, sum = 0, temp[100000];
std::cout << "temp addr = " << temp << std::endl;
memcpy(temp, arr, size * sizeof(int));
for (i = 0; i < size; i++)
sum += temp[i];
// memset(temp, 0, sizeof(temp));
// SecureZeroMemory(temp, sizeof(temp));
memset_volatile(temp, sizeof(temp));
return sum;
}
And I analyzed the compiled binary(64-bit) with x64dbg.
I looked up the header for SecureZeroMemory.
In x86_64 it looks like this, and in the rest it is implemented similarly to my code.
#define SecureZeroMemory RtlSecureZeroMemory
FORCEINLINE
PVOID
RtlSecureZeroMemory(
_Out_writes_bytes_all_(cnt) PVOID ptr,
_In_ SIZE_T cnt
)
{
volatile char *vptr = (volatile char *)ptr;
__stosb((PBYTE )((DWORD64)vptr), 0, cnt);
return ptr;
}
But your method also seems good. I think similar code doesn't have to be rewritten.
I noticed that there are many other things that need to be changed to make it work in Visual Studio.
If you do not plan to port to Windows, I will fork and fix it in my repo.
I don't want to take your time too much.
Thanks for reading.
I'll accept good pull requests that add Windows support.
Testing what gets optimized away is one thing, but in the end it must be guaranteed by the standard. You've tested one compiler with a few options, in 2020. But on another platform, or the same platform in 2030 another compiler (version) may do something else.
What you show and test with volatile
works (under your test conditions), but it's still undefined behaviour. If Microsoft improves their optimizer then this code will silently stop doing what you want.
It seems the only way to zero out secrets, without relying on undefined or unspecified behaviour, which one should never do, is to use dedicated functions like explicit_bzore
, memset_s
, SecureZeroMemory
, or implementation-defined (meaning defined) behaviour such as asm
or similar.
If you do fork, please don't use undefined behaviour as there are many many cases of that coming back to bite years after.
Thank you for permission! And I totally agree with you. I've only tried one compiler and don't know how it will change later.
Windows does not support PAM. As you know it is a Linux feature. Instead, Windows supports CP(Credential Provider). It's a feature that allows customizing logon options.
Here is an official sample of Microsoft.
But I'm not sure it's desirable for the code to be integrated this repo because CP code is so different from PAM. Also I don't know if I can implement it completely securely.
However I will try in my repo and then I will give you the result.
Thank you for comment.
Hi,
I used this code and found that "asm" keyword was incompatible with some compilers (especially Visual Studio).
So I fixed explicit_bzero() directly using "while" and here is the result.
Thanks.