ryanries / PassFiltEx

PassFiltEx. An Active Directory Password Filter.
GNU General Public License v3.0
264 stars 50 forks source link

Question: SecureZeroMemory #13

Closed x43x61x69 closed 5 years ago

x43x61x69 commented 5 years ago

Hi there,

I'm new to Windows API. I read on the Microsoft Doc that password buffer should be securely zero out once used. I saw the related code were comment out.

Is there a particular reason not to have not enabled? (And I noticed that if they were in place, the Test program will quit on the end of every password attempt.)

ryanries commented 5 years ago

Hello,

I believe I have addressed your concerns as of today's push and new release: https://github.com/ryanries/PassFiltEx/releases/tag/v1.1.7

I remember I commented out that code when I moved from allocating PasswordCopy on the heap, to allocating it on the stack. Anyway, I am RtlSecureZeroMemory'ing both Password and PasswordCopy now before leaving the function.

Thanks!

BobVul commented 5 years ago

Whether Password should be zeroed is a bit questionable and is possibly a documentation error.


PSAM_PASSWORD_FILTER_ROUTINE callback function docs say:

Password

Pointer to a UNICODE_STRING that represents the new plaintext password. When you have finished using the password, clear it from memory by calling the SecureZeroMemory function. For more information on protecting the password, see Handling Passwords.

Easy, right?

Nope.

Password Filter Programming Considerations says:

All buffers passed into password notification and filter routines should be treated as read-only. Writing data to these buffers may cause unstable behavior.

Password is ... a buffer passed into the filter routine. It should never be written to, and this includes zeroing it.

There's a bit more discussion in this blog, where apparently they opened a support case and Microsoft confirmed that:

What they meant was “If you have created copies of the password anywhere within your code, make sure you erase memory used to store those copies by calling SecureZeroMemory …”

This also matches what I would normally expect: if the caller created and put the value in the buffer, the caller should be the one handling cleanup of the buffer, usually.

The author of that post also raised an issue against OpenPasswordFilter, https://github.com/jephthai/OpenPasswordFilter/issues/4


Now it's possible that whoever answered that support case is wrong. The doc in question hasn't been updated. But we've got conflicting advice here...

Though I suppose you'd be in a better position to confirm internally, Ryan :)

ryanries commented 5 years ago

@BobVul Issue has been re-opened while I investigate.

In my modest amount of testing, the filter seemed to set passwords correctly regardless of whether I performed a SecureZeroMemory or not, however, now that I think about it, I might be making it impossible for the next password filter in the chain, should the user have multiple password filters installed simultaneously. I should be able to get the definitive answer, though it might take a few weeks as I'll be out of town starting next week.

ryanries commented 5 years ago

I finished my code review, and I can confirm that we should not be clearing the buffer that Windows passes into our function. However, we should (of course) be clearing any copies of the password that we've made. I'll see about getting the MSDN documentation updated for clarity.

BobVul commented 5 years ago

Thanks for confirming!