Review fixes: installer lockout, LSA contracts, lsass-safe AP, tests for the lsass code

.github/workflows/build.yml

@@ -23,6 +23,17 @@ jobs:
         shell: cmd
         run: build.bat
 
+      - name: Parse every PowerShell script
+        shell: pwsh
+        run: |
+          $bad = 0
+          foreach ($f in Get-ChildItem -Filter *.ps1) {
+            $tokens = $null; $errors = $null
+            [System.Management.Automation.Language.Parser]::ParseFile($f.FullName, [ref]$tokens, [ref]$errors) | Out-Null
+            foreach ($e in $errors) { Write-Host "$($f.Name):$($e.Extent.StartLineNumber): $($e.Message)"; $bad++ }
+          }
+          if ($bad) { throw "$bad PowerShell parse error(s)" }
+
       - name: Check that every binary has Control Flow Guard
         shell: pwsh
         run: |
@@ -37,16 +48,26 @@ jobs:
         run: >
           cl /nologo /c /analyze /W4 /EHsc /std:c++17 /DUNICODE /D_UNICODE
           dll.cpp CTacProvider.cpp CTacCredential.cpp CTacFilter.cpp helpers.cpp
-          totp.cpp store.cpp verify.cpp statelock.cpp eventlog.cpp enroll.cpp
-          ap.cpp subauth.cpp
+          totp.cpp store.cpp store_index.cpp verify.cpp statelock.cpp eventlog.cpp
+          enroll.cpp ap.cpp subauth.cpp
+
+      # The code that runs inside lsass gets no free pass: a finding there fails
+      # the build. SDK headers are excluded from the analysis, so only our own
+      # code can trip it.
+      - name: Static analysis of the lsass code (/analyze /WX)
+        shell: cmd
+        run: >
+          cl /nologo /c /analyze /analyze:external- /external:anglebrackets /external:W0
+          /W4 /WX /EHsc /std:c++17 /DUNICODE /D_UNICODE
+          ap.cpp subauth.cpp store_index.cpp
 
   tests:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
       - name: Install OpenSSL headers
         run: sudo apt-get update && sudo apt-get install -y libssl-dev
-      - name: Native C++ tests (real totp.cpp / verify.cpp)
+      - name: Native C++ tests (totp/verify, and the lsass code under ASan/UBSan)
         run: make -C tests/native
       - name: Python RFC 6238 check
         run: python3 tests/totp_check.py

.gitignore

@@ -20,5 +20,6 @@ __pycache__/
 # Created by new-guids.ps1
 unregister-old.reg
 
-# Native test binary
+# Native test binaries
 tests/native/test_verify
+tests/native/test_lsa

CMakeLists.txt

@@ -12,7 +12,7 @@ set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 # These binaries run in LogonUI and lsass: Control Flow Guard and SDL checks, warning level 4.
 if(MSVC)
     add_compile_options(/guard:cf /sdl /W4)
-    add_link_options(/guard:cf)
+    add_link_options(/guard:cf /CETCOMPAT)
 endif()
 
 # --- the credential provider + filter DLL ---
@@ -24,6 +24,7 @@ add_library(TacProvider SHARED
     helpers.cpp
     totp.cpp
     store.cpp
+    store_index.cpp
     verify.cpp
     statelock.cpp
     eventlog.cpp)
@@ -40,6 +41,7 @@ add_executable(enroll
     enroll.cpp
     totp.cpp
     store.cpp
+    store_index.cpp
     statelock.cpp)
 
 target_link_libraries(enroll PRIVATE
@@ -48,13 +50,13 @@ target_link_libraries(enroll PRIVATE
 # --- the LSA authentication package (SKELETON) ---
 # Runs inside lsass.exe. A bug here can leave the machine unbootable.
 # Load only in a throwaway VM with a snapshot. See ap.h and the article.
+# Links only the index reads (store_index.cpp): no DPAPI or TOTP code in lsass.
 add_library(TacAuthPackage SHARED
     ap.cpp
-    store.cpp
-    totp.cpp)
+    store_index.cpp)
 
 target_link_libraries(TacAuthPackage PRIVATE
-    advapi32 crypt32 bcrypt)
+    advapi32)
 
 set_target_properties(TacAuthPackage PROPERTIES
     LINK_FLAGS "/DEF:\"${CMAKE_CURRENT_SOURCE_DIR}/TacAuthPackage.def\"")
