Conversation
|
Thanks for your pull request! As part of our landing process, we manually verify that all modules work as expected. We've added the |
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
zeroSteiner
force-pushed
the
fix/issue/20959
branch
from
a9479a4 to
da04aba
Compare
cdelafuente-r7
left a comment
cdelafuente-r7
left a comment
There was a problem hiding this comment.
Thank you for these improvements! I left a few comments and suggestions for you to look into when you get a chance.
Also, there is an unrelated issue I found in the auxiliary/gather/ldap_passwords module while I was testing. Please see this issue for details.
| # [[SAN URL prefix for strong SID mapping for KDCs running Windows Server Preview Build 25246 and later](https://techcommunity.microsoft.com/blog/askds/preview-of-san-uri-for-certificate-strong-mapping-for-kb5014754/3789785)] | ||
| SAN_URL_PREFIX = "tag:microsoft.com,2022-09-14:sid:" | ||
|
|
||
| ADCS_CA_SERVICE_NAME = 'adcs-ca' |
There was a problem hiding this comment.
I'm wondering if this should be defined in the Report mixin instead. Since it is only used for reporting servicing and modules that don't include this file could also use?
cdelafuente-r7
moved this from In Progress
to Waiting on Contributor
in Metasploit Kanban
cdelafuente-r7
moved this from Waiting on Contributor
to In Progress
in Metasploit Kanban
| } | ||
| end | ||
|
|
||
| def report_icertpassage_service |
There was a problem hiding this comment.
The icpr variable is missing here and I think this should be passed as argument:
| def report_icertpassage_service | |
| def report_icertpassage_service(icpr) |
or maybe just passing the host and port:
| def report_icertpassage_service | |
| def report_icertpassage_service(host, port) |
There was a problem hiding this comment.
We can actually use #simple so we don't need an argument at all. I added this in 7ff46b8.
cdelafuente-r7
moved this from In Progress
to Waiting on Contributor
in Metasploit Kanban
cdelafuente-r7
moved this from Waiting on Contributor
to In Progress
in Metasploit Kanban
| host: icpr.tree.client.dispatcher.tcp_socket.peerhost, | ||
| port: icpr.tree.client.dispatcher.tcp_socket.peerport, | ||
| proto: 'tcp', | ||
| parents: report_icertpassage_service |
There was a problem hiding this comment.
#report_icertpassage_service returns a Mdm::Service object and it is not the format expected by the :parents field. It should be a hash or an array of hashes (for multiple parents). If we want to have these helper methods being used directly into the :parents field, we need to make sure they return the expected format. Maybe doing something like this:
def report_icertpassage_service
service = {
name: 'icertpassage',
resource: { dcerpc: { pipe: 'cert' } },
host: simple.peerhost,
port: simple.peerport,
proto: 'tcp',
parents: report_dcerpc_service
}
report_service(service)
service
end def report_dcerpc_service
service = {
name: 'dcerpc',
info: "Module: #{fullname}",
host: simple.peerhost,
port: simple.peerport,
proto: 'tcp',
resource: { smb: { share: 'IPC$' } },
parents: {
name: 'smb',
host: simple.peerhost,
port: simple.peerport,
proto: 'tcp',
info: "Module: #{fullname}, last negotiated version: SMBv#{simple.client.negotiated_smb_version} (dialect = #{simple.client.dialect})",
parents: {
name: 'tcp',
host: simple.peerhost,
port: simple.peerport,
proto: 'tcp'
}
}
}
report_service(service)
service
endThe only drawback I see with the parent services will be processed each time #report_service is called, with cause multiple queries to the database where it is not really necessary. But it is not a big deal and it is something we can address later.
As a side note, if you register the service in one go (calling #report_service once like you did in #report_service_icertpassage in modules/auxiliary/gather/ldap_esc_vulnerable_cert_finder.rb). There will be only one database query per service.
lib/msf/core/db_manager/service.rb
Outdated
There was a problem hiding this comment.
This should also be updated to allow Mdm::Service?
| return unless services.is_a?(Hash) || services.is_a?(::Array) || services.is_a?(Mdm::Service) |
Or maybe completely remove this check since it is converted to an Array afterwards and each element is validated in the #map loop.
lib/msf/core/db_manager/service.rb
Outdated
| if service.is_a?(Mdm::Service) | ||
| service_obj = service | ||
| elsif service.is_a?(Hash) | ||
| return if service[:port].nil? || service[:proto].nil? |
There was a problem hiding this comment.
After thinking about it might not be a goos idea to return here. Maybe using next instead?
We can also make sure no nil is present in the resulting array:
services.map do |service|
...
end.compact
835ac79 to
3a4c4ec
Compare
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
There was a problem hiding this comment.
Pull request overview
This PR enhances Metasploit’s LDAP/ADCS-related modules to automatically report related services (LDAP, DCERPC/ICertPassage/ADCS CA) and to improve vulnerability reporting by associating findings with the affected LDAP object’s DN (and, for ADCS template findings, the template name) so results are uniquely keyed and easier to interpret.
Changes:
- Add automatic LDAP service reporting on successful bind, and introduce/standardize DCERPC → ICertPassage → ADCS-CA service reporting.
- Update LDAP modules to attach LDAP object DNs (and template names where applicable) to vulnerability resources for more precise reporting/grouping.
- Adjust DB service-chain processing behavior (return
[]instead ofnilfor invalid service hashes) and update specs accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/shared/examples/msf/db_manager/vuln.rb | Updates shared vuln spec to stop relying on sname for reporting. |
| spec/support/shared/examples/msf/db_manager/service.rb | Updates expectations for process_service_chain invalid-parameter behavior ([] vs nil). |
| modules/auxiliary/gather/ldap_passwords.rb | Adds per-entry DN tracking and new vuln reporting for discovered LDAP secrets. |
| modules/auxiliary/gather/ldap_esc_vulnerable_cert_finder.rb | Standardizes ADCS service reporting and adds DN/template metadata to reported vulns. |
| modules/auxiliary/admin/ldap/shadow_credentials.rb | Adds DN/service context to check vuln reporting via CheckCode.vuln. |
| modules/auxiliary/admin/ldap/rbcd.rb | Adds DN/service context to check vuln reporting via CheckCode.vuln. |
| lib/msf/core/exploit/remote/smb/client/ipc.rb | Adds report_dcerpc_service and reports it on IPC connect, establishing service parents. |
| lib/msf/core/exploit/remote/ms_icpr.rb | Introduces adcs-ca service name constant and reports ICPR/CA services with parents. |
| lib/msf/core/exploit/remote/ldap.rb | Adds report_ldap_service, reports it after successful bind, and normalizes peer. |
| lib/msf/core/db_manager/service.rb | Enhances process_service_chain to accept Mdm::Service objects and return compacted arrays. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| parents: { | ||
| name: 'smb', | ||
| parents: { | ||
| name: 'tcp' | ||
| } |
There was a problem hiding this comment.
The tcp parent in this service chain is missing host/port/proto. Because process_service_chain skips hash services without :port and :proto, this tcp parent will be dropped and the chain will stop at smb. Either remove the tcp parent entry or include the required fields so the full hierarchy is actually recorded.
This updates a handful of LDAP related modules to report the LDAP and ADCS services automatically. It also updates vulnerability reporting to include the LDAP object's DN so users can tell exactly where the vulnerability is. This also means vulnerabilities can use common names, so for example all the instances of ESC# are grouped together, but reported separately for each object that's vulnerable as uniquely keyed by the template that's affected.
Closes #20959
Verification
None of the functionality of the modules really changed, we're just reporting more info.
msfconsoleuse auxiliary/gather/ldap_passwordsand run it, see a vulnerability reported for the plaintext credentials, NTLM hashes and Kerberos keys that are found, see a DN for eachuse auxiliary/admin/ldap/rbcdand run the check method, see a vulnerability reported that's associated with the service and includes the affected objectuse auxiliary/admin/ldap/shadow_credentialsand run the check method, see a vulnerability reported that's associated with the service and includes the affected objectuse auxiliary/gather/ldap_esc_vulnerable_cert_finderand run it, see vulnerabilities reported for each of the findins with the template infouse auxiliary/admin/dcerpc/icpr_certand run it, see that the service is reported with the CA name, note that it's consistent with what was reported by the ldap_esc_vulnerable_cert_finder moduleExample
After running through this content in my test environment, these are the services and vulns I have reported: