metasploit-framework icon indicating copy to clipboard operation
metasploit-framework copied to clipboard

Update Railgun definitions to be compatible with x64

Open gwillcox-r7 opened this issue 2 years ago • 5 comments

Fixes the issue mentioned at https://github.com/rapid7/metasploit-framework/pull/17166#issuecomment-1374131359 which occurred due to pointer truncation on x64 payloads when the return values were incorrectly declared as being 32 bit long pointers for some Railgun functions. Whilst I was at it I also went ahead and updated several other functions in the code related to this one and fix one incorrect parameter type instance that was unrelated.

The good news is that most of these functions from what I can tell aren't currently being used by our code so there should be minimal side effects should something go wrong.

Verification

List the steps needed to make sure this thing works

  • [ ] Start msfconsole
  • [ ] Open up an IRB session.
  • [ ] Use railgun.advapi32.<function name here> and see if calling the function results in a crashing session or not. With the fixes this should not be the case.
  • [ ] Alernatively also follow the testing steps I took over at https://github.com/rapid7/metasploit-framework/pull/17166 and confirm that the bug exists on master and not on this branch. You may also need to apply the changes from this branch to the feature-kerberos-authentication branch though in order for this to fully work. I haven't targeted that branch though since I don't think that is the right branch for this type of change but let me know if I'm wrong on this.

gwillcox-r7 avatar Jan 07 '23 01:01 gwillcox-r7

Note this only updates the definitions for def_advapi32.rb using the regex PDWORD['"].*['"]out['"]. The following files also need review still:

  • [ ] def_kernel32.rb
  • [ ] def_netapi32.rb
  • [ ] def_ntdll.rb
  • [ ] def_user32.rb
  • [ ] def_version.rb
  • [ ] def_winspool.rb
  • [ ] def_wlanapi.rb
  • [x] def_wldap32.rb
  • [x] def_ws2_32.rb

A lot of these are fairly short to review containing only 3-5 things to review but others like kernel32 have 97 items to review and might take a little bit longer.

gwillcox-r7 avatar Jan 07 '23 01:01 gwillcox-r7

On an unrelated side note def_wldap32.rb seems to only be used by 1-2 post modules that don't even seem to work with the LDAP server I have on Windows Server 2022. I wonder if we may want to consider updating them to use the new Net::LDAP interface and remove the def_wldap32.rb file if no one is really using it and we have alternatives that don't rely on having a session on the system.

gwillcox-r7 avatar Jan 07 '23 04:01 gwillcox-r7

Welp well looks like trying to change a few of these is having unintended side effects during testing. I'm thinking it may be better to just fix what is known at this stage. A lot of these functions aren't even used anywhere near as I can tell or are at best only used in a select few places.

gwillcox-r7 avatar Jan 07 '23 05:01 gwillcox-r7

Note above regex wasn't complete. A better search is \[['"]PDWORD['"].*['"](in)?out['"]\] which also takes into account the fact that inout is a valid output buffer type, and better matches on the expected data format.

tekwizz123 avatar Jan 08 '23 20:01 tekwizz123

Moving this into draft state. Internal discussions on best approaches need to take place and I don't want to give the impression that this is in merge ready state cause the reality is whilst we could merge this is in now; it wouldn't be an appropriate fix without taking into consideration other factors.

gwillcox-r7 avatar Jan 27 '23 15:01 gwillcox-r7

@gwillcox-r7 Are you planning to return to this or should we close it for now?

smcintyre-r7 avatar Mar 07 '23 19:03 smcintyre-r7

@smcintyre-r7 I think the original idea with this PR was to do everything at once but later realized that we should likely break them down into smaller PRs. Its still useful for tracking some progress and files that need addressing however beyond that I don't think it has as much use anymore.

gwillcox-r7 avatar Mar 07 '23 20:03 gwillcox-r7

Closing PR due to above points, we should take the approach of making smaller PRs to solve the remaining issues mentioned here.

gwillcox-r7 avatar Mar 07 '23 20:03 gwillcox-r7