Skip to content

[BED-6645] SharpHound RegistrySessions status incorrectly reports NetWkstaUserEnum result #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
martinsohn merged 1 commit into from
Oct 17, 2025
Merged

[BED-6645] SharpHound RegistrySessions status incorrectly reports NetWkstaUserEnum result #181

martinsohn merged 1 commit into from
Oct 17, 2025

Conversation

@martinsohn
Copy link
Contributor

@martinsohn martinsohn commented Oct 17, 2025 •
edited by coderabbitai bot
Loading

When --trackcomputercalls is enabled, the RegistrySessions CSV status was incorrectly checking privSessionResult instead of registrySessionResult, causing it to report the NetWkstaUserEnum status instead of the actual registry enumeration result.

Description

When --trackcomputercalls is enabled, the CSV status output for the RegistrySessions task incorrectly uses the privSessionResult variable (from NetWkstaUserEnum) instead of registrySessionResult (from the registry query). This causes the RegistrySessions row in the CSV to show incorrect status information.

Changed line 303 to use registrySessionResult.Collected instead of
privSessionResult.Collected for accurate status reporting.

Motivation and Context

https://specterops.atlassian.net/browse/BED-6645

How Has This Been Tested?

Test environment:

  1. Tested on production SharpHound build from main branch (commit 54ac6dd)
  2. Compiled fixed version locally with the corrected variable reference

Reproduction Steps:

  1. Disabled Remote Registry service on a computer
  2. Ran baseline SharpHound from main branch: .\SharpHound.exe -c session,loggedon --computerfile .\computers.txt --nozip --memcache --trackcomputercalls
  3. Examined generated *_compstatus.csv file
  4. Confirmed bug: RegistrySessions status duplicated NetWkstaUserEnum result instead of showing independent registry enumeration status. Confirmed in computers.json where result of RegistrySessions collection was false

Validation Steps:

  1. Applied fix
  2. Ran fixed build: .\SharpHound.exe -c session,loggedon --computerfile .\computers.txt --nozip --memcache --trackcomputercalls
  3. Confirmed fix: RegistrySessions now correctly reports its own status, independent of NetWkstaUserEnum result

Attached relevant output files pre-fix and post-fix.
postfix - data registry disabled.json
prefix - compstatus registry enabled.csv
prefix - data registry disabled.json
prefix - compstatus registry disabled.csv
prefix - data registry enabled.json

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

Summary by CodeRabbit

Bug Fixes

  • Updated status reporting for registry session entries to accurately reflect collection status or associated failure reasons.

@martinsohn
fix: Use correct result variable for RegistrySessions status tracking
54ac6dd 
When --trackcomputercalls is enabled, the RegistrySessions CSV status
was incorrectly checking privSessionResult instead of registrySessionResult,
causing it to report the NetWkstaUserEnum status instead of the actual
registry enumeration result.
@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025 •
edited by atlassian bot
Loading

Walkthrough

The change modifies status logging for RegistrySessions CSV entries. The Status field now derives its value from registrySessionResult.Collected instead of privSessionResult.Collected, determining whether to report Status or FailureReason accordingly.

Changes

Cohort / File(s) Summary
RegistrySessions Status Logic Fix
src/Runtime/ObjectProcessors.cs		 Modified status determination to use registrySessionResult.Collected instead of privSessionResult.Collected for RegistrySessions CSV entries

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A registry hopped through the logs with care,
Now gathering truth from the correct field there—
No more mixed signals from source to source,
The Status flows true on its proper course! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "BED-6645 SharpHound RegistrySessions status incorrectly reports NetWkstaUserEnum result" is directly aligned with the main change in the changeset. The title clearly identifies the specific bug being fixed—that RegistrySessions CSV status was using the wrong variable (privSessionResult instead of registrySessionResult)—which matches the core modification described in the raw summary. The title is concise, specific, and provides enough context for a reviewer to understand the primary change without being verbose.
Description Check ✅ Passed The PR description comprehensively follows the template with all critical sections completed. The Description section clearly explains the bug and the specific fix applied, the Motivation and Context section includes a direct link to issue BED-6645, and the How Has This Been Tested section is exceptionally detailed with reproduction steps, validation steps, and supporting file attachments. The Types of changes section correctly identifies this as a bug fix, and the checklist appropriately marks completed items such as "All new and existing tests passed." The description is well-structured and provides sufficient context for understanding and reviewing the change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BED-6645
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d53cc04 and 54ac6dd.

📒 Files selected for processing (1)
  • src/Runtime/ObjectProcessors.cs (1 hunks)
🔇 Additional comments (1)
src/Runtime/ObjectProcessors.cs (1)

292-292: LGTM! Variable reference corrected.

The fix correctly changes the status check from privSessionResult.Collected to registrySessionResult.Collected, ensuring the RegistrySessions CSV entry reports its own execution status rather than the NetWkstaUserEnum result. This aligns with the pattern used for other status writes in this method.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mykeelium mykeelium added the bug Something isn't working label Oct 17, 2025
@martinsohn martinsohn merged commit 4c32909 into 2.X Oct 17, 2025
2 checks passed
@martinsohn martinsohn deleted the BED-6645 branch October 17, 2025 15:26
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mykeelium mykeelium mykeelium approved these changes

Assignees

@martinsohn martinsohn

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@martinsohn @mykeelium