[MSGINA][WINLOGON] Carefully zero password memory buffers before freeing them (reactos#8172)

Including variables containing pointers to a password buffer and its lengths.

Author: HBelusca

base/system/winlogon/sas.c

@@ -541,9 +541,19 @@ FreeWlxMprInfo(
     if (MprNotifyInfo->pszDomain)
         LocalFree(MprNotifyInfo->pszDomain);
     if (MprNotifyInfo->pszPassword)
+    {
+        /* Zero out the password buffer before freeing it */
+        SIZE_T pwdLen = (wcslen(MprNotifyInfo->pszPassword) + 1) * sizeof(WCHAR);
+        SecureZeroMemory(MprNotifyInfo->pszPassword, pwdLen);
         LocalFree(MprNotifyInfo->pszPassword);
+    }
     if (MprNotifyInfo->pszOldPassword)
+    {
+        /* Zero out the password buffer before freeing it */
+        SIZE_T pwdLen = (wcslen(MprNotifyInfo->pszOldPassword) + 1) * sizeof(WCHAR);
+        SecureZeroMemory(MprNotifyInfo->pszOldPassword, pwdLen);
         LocalFree(MprNotifyInfo->pszOldPassword);
+    }
 }
 
 static

dll/win32/msgina/gui.c

@@ -622,7 +622,7 @@ DoChangePassword(
                            MB_OK | MB_ICONEXCLAMATION,
                            IDS_CHANGEPWDTITLE,
                            IDS_NONMATCHINGPASSWORDS);
-        return FALSE;
+        goto done;
     }
 
     /* Calculate the request buffer size */