@@ -65,11 +67,10 @@ set_target_properties(TacAuthPackage PROPERTIES
 # Same warnings as above: lsass.exe, VM with snapshot only.
 add_library(TacSubAuth SHARED
     subauth.cpp
-    store.cpp
-    totp.cpp)
+    store_index.cpp)
 
 target_link_libraries(TacSubAuth PRIVATE
-    advapi32 crypt32 bcrypt)
+    advapi32)
 
 set_target_properties(TacSubAuth PROPERTIES
     LINK_FLAGS "/DEF:\"${CMAKE_CURRENT_SOURCE_DIR}/TacSubAuth.def\"")

CTacFilter.cpp

@@ -2,6 +2,7 @@
 #include "guid.h"
 #include "dll.h"
 #include "store.h"
+#include <ntsecapi.h>
 #include <new>
 
 IFACEMETHODIMP_(ULONG) CTacFilter::AddRef()
@@ -56,18 +57,35 @@ IFACEMETHODIMP CTacFilter::Filter(CREDENTIAL_PROVIDER_USAGE_SCENARIO cpus, DWORD
 
     // Nothing enrolled: hiding the other tiles would only lock everyone out.
     // A store that cannot be read is NOT treated as empty - filter as usual.
-    bool any = false;
-    if (tac::QueryAnyEnrollment(any) && !any)
+    // QueryAnyEnrollment counts stored secrets, not registry values, so a
+    // leftover value of another kind cannot make us hide every way in while
+    // nothing can actually authenticate.
+    if (!AnyEnrollment())
         return S_OK;
 
     for (DWORD i = 0; i < cProviders; ++i)
         rgbAllow[i] = IsEqualGUID(rgclsidProviders[i], CLSID_CTacProvider);
     return S_OK;
 }
 
+// Is at least one account enrolled? A store that cannot be read counts as
+// "yes", so an unreadable store does not quietly switch the second factor off.
+bool CTacFilter::AnyEnrollment()
+{
+    bool any = false;
+    if (!tac::QueryAnyEnrollment(any))
+        return true;
+    return any;
+}
+
 // RDP with NLA delivers the credential before a tile exists. We forward it and
 // set the target provider to ourselves; copying the incoming CLSID would send
 // it to a provider we just filtered out, and remote logon would break.
+//
+// Only while we are actually filtering, though: with nothing enrolled, Filter()
+// leaves every other provider visible, and taking the credential here would
+// hand it to a tile that cannot authenticate anybody. Then it belongs to
+// whoever it was addressed to.
 IFACEMETHODIMP CTacFilter::UpdateRemoteCredential(
     const CREDENTIAL_PROVIDER_CREDENTIAL_SERIALIZATION* pcpcsIn,
     CREDENTIAL_PROVIDER_CREDENTIAL_SERIALIZATION* pcpcsOut)
@@ -77,6 +95,21 @@ IFACEMETHODIMP CTacFilter::UpdateRemoteCredential(
     ZeroMemory(pcpcsOut, sizeof(*pcpcsOut));
     if (!pcpcsIn || pcpcsIn->cbSerialization == 0 || !pcpcsIn->rgbSerialization)
         return E_NOTIMPL;
+    if (!AnyEnrollment())
+        return E_NOTIMPL;
+
+    // RDP can forward other logon types too, a smartcard logon for example.
+    // Only a password logon has the layout our tile unpacks, so anything else
+    // stays with the provider it was addressed to instead of being taken and
+    // then refused. MessageType is the first member of the packed
+    // KERB_INTERACTIVE_UNLOCK_LOGON.
+    if (pcpcsIn->cbSerialization < sizeof(KERB_INTERACTIVE_UNLOCK_LOGON))
+        return E_NOTIMPL;
+    const KERB_LOGON_SUBMIT_TYPE type =
+        reinterpret_cast<const KERB_INTERACTIVE_UNLOCK_LOGON*>(
+            pcpcsIn->rgbSerialization)->Logon.MessageType;
+    if (type != KerbInteractiveLogon && type != KerbWorkstationUnlockLogon)
+        return E_NOTIMPL;
 
     pcpcsOut->rgbSerialization =
         static_cast<BYTE*>(CoTaskMemAlloc(pcpcsIn->cbSerialization));

CTacFilter.h

@@ -25,5 +25,8 @@ class CTacFilter : public ICredentialProviderFilter
 
 private:
     virtual ~CTacFilter() { DllRelease(); }
+
+    // At least one stored secret, or a store that cannot be read.
+    static bool AnyEnrollment();
     LONG _cRef;
 };

