Skip to content
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.

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

Add Meterpreter compatibility requirements to lib #15842

Conversation

adfoster-r7
Copy link
Contributor

@adfoster-r7 adfoster-r7 commented Nov 8, 2021

Updating the Meterpreter compatibility requirements for mixins within the lib folder

Building on the work of #15295 and #15659

rubocop -A --only Lint/MeterpreterCommandCompatibility lib

Verification

Review the modules and verify that the command additions make sense

lib/msf/core/post/windows/process.rb Show resolved Hide resolved
lib/msf/core/post/windows/net_api.rb Show resolved Hide resolved
lib/msf/core/post/windows/services.rb Show resolved Hide resolved
lib/msf/core/post/windows/priv.rb Show resolved Hide resolved
stdapi_sys_process_*
stdapi_sys_process_execute
stdapi_sys_process_get_processes
stdapi_sys_process_kill
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core_channel_open might be needed for cmd_out.channel.read on line 257 of the updated file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should be core_channel_close and core_channel_read given its closing and reading from channels?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a entirely unrelated note we should really have a list centralized somewhere of each of these extension names and what they are used for; its a pain atm looking it up across multiple files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, will need to add this in a separate PR 👍

stdapi_sys_config_getprivs
stdapi_sys_config_getuid
stdapi_sys_process_execute
stdapi_sys_process_get_processes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that each_service here calls session.extapi.service.enumerate.each(&block) if extapi is loaded as per

def each_service(&block)
if load_extapi
session.extapi.service.enumerate.each(&block)
else
, do we want to require extapi_service_enum?

It does fall back but I mean the way this and other code particularly registry code is coded is that it hugely depends on if extapi is loaded and if it is but the Meterpreter doesn't support that feature, then things kinda get very odd and messed up very fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with

def service_info(name)
service = {}
if load_extapi
begin
return session.extapi.service.query(name)
rescue Rex::Post::Meterpreter::RequestError => e
vprint_error("Request Error #{e} falling back to registry technique")
end
end

Do we want to require extapi_service_query here as a result?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core_channel_read and core_channel_close potentially required here due to the way run_cmd operates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that each_service here calls session.extapi.service.enumerate.each(&block) if extapi is loaded as per

That will be covered by the mixin that provides the each_service function:

extapi_service_enum
extapi_service_query

core_channel_read and core_channel_close potentially required here due to the way run_cmd operates.

Will lump this in with the implicit nature of stdapi_sys_process_execute being associated with channel open/close/read/eof, can iterate on this in a future PR though 👍

lib/msf/core/post/windows/ldap.rb Show resolved Hide resolved
lib/msf/core/post/file.rb Show resolved Hide resolved
lib/msf/core/post/file.rb Show resolved Hide resolved
lib/msf/core/post/file.rb Show resolved Hide resolved
@gwillcox-r7
Copy link
Contributor

Pinging @adfoster-r7, any updates on fixing these issues? Only asking as its been nearly a month since the last review. If no time that's fine just want to make sure we have a status update on this so people looking at this PR know what is going on.

@adfoster-r7 adfoster-r7 force-pushed the add-meterpreter-compatibility-requirements-to-lib branch from c9af078 to 9dece7e Compare December 13, 2021 13:46
@adfoster-r7 adfoster-r7 force-pushed the add-meterpreter-compatibility-requirements-to-lib branch from 9dece7e to 48f4007 Compare December 13, 2021 13:47
@adfoster-r7
Copy link
Contributor Author

Cross-referencing #15949 too

This PR should fix the issue which stopped modules running as part of the local exploit suggester, as stdapi_sys_process_* - inadvertently required the new stdapi_sys_process_set_term_size command which isn't supported by all meterpreters

@gwillcox-r7
Copy link
Contributor

To clarify before landing there are a few features here that we still need to iterate on and improve. Namely they are:

Comments at #15842 (comment) and previous comments r.e lib/msf/core/post/windows/mssql.rb missing some potential requirements to further lock things down.

Comments at #15842 (comment) and #15842 (comment) r.e lib/msf/core/post/windows/powershell.rb not requiring certain features it likely needs to further lock things down.

Comment at #15842 (comment) r.e creating a centralized list of requirements and what each one is actually used for in some form of documentation. I'm happy to investigate this further in 2022 but will likely need some assistance as some feature have some edge cases that @adfoster-r7 or similar might have better insights into.

For now though this PR is fine and @adfoster-r7 and I both agree this is a good point to pause and land existing work, with the understanding that the automation needs to be improved and run again across the codebase, and that further iteration and improvements are needed on the above issue along with the documentation note as mentioned above.

@gwillcox-r7 gwillcox-r7 merged commit cc18c8d into rapid7:master Dec 13, 2021
@gwillcox-r7 gwillcox-r7 added the rn-enhancement release notes enhancement label Dec 13, 2021
@gwillcox-r7
Copy link
Contributor

Release Notes

Several libraries within the lib folder have now been updated to declare Meterpreter compatibility requirements, which will allow users to more easily determine when they are using a library that the current session does not support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants