Log SMB services#21266
Conversation
|
Looks like there's a crash reproducible with the PR branch checked out and a Samba container running. Prerequisites # Start a Samba container
docker network create msf-test-net
docker run -d --name smb-target -p 445:445 -p 139:139 \
dperson/samba -p -u "testuser;testpass" -s "public;/tmp;yes;no;yes"
# Wait ~10s for Samba to start, then verify it's up
nc -zv 127.0.0.1 445Trigger the crash bundle exec msfconsole -qx "
use auxiliary/admin/smb/list_directory;
set RHOSTS 127.0.0.1;
set RPORT 445;
set SMBUser testuser;
set SMBPass testpass;
set SMBSHARE public;
run;
exit
"Current output (crash) Why this target triggers it: Verify it doesn't crash on master git checkout master
# re-run the same msfconsole command above — module completes normally (no service logged)
git checkout pr-21266Cleanup docker stop smb-target && docker rm smb-target
docker network rm msf-test-netRobot Review 🤖SummaryThis PR adds automatic Changed files:
Security Audit
Documentation
Code Review (AGENTS.md)
Violation 1 — if simple_client.client.negotiated_smb_version.present?
The fix should call Violation 2 — client = client.client if client.is_a?(Rex::Proto::SMB::SimpleClient)
# ...
info << ", last negotiated version: SMBv#{client.negotiated_smb_version} (dialect = #{client.dialect})"After unwrapping Additionally, RSpec (library changes)
Static Analysis
Check Method
Exploitation / Module ExecutionDynamic testing was performed using a Samba Docker container (dperson/samba) on port 445. Test: Result: ❌ Module crashes. The Test: The
Resilience
Verdict: ❌ REQUEST_CHANGESThe PR introduces a regression: any module that calls Required fixes:
Windows Testing
|
zeroSteiner
force-pushed
the
feat/log-smb-services
branch
from
f72bc3e to
f3baacd
Compare
|
Issue was an API descrapency in the legacy SMB client and the new RubySMB client. |
dwelch-r7
left a comment
dwelch-r7
left a comment
There was a problem hiding this comment.
Looks good, to whoever tests this let's make sure for modules like smb_version that also reports the smb_service we aren't accidentally double reporting the service
| { :use_spn => datastore['NTLM::SendSPN'], :name => simple_client.peerhost } | ||
| ) | ||
| ensure | ||
| if simple_client.client.dialect.present? |
There was a problem hiding this comment.
NAB: Let's consider wrappingthis detection up in a function so it's more obvious to future travellers what the intent is and so we can potentially change the underlying implementation in the future/add error handling etc.
4 participants
This was suggested today. Basically we should be logging SMB services. After some testing
#connectdidn't seem like a reliable place to do it so it logs the service in#smb_login. If authentication fails, the client still logs what dialect was negotiated so we log the service even if we couldn't authenticate to it.Verification
List the steps needed to make sure this thing works
msfconsoleDemo
The demo shows the different failures and that the services are reported sensible, no connection -> no service, authentication fails -> service reported, authentication succeeds -> service reported.