pe: null-terminate authenticode digest/thumbprint hex buffers#2221
Merged
plusvic merged 1 commit intoJul 6, 2026
Merged
Conversation
digest_ascii buffers in _process_authenticode() and thumbprint_ascii in write_certificate() are filled by a sprintf loop that writes exactly len*2 hex chars but never sets the trailing byte, even though every buffer is sized len*2+1 to hold it. yr_set_string() calls strlen() on the result before copying, so the string used to actually run past the hex digits into whatever uninitialized stack/heap byte follows until it happens to hit a zero. Same bug shape already fixed correctly for imphash in this same file (pe_get_imphash sets digest_ascii[YR_MD5_LEN * 2] = '\''\0'\'' after its loop) - this applies the same fix to the four authenticode digest sites and the certificate thumbprint site. Fixes VirusTotal#2176. The failing test (test-pe.c, pe.signatures[0].thumbprint compared against a fixed hex string) reads one of the buffers this touches, which explains the platform/compiler-dependent flakiness reported there (stack contents differ across Rocky 8 vs 9).
plusvic
approved these changes
Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2176.
All the hex-digest buffers in
_process_authenticode()(authenticode digest, file_digest, signer digest, countersignature digest) andthumbprint_asciiin thewrite_certificatemacro are allocated with room for a trailing null (len * 2 + 1), but thesprintfloop that fills them only ever writeslen * 2hex characters. Nothing ever sets that last byte, soyr_set_string()callingstrlen()on the buffer reads past the hex digits into whatever uninitialized stack or heap byte happens to follow, until it hits a zero.This same bug shape was already fixed correctly elsewhere in this file:
pe_get_imphash()setsdigest_ascii[YR_MD5_LEN * 2] = '\0'right after its loop. This PR does the same thing at the five spots that were missing it.This lines up with the failure in #2176 (
test-pe.c, thepe.signatures[0].thumbprintcheck failing on Rocky 9 but not Rocky 8) — thumbprint comes from exactly the buffer this touches, and reading uninitialized memory would naturally be sensitive to stack layout/compiler version, which is what varies between those two.Didn't have a build toolchain available to run
make checklocally, so this is verified by code inspection: traced everysprintf-into-hex-buffer site in the file, confirmed each buffer's allocation size already reserves the byte, and matched the fix to the existing correct pattern used for imphash three times over. No behavior change to anything except cases where the buffer wasn't already accidentally null-terminated by chance.