Fix/msf host info cidr filtering#21448
Conversation
smcintyre-r7
left a comment
There was a problem hiding this comment.
These changes make sense to me. So the RPC endpoint doesn't actually support CIDR notation, so you're fixing it here with a bit of post processing. @cdelafuente-r7 Does this seem reasonable, or do you think the fix should be applied to the RPC endpoint? Keeping the logic here would contain the blast radius of potential bugs, e.g. if current RPC clients are aware of and reliant on this behavior.
@kx7m2qd One issue I am seeing is that this branch has changes from your previous PR. All commits before 0aaed23 should be dropped since I already merged those in for you from your previous PR. Please rebase this to drop those commits.
The addresses parameter was passed directly to db_hosts which does exact string matching, so CIDR ranges like 192.168.159.0/24 never matched any host address and returned 0 results. Fix: detect CIDR notation and filter using IPAddr in Ruby after fetching all hosts, instead of passing the CIDR to the API. Exact IP addresses continue to be passed to the API as before. Fixes rapid7#21405
3d6f495 to
8293122
Compare
|
I think it makes more sense to fix the RPC endpoint instead. This would be beneficial for anything using the API. Since the default behavior (accepting just one IP) will still be accepted, I don't think this would impact many RPC clients. These clients should already be sending a single IP. |
|
@kx7m2qd Are you up for adjusting the approach? We'd want to have these changes reverted, and the filtering added to the RPC end point instead. If you're not just let us know, and we'll close this out. |
|
Sure, happy to adjust the approach! I'll revert the MCP-layer changes and move the CIDR filtering to the RPC endpoint. Could you point me to the relevant file? I'm guessing it's somewhere in |
|
It might be this https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/rpc/v10/rpc_db.rb#L403 but I'm not entirely sure. |
|
Thanks! I can see the fix in |
| addresses = Array(opts[:addresses]) | ||
| exact_addrs = addresses.reject { |a| a.to_s.include?('/') } | ||
| cidr_strs = addresses.select { |a| a.to_s.include?('/') } | ||
| cidr_filters = cidr_strs.map { |c| IPAddr.new(c).to_string } |
There was a problem hiding this comment.
This will remove the network block information and give you the network address only. I don't think this will work when you build the SQL query later:
[20] pry(main)> cidr_strs = ["4.5.6.7/24", "6.6.6.7/32"]
=> ["4.5.6.7/24", "6.6.6.7/32"]
[21] pry(main)> cidr_filters = cidr_strs.map { |c| IPAddr.new(c).to_string }
=> ["4.5.6.0", "6.6.6.7"]
A valid CIDR would be:
["4.5.6.0/24", "6.6.6.7/32"]
I believe you want IPAddr.new(c).cidr instead?
| end | ||
|
|
||
| if cidr_filters.any? | ||
| address_filter_sql << cidr_filters.map { 'hosts.address <<= ?::cidr' }.join(' OR ') |
There was a problem hiding this comment.
I'm concerned this will only be compatible with PostgreSQL. We have a plan to migrate to SQLite at some point. Do you think it would be possible to use a more portable solution?
There was a problem hiding this comment.
I've raised this point about us using the IP address types in PostgreSQL multiple times when we've had those discussions. I don't know if there is a more portable solution because there's not a IP / Network data type in SQLite AFAIK. We could do something fancy with bitmasks and integers maybe but that would probably also be dialect specific and this PR here is not the right spot to figure out how we should handle the broader problem of migrating from PostgreSQL to SQLite.
| host[:address] = h.address.to_s | ||
| host[:mac] = h.mac.to_s | ||
| host[:name] = h.name.to_s | ||
| host[:state] = h.state.to_s | ||
| host[:os_name] = h.os_name.to_s | ||
| host[:os_flavor] = h.os_flavor.to_s | ||
| host[:os_sp] = h.os_sp.to_s | ||
| host[:os_lang] = h.os_lang.to_s | ||
| host[:address] = h.address.to_s | ||
| host[:mac] = h.mac.to_s | ||
| host[:name] = h.name.to_s | ||
| host[:state] = h.state.to_s | ||
| host[:os_name] = h.os_name.to_s | ||
| host[:os_flavor] = h.os_flavor.to_s | ||
| host[:os_sp] = h.os_sp.to_s | ||
| host[:os_lang] = h.os_lang.to_s | ||
| host[:updated_at] = h.updated_at.to_i | ||
| host[:purpose] = h.purpose.to_s | ||
| host[:info] = h.info.to_s | ||
| host[:comments] = h.comments.to_s | ||
| ret[:hosts] << host | ||
| host[:purpose] = h.purpose.to_s | ||
| host[:info] = h.info.to_s | ||
| host[:comments] = h.comments.to_s | ||
| ret[:hosts] << host |
There was a problem hiding this comment.
These spaces are not necessary. It is something rubocop usually flags as an issue (we don't have AllowForAlignment set globally) and should be kept as it was originally.
Rubocop error for references:
Layout/ExtraSpacingWithBinDataIgnored: Unnecessary spacing detected.
| # @return [Hash] Host information that starts with the following hash key: | ||
| # * 'hosts' [Array<Hash>] An array of hosts. Each hash in the array will have the following: | ||
| # * 'created_at' [Integer] Creation date. | ||
| # * 'address' [String] IP address. |
There was a problem hiding this comment.
It looks like this option is wrongly documented. Apparently, addresses is used instead. Also, the documentation should mention that CIDR are accepted.
There was a problem hiding this comment.
I fixed this but the location isn't here. L385 is the return value, L374 is the argument (where I made the change).
This reverts commit 8293122.
c56ec72 to
81f832b
Compare
|
|
||
| limit = opts.delete(:limit) || 100 | ||
| exact_addrs = [] | ||
| cidr_filters = [] |
There was a problem hiding this comment.
I believe cidr_filters should be replaced by cidr_addrs:
| cidr_filters = [] | |
| cidr_addrs = [] |
49b81ce to
db5ac40
Compare
Tests the db.hosts RPC handler directly (no HTTP/dispatcher layer): reports a fixed set of hosts and asserts that CIDR address filters resolve correctly, including the regression case where the filter uses a host address rather than the network address (e.g. 10.20.30.100/24 must match the whole /24, which the earlier IPAddr#to_string implementation broke by stripping the prefix). Guarded with `unless ENV['REMOTE_DB']` like the other DB specs: rpc_hosts looks up the workspace through framework.db but runs the CIDR query against the local ApplicationRecord connection, and under REMOTE_DB the separate data-service process cannot see the rows created inside the example's transactional fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What this fixes
Fixes #21405
The
msf_host_infoMCP tool was not honoring CIDR range filters. When a CIDRlike
192.168.159.0/24was passed as theaddressesparameter, it was forwardeddirectly to
db_hostswhich does exact string matching — so no hosts were everreturned.
The fix detects CIDR notation (presence of
/) and performs the filtering in Rubyusing
IPAddrafter fetching all hosts, instead of passing the CIDR to the API.Exact IP addresses continue to be passed to the API as before.
Verification
./msfconsole --mcp-transport http192.168.159.0/24and some outside it)192.168.159.10— verify only that host is returned192.168.159.0/24— verify only hosts in that subnet are returnedonly_up=true— verify results are filtered correctly