Scrub Linux Secret Service plaintext like Windows and macOS

[matt-edmondson]

Fixes #160

The defect

LinuxSecretServiceCredentialStore routed every credential through a managed string:

• TryLoad read the secret with Marshal.PtrToStringUTF8 and deserialized it with DeserializeFromString — the path that does not scrub.
• Save built the plaintext JSON with SerializeToString and handed the string to secret_password_store_sync, so the marshaller also made a native copy it frees without scrubbing.

A managed string is immutable, so neither copy can be zeroed. The plaintext sits on the GC heap — movable by compaction, readable from a crash dump or a heap dump — for an unbounded time after use. That is exactly the exposure CredentialSerialization's own doc comment gives as the reason DeserializeAndScrub and NativeSecretBuffer exist.

The change

Both paths now use byte arrays and unmanaged buffers this code owns and zeroes, which is what Windows and macOS already do:

byte[] blob = NativeSecretBuffer.ReadNulTerminated(passwordPtr);
credential = CredentialSerialization.DeserializeAndScrub(blob);

using NativeSecretBuffer nativeValue = NativeSecretBuffer.NulTerminatedCopyOf(blob);
// ... secret_password_store_sync(..., nativeValue.Pointer, ...)

secret_password_store_sync's password parameter is now IntPtr, not string. That is the part doing the work. Serializing to bytes but still declaring the parameter as a string would have left the runtime free to marshal its own native copy and free it unscrubbed — the property has to hold at the P/Invoke boundary, not just in the caller.

NativeSecretBuffer gains the two primitives this needs, because libsecret deals in nul-terminated C strings where Windows and macOS pass a pointer and a length:

primitive	why
NulTerminatedCopyOf(byte[])	Appends the terminator, and counts it in Length so Zero scrubs the whole allocation rather than leaving the last byte. It also zeroes its own managed intermediate on every path out, including when CopyOf throws.
ReadNulTerminated(IntPtr)	Copies up to the first nul into an array the caller hands to DeserializeAndScrub. This is what lets a C-string store take the same byte-array path as a pointer-and-length one.

NulTerminatedCopyOf rejects a source carrying its own nul byte rather than silently truncating. Nothing can produce one today — SerializeToUtf8Bytes escapes control characters — but a C-string API given an embedded nul would store a truncated secret, which fails silently at save time and surfaces much later as an unparseable blob.

The issue's fallback — document the gap if libsecret offers no byte entry point — turned out not to be needed. secret_password_lookup_binary_sync (its other suggestion) would mean three more P/Invokes and a SecretValue lifetime to manage; the returned gchar* is nul-terminated, and JSON never contains an interior nul, so scanning for the terminator gets there with one small helper instead.

The test that was actually missing

SecretScrubbingTests opens by explaining that scrubbing "lives in these two shared primitives rather than being re-implemented (and left untested) in each store", because a native store is only reachable on its own OS. That is the right design, and it had a hole: the primitives were tested, but nothing checked that a store called them. Windows was moved onto them in #144, Linux was not, and no test noticed for two releases.

NativeStoreScrubbingWiringTests closes that. For each of the three native stores it asserts TryLoad is compiled against DeserializeAndScrub and not DeserializeFromString, and Save against Serialize and not SerializeToString.

Whether a secret passed through a managed string is not observable at runtime — the plaintext's whole problem is that it lingers somewhere nothing can reach — so this inspects the IL for the call instead. Both helpers live in the same assembly as the stores, so a call site carries the target's own MethodDef token and matching on it needs no opcode table.

Two things keep that honest:

• The "must call" assertions are the scan's own control. A scan that found nothing would fail them, rather than quietly passing the "must not call" half and reporting a clean bill of health.
• A false match can only make an assertion stricter, never let a violation through, since the scan is only ever used to answer "is this call present".

Confirmed in practice: against the unfixed store the two tests fail naming the Linux store only — Windows and macOS pass, which is the control firing correctly.

Also added to SecretScrubbingTests, in its existing idiom: 7 cases over the two new primitives, covering the terminator being appended and scrubbed, the embedded-nul rejection, the empty-source and null-pointer edges, reading past a terminator, and the read-then-scrub composition a store performs.

Proved failing without the fix. Reverting only LinuxSecretServiceCredentialStore.cs — the new primitives stay, so it still compiles — two consecutive runs:

failed EveryNativeStoreLoadsThroughTheScrubbingDeserializer (5ms)
  LinuxSecretServiceCredentialStore.TryLoad must deserialize through DeserializeAndScrub so the
  plaintext copy it read is zeroed before the call returns.
failed EveryNativeStoreSavesFromScrubbableBytes (1ms)
  LinuxSecretServiceCredentialStore.Save must serialize to a byte array it can zero once the
  native call returns.

  total: 46   failed: 2   succeeded: 39   skipped: 5

Worth being straight about the split: the wiring tests are what fail on the defect. The 7 primitive tests cover new methods, so they cannot fail against main — they would not compile there. They are coverage for the new code, not evidence for the fix.

Verification

