registry: harden hive parser against malformed inputs#27
Merged
Conversation
Four parser-panic / silent-corruption bugs in pkg/registry/hive.go, all reachable from attacker-controlled hive bytes: 1. GetValueData resident branch: VKRecord.DataLen lower 31 bits are read verbatim and used to slice a 4-byte buffer at [:dataLen]. A DataLen of 0x80000005 panics with "slice bounds out of range". Found by kajaaz using Zorya (issue #25); fix matches her suggested one-line bounds check. 2. SetValueData resident branch (structurally identical to #1): the existing len(newData) == dataLen check doesn't enforce the 4-byte cap, so a hostile dataLen=5 with a matching 5-byte newData slices one byte past the DataOffset field into the adjacent cell. Same guard. 3. SetValueData non-resident branch: vk.DataOffset is attacker- controlled and the code does copy(h.data[dataPos:dataPos+dataLen], newData) without ever validating the destination. Hostile offsets either panic on out-of-bounds or silently scribble over arbitrary hive bytes (a value of 0xFFFFF000 lands near the regf header). Route through readCell (which validates the cell header and bounds) and verify dataLen fits before mutating. 4. enumSubKeys riSig branch: readCell can return a slice shorter than the 4-byte cell header (minimum-size cell with no usable bytes), so the immediate subCell[0:2] / subCell[2:4] reads can panic on a malformed sub-list. The sibling code at line 397/437 already guards the analogous index; mirror it here. Fixes #25 plus three structurally similar bugs surfaced while patching.
Cover the GetValueData guard added in the previous commit with two
table-driven tests: the rejection path (dataLen ∈ {5, 8, 0xFF,
0x7FFFFFFF} with resident bit set) confirms the guard fires without
panicking and surfaces a helpful error; the happy path (dataLen ∈
{1, 2, 3, 4}) confirms the inline DataOffset bytes are returned
correctly so the guard didn't regress valid hives.
The matching guards in SetValueData (resident + non-resident write
paths) and enumSubKeys (riSig branch) require full NK/VK/sub-list
hive synthesis to exercise end-to-end; they're structurally identical
to the read-path guard and are validated by build/vet plus the lab
regression check on secretsdump. Worth a follow-up to extend the
synth helpers and cover them directly.
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
1 participant
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.
Summary
Closes #25 plus three structurally similar bugs in
pkg/registry/hive.gosurfaced while patching. All four are panic-or-silent-corruption paths reachable from attacker-controlled hive bytes; all four are fixed by single-condition guards that match the cell-header invariant the existing code already implicitly assumes.GetValueDataresident (issue #25)data[:dataLen]on a 4-byte bufferdataLen > 4, return errorSetValueDataresidentDataOffsetfieldSetValueDatanon-residentcopy(h.data[off:off+dataLen],...)readCell, verifydataLen <= len(cell)enumSubKeysriSig branchsubCell[0:2]/subCell[2:4]without length checkif len(subCell) < 4 { continue }Issue #25 reports the read-path bug (found by kajaaz via Zorya). The other three are the same bug class one file over, caught during review.
Test plan
go build ./...,go vet ./...,go test ./...all cleanTestGetValueDataResidentLengthRejectedcoversdataLen ∈ {5, 8, 0xFF, 0x7FFFFFFF}with the resident flag;TestGetValueDataResidentLengthAllowedcovers the happy path 1..4 bytes)secretsdump -kagainst a GOAD DC decoded SAM and LSA Secrets correctly through the modified read AND write pathsThe
SetValueData(resident + non-resident) andenumSubKeysriSig guards aren't covered by unit tests in this PR; exercising them requires building a full NK/VK/sub-list hive synthesis helper. Left as a follow-up; the fixes themselves are structurally identical to the read-path guard which is tested.