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

Fix incorrect definitions for ldap_search functions in def_wldap32.rb

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

This fixes up some incorrect definitions within def_wldap32.rb so it can work with x64 Windows. Note that I'm not 100% sure if these definitions are correct or not so some verification might be needed.

Also note ldap_search_sA is presently used by the lib/msf/core/post/windows/ldap.rb library via the query_ldap function, which is in turned used by the get_default_naming_context and query functions.

Therefore, this might need some verification to make sure it doesn't break anything.

Verification

  • [ ] Review the PR and confirm the definitions match MSDN's documentation.
  • [ ] Get a Meterpreter session on a Windows x64 host.
  • [ ] From the main console run loadpath test/modules/ to load the test modules.
  • [ ] post/test/railgun should work, all tests should pass
  • [ ] post/test/railgun_reverse_lookups should also work with all tests passing

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

Note for testing might be an idea to use a module such as modules/post/windows/gather/enum_ad_users.rb and check that is still working.

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

Found a few other functions with incorrect return definitions. Attempted to update them but not 100% sure if the definitions are correct. Let me know if you see anything that looks odd. Rebased to fix the merge conflict.

gwillcox-r7 avatar Feb 02 '23 17:02 gwillcox-r7

I'm still having a real hard time testing. I can't seem to get enum_ad_users to run. It's failing as it's trying to bind, and I don't know what's wrong. I'm wondering if the domain parameter to ldap_sslinitA is in the incorrect format because the docs say it's supposed to be a hostname, but the module says it's a DN.

Either way I can't seem to get it to work on my 2019 DC as either system or running as a domain admin.

Additionally, before I got to this point, I had to make some updates to mark PULONG_PTR and PCHAR as supported types for return values. I don't know if this code works, I'm guessing it needs changes but it at least allowed the module to load and start running.

diff --git a/lib/rex/post/meterpreter/extensions/stdapi/railgun/library.rb b/lib/rex/post/meterpreter/extensions/stdapi/railgun/library.rb
index d06ee4b73a..0bb1da763f 100644
--- a/lib/rex/post/meterpreter/extensions/stdapi/railgun/library.rb
+++ b/lib/rex/post/meterpreter/extensions/stdapi/railgun/library.rb
@@ -300,7 +300,7 @@ class Library
 
     # process return value
     case function.return_type
-      when 'LPVOID', 'ULONG_PTR'
+      when 'LPVOID', 'PULONG_PTR', 'ULONG_PTR'
         if arch == ARCH_X64
           return_hash['return'] = rec_return_value
         else
@@ -316,6 +316,10 @@ class Library
         return_hash['return'] = (rec_return_value != 0)
       when 'VOID'
         return_hash['return'] = nil
+      when 'PCHAR'
+        return_hash['return'] = read_string(rec_return_value)
+      when 'PWCHAR'
+        return_hash['return'] = read_wstring(rec_return_value)
       else
         raise "unexpected return type: #{function.return_type}"
     end
diff --git a/lib/rex/post/meterpreter/extensions/stdapi/railgun/library_function.rb b/lib/rex/post/meterpreter/extensions/stdapi/railgun/library_function.rb
index 15b2ec932b..53ee561685 100644
--- a/lib/rex/post/meterpreter/extensions/stdapi/railgun/library_function.rb
+++ b/lib/rex/post/meterpreter/extensions/stdapi/railgun/library_function.rb
@@ -43,9 +43,9 @@ class LibraryFunction
     'LPVOID'     => ['in', 'return'], # sf: for specifying a memory address (e.g. VirtualAlloc/HeapAlloc/...) where we don't want to back it up with actual mem ala PBLOB
     'ULONG_PTR'  => ['in', 'return'],
     'PDWORD'     => ['in', 'out', 'inout'], # todo: support for functions that return pointers to strings
-    'PULONG_PTR' => ['in', 'out', 'inout'],
-    'PWCHAR'     => ['in', 'out', 'inout'],
-    'PCHAR'      => ['in', 'out', 'inout'],
+    'PULONG_PTR' => ['in', 'out', 'inout', 'return'],
+    'PWCHAR'     => ['in', 'out', 'inout', 'return'],
+    'PCHAR'      => ['in', 'out', 'inout', 'return'],
     'PBLOB'      => ['in', 'out', 'inout'],
   }.freeze

I'm pretty confident we'll need something to that effect so those return types can work. The calls to read_string are what I'm unsure about, but I can't get the module to run to that point for testing. ~~Do you have any ideas?~~

Edit: This was the bind culprit. The type for ld needs to be LPVOID. It should also probably be consistently defined that way instead of using PHANDLE sometimes as it is for ldap_search_sA.

smcintyre-r7 avatar Feb 02 '23 19:02 smcintyre-r7

Looks like setting the return type to LPVOID as you suggested didn't work, but ULONG_PTR or PULONG_PTR works, likely as the former isn't treated as a pointer type.

gwillcox-r7 avatar Feb 02 '23 22:02 gwillcox-r7

sigh So I found that we are calling a function in RailGun with the wrong definition, and not only that but we were also calling it incorrectly. So yeah, I don't really know how that was working if it ever worked at all but there you go 🤷 ldap_set_option should have its last parameter be a pointer to a parameter value but instead we are just passing it the parameter itself directly.

gwillcox-r7 avatar Feb 02 '23 22:02 gwillcox-r7

Seems like DWORD parameters are AND'd with 0xffffffff before being packed in their native architecture, presumably to truncate it down to a 32 bit number. I'm not sure why this action is making ldap_set_option work but it is. As for where this value is retrieved it seems to be from updated return value of ldap_sslinitA.

gwillcox-r7 avatar Feb 02 '23 23:02 gwillcox-r7

Alright since this is now kind of in an odd place lets break this down by the functions defined and which ones have been tested vs not tested so we know where this PR stands.

Functions

Used

  • [x] ldap_unbind
  • [x] ldap_sslinitA
  • [x] ldap_bind_sA
  • [x] ldap_search_sA
  • [x] ldap_set_option
  • [x] ldap_count_entries
  • [x] ldap_first_entry
  • [x] ldap_msgfree

Unused

  • [ ] ldap_search_ext_sA
  • [ ] ldap_next_entry
  • [ ] ldap_first_attributeA
  • [ ] ldap_next_attributeA
  • [ ] ldap_count_values
  • [ ] ldap_get_values
  • [ ] ldap_value_free
  • [ ] ldap_memfree
  • [ ] ber_free
  • [ ] ldap_err2string

gwillcox-r7 avatar Feb 10 '23 16:02 gwillcox-r7

@smcintyre-r7 Reviewed all used functions that have been modified and confirmed they work on a x64 target. Is there any additional work that needs to be done for this to be landed?

gwillcox-r7 avatar Feb 10 '23 17:02 gwillcox-r7

Per Slack convo, squashing these commits down before we go ahead and land this.

gwillcox-r7 avatar Feb 23 '23 14:02 gwillcox-r7

Release Notes

This fixes some incorrect Railgun definitions for the wldap32 Windows library.

smcintyre-r7 avatar Feb 23 '23 19:02 smcintyre-r7