• dotnet build CredentialCache.sln -c Release — succeeded, 0 warnings, 0 errors, across net9.0 and net10.0
• dotnet test CredentialCache.sln -c Release — 46 total, 41 passed, 0 failed, 5 skipped, two consecutive runs
• Same suite against reverted LinuxSecretServiceCredentialStore.cs — 2 of 46 failed, two consecutive runs, as above

The 5 skips are pre-existing and environmental: 4 NativeCredentialStoreTests cases self-skip because this container has no Secret Service (secret_schema_new fails at the Schema static initializer), and 1 is Windows-only.

The changed native path is therefore not executed anywhere in this run. It is exercised by NativeCredentialStoreTests on the cross-platform.yml leg that brings up dbus-run-session + gnome-keyring-daemon, and that leg is where a marshalling mistake in the new IntPtr signature would show up. Flagging it rather than implying the suite covers it.

Docs

README.md's platform notes gain a bullet stating that all three native stores handle plaintext on the same terms, and that a credential the caller holds after TryGet is an ordinary managed object whose lifetime is theirs — the library scrubs its own copies, not yours.

The Linux store's class doc gains the same statement; the issue notes it previously read as if scrubbing were platform-uniform when it was not.

Worth knowing

Building this repository rewrites .gitignore in the working tree from the SDK's shared template, adding Unity and Godot rules unrelated to anything here. It is not part of this diff; I reverted it. Anyone working this repository will see it reappear after a build.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LLBd6CLjwb1r6wuDqDzW4J

---

Generated by Claude Code

[matt-edmondson]

Pushed d00757d for the failing quality gate

SonarCloud failed on 97a49ab: 70.4% coverage on new code, required ≥ 80%.

Measured it locally rather than guessing, with cobertura over the test run and the line set intersected against the diff. All 20 uncovered new lines were in LinuxSecretServiceCredentialStore's own body; the new NativeSecretBuffer primitives were 21/21 covered. The store's methods need a live Secret Service, which no CI leg in dotnet.yml provides — so the gate was measuring exactly the code that cannot run there.

Two separate causes, and neither is "the gate is wrong":

1. I re-indented code I hadn't changed. Most of those 20 lines were the pre-existing secret_password_store_sync call. Wrapping Save in try/finally re-indented the whole argument list, so all eleven lines counted as new. Save now uses a using declaration, which needs no block — the call is byte-identical to main except the one argument that had to change:

-			value,
+			value.Pointer,

That is worth having independently of coverage: the diff now shows what the fix actually changes instead of burying it in a re-indent.

2. Composition logic was sitting where no test can reach it. The remaining lines were the read-and-scrub and serialize-and-scrub sequences, in a method body that only runs on Linux-with-libsecret. Those moved into NativeSecretBuffer.ReadCredential and OfCredential, so each store method is one call and the logic is exercised on every platform.

This is the principle SecretScrubbingTests already opens with — "the scrubbing lives in these two shared primitives rather than being re-implemented (and left untested) in each store" — applied to the part of it that was still in the store. It is the same reasoning as the original fix, carried one step further, so I'd have wanted it here anyway.

	before	after
uncovered new lines in the store	20	3
new-code line coverage (local, Linux only)	58.7%	90.6%

The three that remain are the store's two statements and one call argument. They need libsecret; there is no honest way to cover them from here, and I have not tried to hide them.

The guard still catches the original defect

Accepting the wrapper as a stand-in for a direct call would weaken the wiring test, so a third case pins the wrappers themselves:

Assert.IsTrue(Calls(ReadCredential, DeserializeAndScrub), ...);
Assert.IsFalse(Calls(ReadCredential, DeserializeFromString), ...);

Re-proved against main's store (git checkout origin/main -- LinuxSecretServiceCredentialStore.cs), two consecutive runs:

failed EveryNativeStoreLoadsThroughTheScrubbingDeserializer (6ms)
  LinuxSecretServiceCredentialStore.TryLoad must deserialize through DeserializeAndScrub,
  directly or via NativeSecretBuffer.ReadCredential, so the plaintext copy it read is zeroed.
failed EveryNativeStoreSavesFromScrubbableBytes (0ms)
  LinuxSecretServiceCredentialStore.Save must serialize to bytes it can zero, directly or via
  NativeSecretBuffer.OfCredential.

  total: 51   failed: 2   succeeded: 44   skipped: 5

Still naming the Linux store only — Windows and macOS pass, which is the scan's positive control firing correctly.

Worth recording, because it nearly fooled me: once this branch had a commit, git checkout <path> restores from the index, not from main. My first re-proof attempt "reverted" the file to the already-fixed committed version and the tests passed, which looked like the guard had gone blind. It had not — the revert was the thing that was wrong. Reverting from origin/main explicitly is what the run above does.

Verification

• dotnet build CredentialCache.sln -c Release — 0 warnings, 0 errors, net9.0 and net10.0
• dotnet test CredentialCache.sln -c Release — 51 total, 46 passed, 0 failed, 5 skipped, three consecutive runs
• Against main's store — 2 of 51 failed, two consecutive runs, as above

4 tests added for the two new wrappers, including that ReadCredential answers null for all three "nothing to read" cases (no entry, empty entry, unparseable bytes), since a store reads that as "no credential for this persona" rather than an error.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
93.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud