rapid7 / rex-core

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

Migrate rex/logging into Core from Msf namespace #32

Closed sempervictus closed 1 year ago

sempervictus commented 1 year ago

Using Rex' various gems without Msf will result in errors when the logging subsystem is undefined (as that remained in Msf during the great Rex excision). This manifests in rex-socket as noted by @zeroSteiner in https://github.com/rapid7/rex-socket/issues/38.

Address the dependency problem by moving rex/logging into this gem which is already required by rex-socket and other descendants.

Testing: None - this is a quick-n-dirty subdirectory move. If this works, someone with real git skill should migrate the relevant historyof the code; as losing that stuff results in people not knowing whom to ask when the time comes to fix some deeply-bored bug.

adfoster-r7 commented 1 year ago

I'm guessing this is an issue other gems as well? i.e. this isn't a viable fix in isolation?

$ ls | grep rex | xargs egrep -ir 'elog|wlog|ilog|rlog|dlog'

rex-core/lib/rex/io/socket_abstraction.rb:              wlog('monitor_rsock: the remote socket has been closed, exiting loop')
rex-core/lib/rex/io/socket_abstraction.rb:              wlog("monitor_rsock: exception during select: #{e.class} #{e}")
rex-core/lib/rex/io/socket_abstraction.rb:                  wlog('monitor_rsock: closed remote socket due to nil read')
rex-core/lib/rex/io/socket_abstraction.rb:                dlog('monitor_rsock: EOF in rsock')
rex-core/lib/rex/io/socket_abstraction.rb:                wlog("monitor_rsock: exception during read: #{e.class} #{e}")
rex-core/lib/rex/io/socket_abstraction.rb:                    wlog('monitor_rsock: failed writing, socket must be dead')
rex-core/lib/rex/io/socket_abstraction.rb:                  wlog("monitor_rsock: exception during write: #{e.class} #{e}")
rex-core/lib/rex/io/stream_server.rb:              elog('The accept() returned nil in stream server listener monitor')
rex-core/lib/rex/io/stream_server.rb:            elog("Error in stream server server monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:            rlog(ExceptionCallStack)
rex-core/lib/rex/io/stream_server.rb:            elog("Error in stream server client monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:            rlog(ExceptionCallStack)
rex-core/lib/rex/io/stream_server.rb:          elog("Error in stream server client monitor: #{$!}")
rex-core/lib/rex/io/stream_server.rb:          rlog(ExceptionCallStack)
rex-socket/lib/rex/socket.rb:      elog("#{e.message} (#{e.class})#{e.backtrace * "\n"}\n", LogSource, LEV_3)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read cert: #{e.class}: #{e}", LogSource)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read client cert: #{e.class}: #{e}", LogSource)
rex-socket/lib/rex/socket/parameters.rb:        elog("Failed to read client key: #{e.class}: #{e}", LogSource)
sempervictus commented 1 year ago

Socket needs Core, so i think that if we can make Core happy by moving this in here, Socket should "become" happy as well.

sempervictus commented 1 year ago

ping @adfoster-r7 - any blockers on this one? I cant rip these files from the Framework side until we have this PR merged :smile:

adfoster-r7 commented 1 year ago

No blockers; But since this will auto release as a gem bump on merge - it'd be good to have a the metasploit-framework PR prepped waiting to go so we can ship both one after the other 👀

sempervictus commented 1 year ago

@adfoster-r7 - Msf side staged, should be good to roll. Requesting manual validation post-merge as my environment deviates significantly from upstream's such that "what works here may not work for everyone."

sempervictus commented 1 year ago

Is there anyone around who knows how to move or clone the git history for these files from the MSF repo into this one? We want to preserve those histories as much as possible so subsequent generations can hunt down and yell at/beg for help from the original authors :smile:

adfoster-r7 commented 1 year ago

Good idea :+1:

Should be able to look into this after 6.3 is released - unless someone gets nerds sniped on it sooner 😄

gwillcox-r7 commented 1 year ago

@adfoster-r7 This looks good to me, can you clarify if there is anything you would need from my side in order to get this landed? I think you wanted to make sure there was the corresponding Metasploit Framework PR and that seems to be up at the moment.

adfoster-r7 commented 1 year ago

@gwillcox-r7 The latest was this comment: https://github.com/rapid7/metasploit-framework/pull/17506#issuecomment-1450392145

I'm not super opinionated on preserving the history

adfoster-r7 commented 1 year ago

This PR won't work in its current form, attempting to log raises an error:

2.7.2 :001 > require 'rex/core'
 => true 
2.7.2 :002 > elog('a')
Traceback (most recent call last):
        7: from /Users/user/.rvm/gems/ruby-2.7.2/bin/ruby_executable_hooks:22:in `<main>'
        6: from /Users/user/.rvm/gems/ruby-2.7.2/bin/ruby_executable_hooks:22:in `eval'
        5: from /Users/user/.rvm/gems/ruby-2.7.2/bin/irb:23:in `<main>'
        4: from /Users/user/.rvm/gems/ruby-2.7.2/bin/irb:23:in `load'
        3: from /Users/user/.rvm/rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        2: from (irb):2
        1: from /Users/user/Documents/code/rex-core/lib/rex/logging/log_dispatcher.rb:160:in `elog'
NameError (uninitialized constant LOG_ERROR)
Did you mean?  LoadError
               IOError

It turns out that msf console is mutating the global Kernel to include the rex logging constants:

[13] pry(#<Msf::Framework>)> Kernel.const_source_location(:LOG_ERROR)
=> ["/Users/user/Documents/code/metasploit-framework/lib/rex/logging.rb", 8]

Which we can see here:

https://github.com/rapid7/metasploit-framework/blob/c6547737a6f28252c6638409e61d0e86ece6fc81/lib/msf.rb#L5

So this PR would need to be modified to work outside of the context of msfconsole still; Taking a step back though - including a gem probably shouldn't globally define methods such as elog etc, so I'm not sure this PR is a good idea in isolation either.

adfoster-r7 commented 1 year ago

Will mark this as attic'd for now - to help keep the PR queue tidy; it looks like there's more work required in other places to actually wire this up correctly (context) - regardless of preserving Git history :+1:

Just for a paper trail: Cross-referencing the other test PR https://github.com/rapid7/metasploit-framework/pull/17506#issuecomment-1584889282 which I think had more of the missing pieces than this PR