rapid7 / rex-core

Created by David Maloney via the GitHub Connector
Other
4 stars 23 forks source link

Prevent :dup attempts on refcounted objects #19

Closed sempervictus closed 2 years ago

sempervictus commented 2 years ago

During development of the LDAP service for log4j efforts, a change in the code after my unfortunate/inadvertent departure from daily operations in Framework surfaced in the interaction between low level Rex::ServiceManager objects and the Scanner mixin. The mixin uses a "new" method called :replicant which appears to assume that all instance variables in a MetasploitModule are without state and can therefore be :dup'd without safety latches. At least when it comes to Rex' services, this is usually not the case as that level of infrastructure tends to handle system resource abstractions such as bound/listening sockets which cannot be duplicated. The result of this interaction is "loss" of service references such that the originally started service cannot be accessed by any means from within the framework but is still bound to the sockets, preventing their use until Ruby is shut down.

After reviewing the issue with @Op3n4M3, @zeroSteiner, and other members of the R7 team, two viable architectura approaches were identified:

  1. Rewrite the :replicant logic to account for state in the vars which it duplicates. This is fraught and dangerous - the effort to implement this was heroic in itself (we, the community, have had that feature request in the air since long before R7 bought MSF).
  2. Dig down deeper into the core and identify interception points for the breaking :dup call and attempt to mitigate there.

Given that option 1 would still permit another caller to dup the unique object, this commit targets option 2 by aliasing the :dup method to the :ref method in the Ref mixin which is part of the Rex::Service extension applied by Rex::ServiceManager during its :start call.

Testing: not enough - this could have wide-ranging impacts, break all sorts of things because rex-core is... the core. Requesting all hackers with clever ideas and a propensity for having Murphy's Law impact all that they do to run this through its paces hard and repeatedly.

sempervictus commented 2 years ago

If you're out there @hdm, would love your thoughts on this as well as i'm pretty sure the original code predates my involvement and the event/sync system was your design AFAIK.

hdm commented 2 years ago

I am too far out of the loop to be helpful here, but my 0.02 is that service states should definitely not be duplicated, and finding some way to avoid that and reinitialize after the replicant copy is created would be better.

Alternatively, treat this like HTTP server, where multiple modules can use it at once

sempervictus commented 2 years ago

I am too far out of the loop to be helpful here, but my 0.02 is that service states should definitely not be duplicated, and finding some way to avoid that and reinitialize after the replicant copy is created would be better.

Alternatively, treat this like HTTP server, where multiple modules can use it at once

That is brilliant sir (as always)! If we alter the service architecture of the lower-level protocols to handle resources for instantiation instead of socket-level binds, then we could keep them idempotent across accessors since handling a duplicate managed resource beats trying to manage an OS-marshalled one any day. Thanks for the input @hdm, i'll look into what this would take for existing Rex services and their Msf mixin consumers.

Curious about this "out of the loop" notion - i was under the impression that your storage subsystem was a well-indexed append-only log database; or have you figured out a mechanism to rm segments from there without breaking the linked-lists? :heart: I actually just stepped back in for a bit to help out with the l4j thing and suddenly i have a task list in front of me :wink:

gwillcox-r7 commented 2 years ago

@adfoster-r7 @jmartin-r7 Do we have any insights or updates on this? Happy to try look into this and push this forwards a bit but seems like progress on this has stalled.

adfoster-r7 commented 2 years ago

Thanks Grant I had forgotten to close this, this PR was actually superseded by: