New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed smb_enumshares to support dir list in SRVSVC #2150

Merged
merged 3 commits into from Jul 24, 2013

Conversation

Projects
None yet
4 participants
@webstersprodigy
Contributor

webstersprodigy commented Jul 24, 2013

There's a kind of "by design" bug with smb_enumshares where it wasn't listing directories when using srvsvc, which some environments need by default. After some debugging, it seems like the problem is because srvsvc adds a null byte. When accounting for this, it seems to work.

Tested with and without USE_SRVSVC_ONLY with windows xp, and with server 2008 r2.

@mubix

This comment has been minimized.

Contributor

mubix commented Jul 24, 2013

Works wonderfully! win7, win7x64, win2k8, win2k3, xpsp3

@mubix

This comment has been minimized.

Contributor

mubix commented Jul 24, 2013

Examples: XP

[+] 172.16.10.8:139 \\PROJECTMENTOR\XPSP3\Desktop (DISK) Readable Writable
======================================================================

 Type  Name                   Created              Accessed             Written              Changed              Size
 ----  ----                   -------              --------             -------              -------              ----
 ARC   autoruns.exe           05-03-2013 13:21:47  07-22-2013 15:50:16  03-24-2013 23:24:00  06-27-2013 21:25:34  659456
 ARC   bob.exe                04-29-2013 11:34:34  07-22-2013 15:50:16  04-29-2013 11:34:35  05-03-2013 13:17:12  2461696
 ARC   ComplexPath.exe        06-27-2013 01:16:34  07-22-2013 15:50:16  06-27-2013 01:16:02  07-22-2013 15:50:16  57344

Examples: Win7x64

[+] 172.16.10.9:445 \\PROJECTMENTOR\WIN7X64\ADMIN$ (DISK) Readable Writable
========================================================================

 Type  Name                      Created              Accessed             Written              Changed              Size
 ----  ----                      -------              --------             -------              -------              ----
 ARC   bfsvc.exe                 11-20-2010 22:24:46  11-20-2010 22:24:46  11-20-2010 22:24:46  08-22-2012 02:17:14  73728
 ARC   fveupdate.exe             07-13-2009 19:22:13  07-13-2009 19:22:13  07-13-2009 21:39:10  08-22-2012 02:17:14  16384

Examples: Win2k8 Domain Controller:

[+] 172.16.10.10:445 \\PROJECTMENTOR\DC1\ADMIN$ (DISK) Readable Writable
=====================================================================

 Type  Name                      Created              Accessed             Written              Changed              Size
 ----  ----                      -------              --------             -------              -------              ----
 ARC   HelpPane.exe              07-13-2009 20:29:53  07-13-2009 20:29:53  07-13-2009 21:39:12  08-22-2012 01:54:45  737280
 ARC   fveupdate.exe             07-13-2009 19:22:13  07-13-2009 19:22:13  07-13-2009 21:39:10  08-22-2012 01:54:45  16384
@mubix

This comment has been minimized.

Contributor

mubix commented Jul 24, 2013

Only odd behavior and this could be unrelated to your code is that SYSVOL and NETLOGON were not dir listed. Just ADMIN$ and C$. Testing. I think this mod is ready for commit IMHO though.

@mubix

This comment has been minimized.

Contributor

mubix commented Jul 24, 2013

Created a new share on XP and Win7X64, it listed just fine, next testing if I create a new share on the DC if it will show.

@mubix

This comment has been minimized.

Contributor

mubix commented Jul 24, 2013

No joy on Win2k8 created a new "Temp" share. Didn't list.

@wvu-r7

This comment has been minimized.

Contributor

wvu-r7 commented Jul 24, 2013

Thanks for testing, @mubix!

@webstersprodigy

This comment has been minimized.

Contributor

webstersprodigy commented Jul 24, 2013

Thanks for the tests @mubix! The changes in the commit shouldn't affect the shares being listed, since the only code path being changed was the dirlist option (after the shares are already enumerated), but yeah, I've noticed the same thing where some shares sometimes don't list

# srvsvc adds a null byte that needs to be removed
if datastore['USE_SRVSVC_ONLY']
share = share[0..-2]

This comment has been minimized.

@wvu-r7

wvu-r7 Jul 24, 2013

Contributor

I think you should use something like chomp here. I'm just not a fan of indiscriminate slicing, even though a null byte is assumed... and I don't like making assumptions.

This comment has been minimized.

@webstersprodigy

webstersprodigy Jul 24, 2013

Contributor

Good point. I just chomped and removed the if statement altogether

@wvu-r7

This comment has been minimized.

Contributor

wvu-r7 commented Jul 24, 2013

I think the if statement was fine. Now it's chomp'ing even though USE_SRVSVC_ONLY is false by default.

@webstersprodigy

This comment has been minimized.

Contributor

webstersprodigy commented Jul 24, 2013

Sure, but the root of why it wasn't working with srvsvc is because a null byte was added, so the chomp should be good in either case, right? Tested with srvsvc in winxp and it should still work. But this is one I don't really care about either way :)

@webstersprodigy

This comment has been minimized.

Contributor

webstersprodigy commented Jul 24, 2013

*Tested with srvsvc set to false in winxp

@@ -47,7 +46,7 @@ def initialize(info={})
register_options(
[
OptBool.new('DIR_SHARE', [true, 'Show all the folders and files', false ]),
OptBool.new('USE_SRVSVC_ONLY', [true, 'List shares only with SRVSVC', false ])
OptBool.new('USE_SRVSVC_ONLY', [true, 'List shares with SRVSVC', false ])

This comment has been minimized.

@jvazquez-r7

jvazquez-r7 Jul 24, 2013

Contributor

I've not checked in depth but why is this description being changed :?

This comment has been minimized.

@webstersprodigy

webstersprodigy Jul 24, 2013

Contributor

I read USE_SRVSVC_ONLY to mean "only" list the shares versus DIR_SHARE since when it was set previously it would not try to dir_share, but re-reading I think I mis-read. I'll change that back

@jvazquez-r7

This comment has been minimized.

Contributor

jvazquez-r7 commented Jul 24, 2013

I guess not related to this pull request, but while eyeballing, noticed this module is doing things should be reviewed like:

  • Assigning datastore options at runtime:
                datastore['USE_SRVSVC_ONLY'] = true # Make sure the module is aware of this state

Later at cleanup:

    def cleanup
        datastore['RPORT']           = @rport
        datastore['SMBDirect']       = @smb_redirect
        datastore['USE_SRVSVC_ONLY'] = @srvsvc
    end

I'm not sure why is it being done, but ask myself if instance variables are not sufficient o manage the state if needed :?

By design we learned datastore options are thought to be set by the user, and shouldn't be changed at runtime from code.

@wvu-r7

This comment has been minimized.

Contributor

wvu-r7 commented Jul 24, 2013

@webstersprodigy: Fair enough. I can rationalize it as normalization.

wvu-r7 added a commit that referenced this pull request Jul 24, 2013

@wvu-r7 wvu-r7 merged commit 9d03276 into rapid7:master Jul 24, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment