Closed thelightcosine closed 9 years ago
Can one of the admins verify this patch? For more information see: https://github.com/rapid7/meterpreter/wiki/CI-Testing
this will need to be looked at by @busterb @OJ or @hmoore-r7 , or some combination thereof!
0r @bcook-r7 rather
Thanks @dmaloney-r7 ! I'm happy to look at this for you. There does seem to be some changes in areas that I wouldn't expect (for example base_dispatch.c
). Does that look right to you?
Cheers!
I think you might need to merge with master again :)
@oj, gack, no that doesn't look right. that might be because i had merged master then reverted the merge due to some issues. lemme try remerging master
@oj okay, i've reverted the revert, but the same issues may be back now. i'm going to wait for you to take a look and make sure it wasn't something i was screwing up in the testing procedure
You will need to be testing against PR #5214 in metasploit-framework for now.
Thanks guys. I'll have the migrate fix in soon for that other PR too.
@dmaloney-r7 Looks much better now mate. Thanks.
Sorry for the comment noise folks. It was @dmaloney-r7's first PR to Meterpreter in ages so I donned my super picky hat ;)
Nice module! Great work Dave. For the most part it looks good, just a few points where failure cases aren't covered and resources might be leaked. There's a few style issues, but I'll clean up and PR for you.
In other news.. that Jet API is horrible isn't it!
@oj , thanks, reviewing your comments now. and yes that API is atrocious
@oj i'm not familiar with doxygen, does it work like YARD? is there an example you can point me to?
the context, accountcolumns, and ntdsstate all get freed when the channel is closed. freeing them anytime before that could lead to a state where the user could still try and read on the channel and we'd get null dereference errors that would crash the session
@oj @bcook-r7 with the merge back in, i think i'm blocked until https://github.com/rapid7/metasploit-framework/pull/5214 lands so that i can merge those changes into my framework branch.
any further code review would be much appreciated on the C side for now though. I'm sure there's plenty of room for imrpovement
re doxygen, yes, it works just like YARD - just run 'doxygen doxygen.cnf' the framework bits should land today.
@dmaloney-r7 So, while this module is indeed something related to privileges coz you're pwning ntds, I'm thinking that this code doesn't belong in the priv
extension. The reason I say this is that, for the most part, you're not going to use this with every session, instead you're going to be targeting this when you've established a clear foothold with the appropriate account permissions. Given that priv
is wired into every Meterpreter session, this extra functionality will add bytes, but won't be used most of the time.
I'm thinking that it belongs in extapi
instead. What do you think? Am I way off?
@dmaloney-r7 Here are some samples of the Doxygen comments: https://github.com/rapid7/meterpreter/blob/master/source/extensions/extapi/service.c
In particular:
Hope that helps!
@oj , I can definitely see your point here. I guess my question is how much are we adding to the size of priv, and is it really disruptive? I feel like priv is the logical place for this functionality to live. However, if it's disprutive to have that added wieght there, then I can look at moving it.
my personal preference is to leave it in priv unless it's going to be a problem. partially because I think it makes sense there, that's where the other hashdump stuff is, and partially because I really don't want to pick all that code up move it and rewire it ;)
So I'm running into a strange issue here. I can open and close ntds channels ad infitium. as demonstarted with
50.times do
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
channel.close
end
and that works fine. I can open a channel, read all the accounts and close the channel once as demonstratable by
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
data = channel.read(78960)
channel.close
But if I open a channel, read and then close, twice it crashes the session, as demonstrated with
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
data = channel.read(78960)
channel.close
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
data = channel.read(78960)
channel.close
It also crashes if I don't read on the first time
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
channel.close
channel = client.priv.ntds_parse('c:\ntds\ntds.dit')
data = channel.read(78960)
channel.close
I'm really at a loss as to what's going on here
my sanity is hanging by a thread here. i cannot figure out what's causing this. If I do the second read through, but never close the channel, it never crashes. as soon as i close it, the whole thing crashed with an access violation
Haven't had any troubles at all reading/closing multiple times on 2012 r2. I'll try to setup a 2008 DC later to try to match your environment.
That compression flag isn't used anywhere else? Should it be? E.g. file downloads...
Ah looks like other stuff send the channel flags from the MSF side.
The beginning of ntdsAccount encodes a wchar_t string into the wire format as data is pulled from the stream:
struct ntdsAccount{
wchar_t accountName[20];
wchar_t accountDescription[1024];
Can you convert these to char UTF-8 encoded so we can keep meterpreter wire formats consistently UTF-8 where possible? It shouldn't be too hard to convert them, just WideCharToMultiByte (see common/unicode.c for example code). If all goes to plan, these two fields should not need any null character trimming done on the metasploit framework side other than the terminator.
I'm going to have a go at shepherding this along. Looks like a couple of memory leaks left but nothing terrible.
@bcook-r7 make sure you look at the updated copy, the conversation page didn't fully update. some of the issues juan brought up have been actually fixed, you just can't see it on this page. And thanks!
I made a number of tweaks and bugfixes before landing:
Since logon names can be longer than 20 characters, I extended it: 7b2e60a63eaa16b4181691d32f0b4d35ef814455
There were memory leaks using the UTF8 conversion functions: ddb0d108e3e1b8ee22e2faaa7879c1c5ea200e4d
I moved this from priv to extapi, since it does not actually perform privilege escalation: c6667dde2639ae61416f6ad1cd67d460a2b82c97
As a side-effect of the move, warnings were enabled, which revealed that many string functions were used incorrectly, specifying the source buffer length rather than destination for strncpy, strncat, etc. I converted these to use strncpy_s and friends.
Also, EOF handling on the stream was implemented incorrectly - returning anything other than SUCCESS in the read handler shuts down the whole meterpreter session, so I changed it to simply return 0 bytes instead: a6df4d9ef4c7229839634bfd53cf11e8e6969f95
This PR adds the NTDS parser to Meterpreter's priv extension As per MSP-12356
to setup for this have a Domain Controller with some accounts on it. Get a copy of NTDS.dit. On 2008 you can use
ntdsutil.exe "activate instance ntds" "ifm" "Create Full C:\Temp\123" quit quit
to get a copy. Move the NTDS.dit file to a friendly path. Open up a meterp session on that box thenVERIFICATION STEPS