Skip to content

[Windows] Fix calculating credential blob size - #8

Closed
hawkeye116477 wants to merge 1 commit into
sillsdev:masterfrom
hawkeye116477:blob_size
Closed

[Windows] Fix calculating credential blob size#8
hawkeye116477 wants to merge 1 commit into
sillsdev:masterfrom
hawkeye116477:blob_size

Conversation

@hawkeye116477

Copy link
Copy Markdown

Earlier unecessary terminal sign was added to end of string.

@imnasnainaec

Copy link
Copy Markdown

According to Devin, this PR isn't a correct fix:

SIL.PasswordStore/Provider/WindowsProvider.cs:R28-34

Stored passwords can come back with extra garbage characters appended

The saved password is now recorded with a length that excludes its end marker (Encoding.Unicode.GetByteCount(password) at SIL.PasswordStore/Provider/WindowsProvider.cs:28-34), while the code that reads it back still relies on that marker to know where the password ends, so a retrieved password can contain trailing junk.
Impact: Users may get back a corrupted password (with random extra characters) instead of the one they saved.

Blob length excludes the terminating NUL while the reader scans for one

Marshal.StringToCoTaskMemUni allocates the string plus a terminating UTF-16 NUL, but only CredentialBlobSize bytes are persisted by CredWrite. With the new size (exact byte count of the characters, no terminator), the stored blob contains no NUL. On read, SIL.PasswordStore/Provider/WindowsProvider.cs:61 calls Marshal.PtrToStringUni(credential.CredentialBlob) with no length, so it keeps reading memory past the end of the returned blob until it happens to hit a zero word, producing extra characters or reading unrelated memory.

Fix: read using the known length, e.g. Marshal.PtrToStringUni(ptr, (int)(credential.CredentialBlobSize / 2)) (also handle odd sizes), or keep writing the terminator by adding sizeof(char) to the blob size.


When asked if the existing code on master works, Devin says that it works for ASCII passwords but not for non-ASCII.

@hawkeye116477

hawkeye116477 commented Aug 10, 2026

Copy link
Copy Markdown
Author

this PR isn't a correct fix
while the code that reads it back still relies on that marker to know where the password ends, so a retrieved password can contain trailing junk.

It's fine, just needs merging #7 also.

@imnasnainaec

Copy link
Copy Markdown

If the two PRs are so tightly coupled, please combine them into a single PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants