fix(registry): match hostnames case-insensitively against target_hosts_by_hostname - #9
Open
chryzsh wants to merge 1 commit into
Open
Conversation
…s_by_hostname Every other collector (http.py, privileged.py, smb.py, mssql.py) looks up ctx.target_hosts_by_hostname with the hostname lowercased first, matching how context.py always keys that dict on the lowercased canonical hostname (context.py:450). registry.py however has five lookups that used the raw, un-lowercased hostname passed into each phase function. Since the hostname argument itself is never normalized (context.py submits the original-case form to the work queue), any target with an uppercase character triggered a mismatch, four of the five crashed with KeyError, and the fifth (get_current_user) silently dropped a valid host_object_sid, dropping the RemoteRegistry-sourced arm of the HasSession edge with no error at all. Fixed all five call sites to lowercase the lookup key and guard the result before touching .ad_object, matching the pattern already used elsewhere in this file's sibling collectors. registry_collect_test.py and registry_current_user_test.py both used an all-lowercase fake hostname for the lookup key and the stored dict key alike, so neither could have caught this: .lower() is a no-op on an already-lowercase string. Updated both to use a mixed-case hostname against a lowercased dict key. Verified by reverting the registry.py fix and confirming both test files fail with exactly the predicted KeyError / silently-None host_object_sid, then restoring the fix and confirming all tests pass again. Tests: 140 passed (registry + mssql subset), 63 passed (CI-curated subset). ruff: all checks passed mypy: no issues found uv run pre-commit: done
Author
|
Realized immediately after submitting this PR that its a duplicate of #8 but I'd argue my solution is slightly more complete because I added the None-check for |
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.
Bug
registry.py looks up hosts in target_hosts_by_hostname using whatever case the hostname is in, but that dict always stores keys lowercase (context.py:106 and :450). every other collector already lowercases before doing this lookup (http.py, privileged.py, smb.py, mssql.py) while registry.py doesn't.
Impact
Since the hostname passed around never gets normalized, any host with uppercase letters breaks the lookup. 4 of the 5 cases will crash with "KeyError". the 5th one (get_current_user) doesn't crash, it just silently returns None instead of the real entry, so
host_object_siddisappears, which kills one of the only ways to get the HasSession edge without SCCM admin rights.Fix
I fixed all 5: lowercase before the lookup, and check for None before touching .ad_object. same as the other 4 collectors already do.
Fixing tests
I also found out the existing tests for this file (registry_collect_test.py, registry_current_user_test.py) could never have detected this. The fake hostname in both was already lowercase, both for the lookup and the dict key, so lowercasing it changes nothing. I fixed the tests to use mixed case for the hostname but lowercase for the dict key, matching how it actually works if you have a mixed or non-lowercase hostname. I then reverted my fix and both test files fail with exactly the errors you'd expect. Then restored them to make sure they pass correctly.
I didn't this against a real SCCM lab because I don't have a lab with mixed case hsotnames.