README.md

@@ -25,14 +25,19 @@ use this anywhere, so you don't use the CLSIDs of this repository.
 In an elevated PowerShell on the test VM:
 
 ```powershell
-New-Item -ItemType Directory -Force "C:\Program Files\TheAdminCafe" | Out-Null
-Copy-Item .\TacProvider.dll, .\enroll.exe "C:\Program Files\TheAdminCafe\"
-reg import .\register.reg
+.\install-doppio.ps1
 
-# Enroll every local account you need, including a break-glass admin
+# Enroll every local account that signs in at the console or over RDP,
+# including a break-glass admin
 & "C:\Program Files\TheAdminCafe\enroll.exe" alice
+& "C:\Program Files\TheAdminCafe\enroll.exe" breakglass
 ```
 
+`install-doppio.ps1` copies the files to `C:\Program Files\TheAdminCafe` and
+imports `register.reg`, after checking that nobody else's provider already
+holds those CLSIDs. (By hand: create the folder, copy `TacProvider.dll` and
+`enroll.exe`, `reg import .\register.reg`.)
+
 `enroll.exe` shows the secret and asks for a code from your authenticator app
 before it stores anything, so a secret mistyped into the app can't lock you
 out. Log off, click "Sign-in options", choose the brown **2FA** icon and log on
@@ -41,9 +46,15 @@ with password and code.
 Only when this works for every account, hide all other sign-in options:
 
 ```powershell
-reg import .\register-filter.reg
+.\install-doppio.ps1 -EnableFilter
 ```
 
+It refuses while an enabled local account is not enrolled, because that account
+could not sign in at the console or over RDP afterwards. An account that never
+signs in interactively (a service account) can be named with
+`-AllowUnenrolled svc-backup`. (By hand: `reg import .\register-filter.reg`,
+without that check.)
+
 From then on the logon and unlock screens show **only** the 2FA tiles - no
 password tile, no PIN, no Windows Hello. That is the point: with any of them
 left, the code would be optional. Microsoft advises against it, because a
@@ -52,7 +63,10 @@ broken provider then leaves no way in. What is left for recovery:
 - The 2FA provider always shows an **"Other user"** tile at logon, so an
   account Windows does not list (a hidden break-glass admin) can still sign in.
 - While **no account is enrolled at all**, the filter hides nothing. Importing
-  it too early, or after `enroll.exe /purge`, cannot lock you out.
+  it too early, or after `enroll.exe /purge`, cannot lock you out. Only stored
+  secrets count, not stray registry values - but the secret of an account you
+  deleted without `/remove` does count. `enroll.exe /list` shows it as
+  "(deleted account)"; `enroll.exe /remove <SID>` removes it.
 - If `TacProvider.dll` cannot be loaded, the filter in the same DLL cannot be
   created either, and Windows shows all sign-in options again. Confirm this once
   in the VM by renaming the DLL.
@@ -68,11 +82,15 @@ password. The password check is a `LogonUser` call, so each sign-in leaves one
 extra logon event (4624, type 2, LogonUI.exe) in the Security log.
 
 Unlock a user after too many wrong codes: `enroll.exe /unlock alice`.