@@ -639,7 +639,7 @@ DoChangePassword(
     if (RequestBuffer == NULL)
     {
         ERR("HeapAlloc failed\n");
-        return FALSE;
+        goto done;
     }
 
     /* Initialize the request buffer */
@@ -729,13 +729,27 @@ DoChangePassword(
         (wcscmp(Domain, pgContext->DomainName) == 0) &&
         (wcscmp(OldPassword, pgContext->Password) == 0))
     {
-        ZeroMemory(pgContext->Password, sizeof(pgContext->Password));
+        SecureZeroMemory(pgContext->Password, sizeof(pgContext->Password));
         wcscpy(pgContext->Password, NewPassword1);
     }
 
 done:
+    /* Zero out the password buffers */
+    SecureZeroMemory(&OldPassword, sizeof(OldPassword));
+    SecureZeroMemory(&NewPassword1, sizeof(NewPassword1));
+    SecureZeroMemory(&NewPassword2, sizeof(NewPassword2));
+
     if (RequestBuffer != NULL)
+    {
+        /* Zero out the password buffers before freeing them */
+        SecureZeroMemory(RequestBuffer->OldPassword.Buffer,
+                         RequestBuffer->OldPassword.MaximumLength);
+        SecureZeroMemory(&RequestBuffer->OldPassword, sizeof(RequestBuffer->OldPassword));
+        SecureZeroMemory(RequestBuffer->NewPassword.Buffer,
+                         RequestBuffer->NewPassword.MaximumLength);
+        SecureZeroMemory(&RequestBuffer->NewPassword, sizeof(RequestBuffer->NewPassword));
         HeapFree(GetProcessHeap(), 0, RequestBuffer);
+    }
 
     if (ResponseBuffer != NULL)
         LsaFreeReturnBuffer(ResponseBuffer);
@@ -1143,20 +1157,23 @@ DoLogon(
                  (SubStatus == STATUS_PASSWORD_EXPIRED))
         {
             if (SubStatus == STATUS_PASSWORD_MUST_CHANGE)
+            {
                 ResourceMessageBox(pgContext,
                                    hwndDlg,
                                    MB_OK | MB_ICONSTOP,
                                    IDS_LOGONTITLE,
                                    IDS_PASSWORDMUSTCHANGE);
+            }
             else
+            {
                 ResourceMessageBox(pgContext,
                                    hwndDlg,
                                    MB_OK | MB_ICONSTOP,
                                    IDS_LOGONTITLE,
                                    IDS_PASSWORDEXPIRED);
+            }
 
-            if (!OnChangePassword(hwndDlg,
-                                  pgContext))
+            if (!OnChangePassword(hwndDlg, pgContext))
                 goto done;
 
             Status = DoLoginTasks(pgContext,
@@ -1167,7 +1184,6 @@ DoLogon(
             if (!NT_SUCCESS(Status))
             {
                 TRACE("Login after password change failed! (Status 0x%08lx)\n", Status);
-
                 goto done;
             }
         }
@@ -1220,7 +1236,7 @@ DoLogon(
         goto done;
     }
 
-    ZeroMemory(pgContext->Password, sizeof(pgContext->Password));
+    SecureZeroMemory(pgContext->Password, sizeof(pgContext->Password));
     wcscpy(pgContext->Password, Password);
 
     result = TRUE;
@@ -1232,7 +1248,12 @@ DoLogon(
         HeapFree(GetProcessHeap(), 0, UserName);
 
     if (Password != NULL)
+    {
+        /* Zero out the password buffer before freeing it */
+        SIZE_T pwdLen = (wcslen(Password) + 1) * sizeof(WCHAR);
+        SecureZeroMemory(Password, pwdLen);
         HeapFree(GetProcessHeap(), 0, Password);
+    }
 
     if (Domain != NULL)
         HeapFree(GetProcessHeap(), 0, Domain);
@@ -1542,7 +1563,12 @@ DoUnlock(
         HeapFree(GetProcessHeap(), 0, UserName);
 
     if (Password != NULL)
+    {
+        /* Zero out the password buffer before freeing it */
+        SIZE_T pwdLen = (wcslen(Password) + 1) * sizeof(WCHAR);
+        SecureZeroMemory(Password, pwdLen);
         HeapFree(GetProcessHeap(), 0, Password);
+    }
 
     return res;
 }

dll/win32/msgina/lsa.c

@@ -130,19 +130,23 @@ MyLogonUser(
     AuthInfo->UserName.MaximumLength = UserName.MaximumLength;
     AuthInfo->UserName.Buffer = (PWCHAR)Ptr;
     if (UserName.MaximumLength > 0)
+    {
         RtlCopyMemory(AuthInfo->UserName.Buffer,
                       UserName.Buffer,
                       UserName.MaximumLength);
+    }
 
     Ptr += UserName.MaximumLength;
 
     AuthInfo->Password.Length = Password.Length;
     AuthInfo->Password.MaximumLength = Password.MaximumLength;
     AuthInfo->Password.Buffer = (PWCHAR)Ptr;
     if (Password.MaximumLength > 0)
+    {
         RtlCopyMemory(AuthInfo->Password.Buffer,
                       Password.Buffer,
                       Password.MaximumLength);
+    }
 
     /* Create the Logon SID*/
     AllocateLocallyUniqueId(&LogonId);
@@ -232,21 +236,18 @@ MyLogonUser(
     TRACE("Luid: 0x%08lx%08lx\n", Luid.HighPart, Luid.LowPart);
 
     if (TokenHandle != NULL)
-    {
         TRACE("TokenHandle: %p\n", TokenHandle);
-    }
 
     *phToken = TokenHandle;
 
 done:
+    SecureZeroMemory(&Password, sizeof(Password));
+
     if (ProfileBuffer != NULL)
         LsaFreeReturnBuffer(ProfileBuffer);
 
-    if (!NT_SUCCESS(Status))
-    {
-        if (TokenHandle != NULL)
-            CloseHandle(TokenHandle);
-    }
+    if (!NT_SUCCESS(Status) && (TokenHandle != NULL))
+        CloseHandle(TokenHandle);
 
     if (TokenGroups != NULL)
         RtlFreeHeap(RtlGetProcessHeap(), 0, TokenGroups);
@@ -258,7 +259,12 @@ MyLogonUser(
         RtlFreeSid(LogonSid);
 
     if (AuthInfo != NULL)
+    {
+        /* Zero out the password buffers before freeing */
+        SecureZeroMemory(AuthInfo->Password.Buffer, AuthInfo->Password.MaximumLength);
+        SecureZeroMemory(&AuthInfo->Password, sizeof(AuthInfo->Password));
         RtlFreeHeap(RtlGetProcessHeap(), 0, AuthInfo);
+    }
 
     return Status;
 }

dll/win32/msgina/msgina.c

@@ -1071,7 +1071,7 @@ WlxLogoff(
     TRACE("WlxLogoff(%p)\n", pWlxContext);
 
     /* Delete the password */
-    ZeroMemory(pgContext->Password, sizeof(pgContext->Password));
+    SecureZeroMemory(pgContext->Password, sizeof(pgContext->Password));
 
     /* Close the user token */
     CloseHandle(pgContext->UserToken);