diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
index 2f2409e..137eb25 100644
--- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
+++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
@@ -36,6 +36,7 @@ public class NativeStoreScrubbingWiringTests
private static readonly MethodInfo ReadCredential = Buffer(nameof(NativeSecretBuffer.ReadCredential));
private static readonly MethodInfo OfCredential = Buffer(nameof(NativeSecretBuffer.OfCredential));
+ private static readonly MethodInfo CopyWithinLimit = Buffer(nameof(NativeSecretBuffer.CopyWithinLimit));
private static MethodInfo Helper(string name) =>
typeof(CredentialSerialization).GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static)
@@ -133,6 +134,19 @@ public void EveryNativeStoreSavesFromScrubbableBytes()
}
}
+ ///
+ /// The Windows store is the one with a blob size limit. Checking it in the store body put
+ /// the throw ahead of the store's own try, so an over-limit credential was rejected
+ /// without its serialized plaintext ever being zeroed (#165). The shared helper owns the
+ /// check inside its scrubbing, where exercises it.
+ ///
+ [TestMethod]
+ public void TheWindowsStoreChecksItsBlobLimitInsideTheScrubbing() =>
+ Assert.IsTrue(
+ Calls(Method(typeof(WindowsCredentialStore), nameof(ICredentialStore.Save)), CopyWithinLimit),
+ "WindowsCredentialStore.Save must copy its blob through NativeSecretBuffer.CopyWithinLimit, "
+ + "which zeroes the serialized plaintext on the over-limit rejection path as well.");
+
///
/// The second hop of the two assertions above. Accepting the wrapper as equivalent to a
/// direct call is only sound while the wrapper itself uses the scrubbing helper, so that is
diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs
index ea3da40..6cab047 100644
--- a/CredentialCache.Test/SecretScrubbingTests.cs
+++ b/CredentialCache.Test/SecretScrubbingTests.cs
@@ -158,6 +158,37 @@ public void NativeSecretBufferHandlesAnEmptySource()
public void NativeSecretBufferRejectsANullSource() =>
Assert.ThrowsExactly(() => NativeSecretBuffer.CopyOf(null!));
+ [TestMethod]
+ public void CopyWithinLimitCopiesASourceAtTheLimitAndZeroesTheManagedCopy()
+ {
+ byte[] blob = SerializedCredential();
+ byte[] expected = (byte[])blob.Clone();
+
+ using NativeSecretBuffer buffer = NativeSecretBuffer.CopyWithinLimit(blob, blob.Length, "Test store");
+
+ Assert.AreSequenceEqual(expected, ReadUnmanaged(buffer));
+ Assert.AreSequenceEqual(new byte[blob.Length], blob,
+ "The managed plaintext must be zeroed once it has been copied out.");
+ }
+
+ [TestMethod]
+ public void CopyWithinLimitRejectsAnOversizeSourceAndStillZeroesIt()
+ {
+ byte[] blob = SerializedCredential();
+
+ CredentialStoreException ex = Assert.ThrowsExactly(
+ () => NativeSecretBuffer.CopyWithinLimit(blob, blob.Length - 1, "Test store"));
+
+ StringAssert.Contains(ex.Message, "Test store");
+ StringAssert.Contains(ex.Message, $"(was {blob.Length})");
+ Assert.AreSequenceEqual(new byte[blob.Length], blob,
+ "An over-limit credential must be scrubbed on the rejection path too.");
+ }
+
+ [TestMethod]
+ public void CopyWithinLimitRejectsANullSource() =>
+ Assert.ThrowsExactly(() => NativeSecretBuffer.CopyWithinLimit(null!, 1, "Test store"));
+
[TestMethod]
public void NulTerminatedCopyOfAppendsTheTerminator()
{
diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs
index 746c4d7..91d1b84 100644
--- a/CredentialCache/Storage/NativeSecretBuffer.cs
+++ b/CredentialCache/Storage/NativeSecretBuffer.cs
@@ -60,6 +60,41 @@ internal static NativeSecretBuffer CopyOf(byte[] source)
return new NativeSecretBuffer(pointer, source.Length);
}
+ ///
+ /// Copies into newly allocated unmanaged memory, provided it fits
+ /// within , and zeroes before returning.
+ ///
+ /// The plaintext bytes to copy. Always zeroed, whether or not this throws.
+ /// The largest blob the native store accepts, in bytes.
+ /// The store's name, for the message of an over-limit exception.
+ /// A buffer owning the unmanaged copy, which the caller must dispose.
+ /// is longer than .
+ ///
+ /// A store that checks the limit itself is tempted to do it before its own try, which
+ /// leaves the plaintext on the managed heap unscrubbed whenever the check throws. Taking the
+ /// managed copy over here keeps the rejection path inside the scrubbing, and keeps it
+ /// exercised by tests on every operating system rather than only the store's own.
+ ///
+ internal static NativeSecretBuffer CopyWithinLimit(byte[] source, int maxLength, string storeName)
+ {
+ ArgumentNullException.ThrowIfNull(source);
+
+ try
+ {
+ if (source.Length > maxLength)
+ {
+ throw new CredentialStoreException(
+ $"Credential exceeds the {storeName} blob size limit of {maxLength} bytes (was {source.Length}).");
+ }
+
+ return CopyOf(source);
+ }
+ finally
+ {
+ CredentialSerialization.Zero(source);
+ }
+ }
+
///
/// Copies into newly allocated unmanaged memory followed by a
/// single nul byte, for a native API that takes a C string rather than a pointer and a
diff --git a/CredentialCache/Storage/WindowsCredentialStore.cs b/CredentialCache/Storage/WindowsCredentialStore.cs
index ba3526b..3baf6d6 100644
--- a/CredentialCache/Storage/WindowsCredentialStore.cs
+++ b/CredentialCache/Storage/WindowsCredentialStore.cs
@@ -72,39 +72,28 @@ public void Save(PersonaGUID persona, Credential credential)
ArgumentNullException.ThrowIfNull(persona);
ArgumentNullException.ThrowIfNull(credential);
- byte[] blob = CredentialSerialization.Serialize(credential);
- if (blob.Length > NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE)
+ // CopyWithinLimit zeroes the serialized bytes on every path, the over-limit rejection
+ // included. The unmanaged copy handed to CredWriteW is plaintext too; NativeSecretBuffer
+ // zeroes it before freeing it, which Marshal.FreeHGlobal alone does not.
+ using NativeSecretBuffer nativeBlob = NativeSecretBuffer.CopyWithinLimit(
+ CredentialSerialization.Serialize(credential),
+ NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE,
+ Name);
+
+ NativeMethods.CREDENTIAL native = new()
{
- throw new CredentialStoreException(
- $"Credential exceeds the Windows Credential Manager blob size limit of " +
- $"{NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE} bytes (was {blob.Length}).");
- }
-
- try
+ Type = NativeMethods.CRED_TYPE_GENERIC,
+ TargetName = TargetFor(persona),
+ CredentialBlob = nativeBlob.Pointer,
+ CredentialBlobSize = nativeBlob.Length,
+ Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE,
+ UserName = Environment.UserName,
+ };
+
+ if (!NativeMethods.CredWrite(ref native, 0))
{
- // The unmanaged copy handed to CredWriteW is plaintext; NativeSecretBuffer
- // zeroes it before freeing it, which Marshal.FreeHGlobal alone does not.
- using NativeSecretBuffer nativeBlob = NativeSecretBuffer.CopyOf(blob);
-
- NativeMethods.CREDENTIAL native = new()
- {
- Type = NativeMethods.CRED_TYPE_GENERIC,
- TargetName = TargetFor(persona),
- CredentialBlob = nativeBlob.Pointer,
- CredentialBlobSize = nativeBlob.Length,
- Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE,
- UserName = Environment.UserName,
- };
-
- if (!NativeMethods.CredWrite(ref native, 0))
- {
- int err = Marshal.GetLastWin32Error();
- throw new CredentialStoreException($"CredWrite failed for '{persona}'.", new Win32Exception(err));
- }
- }
- finally
- {
- CredentialSerialization.Zero(blob);
+ int err = Marshal.GetLastWin32Error();
+ throw new CredentialStoreException($"CredWrite failed for '{persona}'.", new Win32Exception(err));
}
}