-Remove an enrollment again: `enroll.exe /remove alice`. That drops the secret,
-the state and the index entries the LSA packages read. Use it before you delete
-the Windows account; `/remove` still cleans up afterwards, but only if you
-remember the name. After renaming an enrolled account, or after updating from a
-version without the RID index, run `enroll.exe /reindex`.
+See who is enrolled, and whether the LSA indexes know about them:
+`enroll.exe /list`. Remove an enrollment again: `enroll.exe /remove alice`.
+That drops the secret, the state and the index entries the LSA packages read.
+Use it before you delete the Windows account; afterwards, take the SID from
+`/list` and run `enroll.exe /remove S-1-5-21-...`. After renaming an enrolled
+account, or after updating from a version without the RID index, run
+`enroll.exe /reindex`. Every `enroll.exe` run (except `/list` and `/purge`) also
+re-applies the SYSTEM-and-Administrators ACL to existing store keys; the logon
+path no longer does that on every attempt.
 Uninstall: `enroll.exe /purge` (deletes all secrets, state and indexes), then
 `reg import .\unregister.reg`, reboot, delete the folder (it also holds
 `state.lock`, which serializes the state updates of parallel logons). Without
@@ -81,6 +99,9 @@ Uninstall: `enroll.exe /purge` (deletes all secrets, state and indexes), then
 Events (Application log, source "TheAdminCafe 2FA"): 100 password and code
 accepted, 101 wrong code, 102 code already used, 103/104 lock, 105 not
 enrolled, 106 state error, 107 password refused, 108 final result from LSA.
+From the LSA pieces: 200 authentication package loaded (written while LSA
+starts, before the Eventlog service runs, so usually only visible in the
+debugger), 201/202 package denied/declined a logon, 210 filter refused a logon.
 
 ## What the tile does not cover
 
@@ -116,21 +137,40 @@ alone. On a domain member, a GPO that defines these rights overwrites them.
 **2. The LSA authentication package** (`ap.cpp`, `TacAuthPackage.dll`). Runs in
 `lsass.exe` and shows the package interface. It is **not a security control**:
 a logon only reaches it if the caller selects it by package id, and every such
-logon fails anyway, because the package never builds a token. It reads only the
-user name from the logon buffer (bounds-checked), never the password, and asks
-the store with a single registry read whether that name is enrolled - no DPAPI
-decrypt and no name/SID lookup, so nothing re-enters LSA from the logon path.
+logon fails anyway, because the package never builds a token - denied with
+`STATUS_ACCOUNT_RESTRICTION` for an enrolled account on a non-interactive path,
+declined with `STATUS_INVALID_LOGON_TYPE` otherwise. It reads only the user
+name from the logon buffer (bounds-checked, as a client address or a packed
+offset), never the password, returns it as `AccountName` for the audit record,
+and asks the store with a single registry read whether that name is enrolled -
+no DPAPI decrypt and no name/SID lookup, so nothing re-enters LSA from the
+logon path.
 
 **3. The MSV1_0 sub-authentication filter** (`subauth.cpp`, `TacSubAuth.dll`,
 registered as `Auth0`). MSV1_0 calls it **after** it has validated a logon, and
-the filter can only veto: it refuses a network logon for an enrolled account and
-has no objection to anything else. MSV1_0 keeps the password check, the account
-restrictions and the token. The account is identified by the RID from the SAM,
-which survives a rename, with a single registry read (no DPAPI, no lookup). It
-reads nothing but the RID and the account name; it never touches the password
-hashes or the challenge-response. That credential-safety line is the whole
-point: the same LSA seam is where credential-theft malware sits, and this code
-deliberately does not read or keep credential material.
+the filter can only veto: it refuses a network logon for an enrolled account
+with `STATUS_INVALID_WORKSTATION` and has no objection to anything else, nor to
+a pass-through logon for an account that lives elsewhere. MSV1_0 keeps the
+password check, the account restrictions and the token. The account is
+identified by the RID from the SAM, which survives a rename, with a single
+registry read (no DPAPI, no lookup). It reads nothing but the RID and the
+account name; it never touches the password hashes or the challenge-response.
+That credential-safety line is the whole point: the same LSA seam is where
+credential-theft malware sits, and this code deliberately does not read or keep
+credential material.
+
+Both LSA pieces follow the same rules (`lsasafe.h`): no heap and no CRT of our
+own on the logon path, SEH around the decision, and an enrollment index that
+cannot be read counts as "enrolled" (fail closed). They link only
+`store_index.cpp` from the store, not the DPAPI and TOTP code.
+
+The filter leaves `LogoffTime` and `KickoffTime` alone; what MSV1_0 hands in
+there is undocumented, so every call writes the incoming values to the
+debugger. Logons where the server gets the password in clear (type 8,
+NetworkCleartext, e.g. WinRM or IIS with Basic auth) most likely reach the
+filter as an interactive logon level and get no objection. Untested: check in
+the VM which LogonLevel arrives, and whether the deny right for network logons
+catches type 8.
 
 Earlier versions exported `Msv1_0SubAuthenticationRoutine` under `Auth1..4`.
 That is the wrong hook: it only runs for logons that ask for it, and when it
@@ -140,6 +180,10 @@ password". `install-subauth.ps1` removes the old registration.
 Like the deny right, the filter also blocks RDP with NLA for enrolled accounts.
 Microsoft documents `Auth0` for domain controllers; whether MSV1_0 calls it for
 local accounts on your Windows version is the first thing to check in the VM.
+`install-subauth.ps1` refuses to run on a domain controller, where the filter
+would check domain RIDs against a local index. MSV1_0 loads the DLL the first
+time it needs it, so a broken filter can surface at the first network logon
+after the reboot, not only at boot.
 
 ```powershell
 .\install-authpackage.ps1     # the authentication package
@@ -152,10 +196,12 @@ local accounts on your Windows version is the first thing to check in the VM.
 > Safe Mode. Load them only in a throwaway VM with a snapshot, a BitLocker
 > recovery key and a WinRE way to edit the registry offline. Debug over a kernel
 > debugger. They are **learning skeletons, untested on live LSA.** With LSA
-> protection (RunAsPPL) on - the default on many current Windows 11 installs -
-> lsass refuses to load them at all, because they are not signed. See the
-> article "2FA inside LSA". The install scripts refuse to run while LSA
-> protection is on (registry check).
+> protection (RunAsPPL) on - the default on new, enterprise-joined Windows 11
+> 22H2+ installs with HVCI - lsass refuses to load them at all, because they
+> are not signed. See the article "2FA inside LSA". The install scripts refuse
+> to run while LSA protection is on: they check `RunAsPPL`/`RunAsPPLBoot` and,
+> because the default-on case is not documented to show up there, WinInit
+> event 12 ("started as a protected process") since the last boot.
 
 ## Limits
 
@@ -175,12 +221,25 @@ local accounts on your Windows version is the first thing to check in the VM.
 
 ## Tests
 
-`tests/totp_check.py` checks the algorithm against RFC 6238 in Python.
-`tests/native/` compiles the real `totp.cpp` and `verify.cpp` on Linux, against
-OpenSSL and an in-memory store, and checks the lockout, replay and fail-closed
-behaviour: `make -C tests/native`. `.github/workflows/build.yml` runs both, and
-builds everything with MSVC (`/W4 /sdl /guard:cf`, plus `/analyze`) on each push.
-None of this loads anything into LogonUI or lsass; that still needs the VM.
+`tests/totp_check.py` checks the algorithm against RFC 6238 in Python and
+exits non-zero on any failed check. `tests/native/` builds two tests on Linux
+with `make -C tests/native`:
+
+- `test_verify` compiles the real `totp.cpp` and `verify.cpp` against OpenSSL
+  and an in-memory store, and checks the lockout, replay and fail-closed
+  behaviour.
+- `test_lsa` compiles the real `subauth.cpp`, `ap.cpp` and `store_index.cpp` -
+  the code that runs inside lsass - against a fake registry, event log and LSA
+  heap, under AddressSanitizer and UBSan. It checks every veto and fail-closed
+  path, the `[out]` parameters, hostile logon buffers, non-ASCII names, and that
+  no LSA heap block or registry handle leaks. SEH is mapped to C++ exceptions
+  there, so the real `__except` still needs the VM.
+
+`.github/workflows/build.yml` runs both, parses every PowerShell script, and
+builds everything with MSVC (`/W4 /sdl /guard:cf /CETCOMPAT`) on each push.
+`/analyze` runs over all sources as a report, and with `/WX` over the lsass
+code, where a finding fails the build. None of this loads anything into
+LogonUI or lsass; that still needs the VM.
 
 ## License