[NSE] smb-ls fixes and improvements

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[NSE] smb-ls fixes and improvements

Pierre LALET
Hi list,

Here is a patch that:

  - Fixes a bug in smb.lua (smb-ls was broken, at least against a
    Samba service I found).

  - Allows smb-ls to be used with multiple shares at once and creates
    a default value ("\") for its `path` argument.

  - Allows smb-enum-shares to tell smb-ls which share to browse when
    no share has been specified.

I'm new to LUA & NSE so forgive me if my code is not as good as it
should be.

I have tried to make sure that the script smb-ls and smb-enum-shares
still work as before when used with the same arguments (no API was
harmed in the making of this patch).

I think we should consider to use 1 as default value when no value has
been set for `maxdepth` parameter (for now it defaults to 0 which can
take a *really* long time to complete on some shares, 1 would mean "no
recursion").

I did not add that in this patch because that would change the current
behavior of smb-ls.

Of course, remarks and comments welcome!

--
Pierre
http://pierre.droids-corp.org/

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/

smb-ls.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Henri Doreau
2015-03-07 20:36 GMT+01:00 Pierre LALET <[hidden email]>:

> Hi list,
>
> Here is a patch that:
>
>   - Fixes a bug in smb.lua (smb-ls was broken, at least against a
>     Samba service I found).
>
>   - Allows smb-ls to be used with multiple shares at once and creates
>     a default value ("\") for its `path` argument.
>
>   - Allows smb-enum-shares to tell smb-ls which share to browse when
>     no share has been specified.
>
> I'm new to LUA & NSE so forgive me if my code is not as good as it
> should be.
>
> I have tried to make sure that the script smb-ls and smb-enum-shares
> still work as before when used with the same arguments (no API was
> harmed in the making of this patch).
>
> I think we should consider to use 1 as default value when no value has
> been set for `maxdepth` parameter (for now it defaults to 0 which can
> take a *really* long time to complete on some shares, 1 would mean "no
> recursion").
>
> I did not add that in this patch because that would change the current
> behavior of smb-ls.
>
> Of course, remarks and comments welcome!
>
> --
> Pierre
> http://pierre.droids-corp.org/

Hi Pierre,

thanks for the patch. It looks very good. A couple minor comments though:

* improvement: Would it make sense to identify the service port on
which share was found in the host registry? So that scripts can
efficiently interact with hosts running multiple SMB instances?

* style: Limit depth of nested blocks. At the start of the action()
function of smb-ls, you can flatten the structure by doing:
"""
 -- give priority to specified shares if specified
  if arg_shares ~= nil then
     arg_shares = stdnse.strsplit(",", arg_shares)
  elseif arg_share ~= nil then
     arg_shares = {arg_share}
  else
     arg_shares = host.registry['smb_shares']
  end
"""

* It was already in the script, but this lua idiom either needs a
rewrite or a comment! :)
"""
  local lstab = tab.new((arg_checksum and 4 or 3))
"""

As for the behavior regarding recursion I would be ok with the default
value from zero to one, unless others have different opinion on the
matter. Anyone?


Regards

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Pierre LALET
Hi Henri,

Thanks for the review!

On Mon, Mar 09, 2015 at 07:09:49PM +0100, Henri Doreau wrote:
> * improvement: Would it make sense to identify the service port on
> which share was found in the host registry? So that scripts can
> efficiently interact with hosts running multiple SMB instances?

It could, but port discovery is hidden deep in the smb module (it
seems that start() discovers the ports by itself, giving priority to
445 if available).

> * style: Limit depth of nested blocks. At the start of the action()
> function of smb-ls, you can flatten the structure by doing:
> """
>  -- give priority to specified shares if specified
>   if arg_shares ~= nil then
>      arg_shares = stdnse.strsplit(",", arg_shares)
>   elseif arg_share ~= nil then
>      arg_shares = {arg_share}
>   else
>      arg_shares = host.registry['smb_shares']
>   end
> """

Agreed.

> * It was already in the script, but this lua idiom either needs a
> rewrite or a comment! :)
> """
>   local lstab = tab.new((arg_checksum and 4 or 3))
> """

Would this be helpful?

"""
           -- three columns per row, plus one for checksum if requested
           local lstab = tab.new((arg_checksum and 4 or 3))
"""

> As for the behavior regarding recursion I would be ok with the default
> value from zero to one, unless others have different opinion on the
> matter. Anyone?

No recursion seems to me a better default value than infinite
recursion for such a script, particularly with the share discovery
feature.

Thanks agains,

--
Pierre
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Daniel Miller-10
On Mon, Mar 9, 2015 at 2:00 PM, Pierre LALET <[hidden email]> wrote:

> As for the behavior regarding recursion I would be ok with the default
> value from zero to one, unless others have different opinion on the
> matter. Anyone?

No recursion seems to me a better default value than infinite
recursion for such a script, particularly with the share discovery
feature.

Whatever the decision, it should probably be unified among the various *-ls scripts:  afp-ls, gopher-ls, http-vlcstreamer-ls, nfs-ls, and smb-ls. Also, ftp-anon has a file listing feature, which should probably be made into a separate script with support for script-arg credentials.

Dan

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Pierre LALET
On Mon, Mar 09, 2015 at 04:28:35PM -0500, Daniel Miller wrote:
> > No recursion seems to me a better default value than infinite
> > recursion for such a script, particularly with the share discovery
> > feature.
>
> Whatever the decision, it should probably be unified among the various *-ls
> scripts:  afp-ls, gopher-ls, http-vlcstreamer-ls, nfs-ls, and smb-ls. Also,
> ftp-anon has a file listing feature, which should probably be made into a
> separate script with support for script-arg credentials.

Agreed. No recursion is the default for at least nfs-ls. I don't think
ftp-anon supports recursion at all.

--
Pierre
http://pierre.droids-corp.org/
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Henri Doreau
Hi,

Pierre has submitted two pull requests (for this and a NULL-deref) on
github. Respectively #89 and #90.
Dan: you may want to double check but both PR look good to me and can be landed.

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Henri Doreau
2015-03-23 12:49 GMT+01:00 Henri Doreau <[hidden email]>:
> Hi,
>
> Pierre has submitted two pull requests (for this and a NULL-deref) on
> github. Respectively #89 and #90.
> Dan: you may want to double check but both PR look good to me and can be landed.
>

I just merged it. Thanks again Pierre.

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Daniel Miller-10


On Thu, Apr 30, 2015 at 12:57 PM, Henri Doreau <[hidden email]> wrote:
2015-03-23 12:49 GMT+01:00 Henri Doreau <[hidden email]>:
> Hi,
>
> Pierre has submitted two pull requests (for this and a NULL-deref) on
> github. Respectively #89 and #90.
> Dan: you may want to double check but both PR look good to me and can be landed.
>

I just merged it. Thanks again Pierre.

Henri,

Thanks for looking at this. Since we can't merge directly on Github yet (still going through Subversion), it is helpful if you include a reference to the PR or issue in the commit message. Something like "Closes #90" or "Fixes #90" would have caused the PR to be automatically closed. I've closed it now, and referenced the appropriate commit.

And thanks, Pierre! I'm sure this improvement will be much-appreciated.

Dan

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Pierre LALET
In reply to this post by Henri Doreau
On Thu, Apr 30, 2015 at 07:57:39PM +0200, Henri Doreau wrote:
> > Pierre has submitted two pull requests (for this and a NULL-deref) on
> > github. Respectively #89 and #90.
> > Dan: you may want to double check but both PR look good to me and can be landed.
> >
>
> I just merged it. Thanks again Pierre.

Thanks a lot Henri.

As Daniel suggested to unify the default recursion settings among the
*-ls scripts (plus ftp-anon), I though it would be a good idea to
write an "ls" module to handle both the common options and the outputs
(structured and "readable") of those scripts.

I have implemented such a module and started to update the scripts to
use the module.

The most important benefit in the end is the unified output (and
particularly the structured output for me).

It's not totally complete but should be usable (I'm running continous
scans against random addresses to test the scripts). For now, I have
modified afp-ls, nfs-ls and smb-ls to use the module, and I am working
on ftp-anon. I also have added a script http-ls to gather information
from "index of" pages.

Do you think it could be of some interest? If so, should I create a PR
on Github or post the patch here?

Thanks,

--
Pierre
http://pierre.droids-corp.org/
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Daniel Miller-10
On Fri, May 1, 2015 at 6:29 AM, Pierre LALET <[hidden email]> wrote:
On Thu, Apr 30, 2015 at 07:57:39PM +0200, Henri Doreau wrote:
> > Pierre has submitted two pull requests (for this and a NULL-deref) on
> > github. Respectively #89 and #90.
> > Dan: you may want to double check but both PR look good to me and can be landed.
> >
>
> I just merged it. Thanks again Pierre.

Thanks a lot Henri.

As Daniel suggested to unify the default recursion settings among the
*-ls scripts (plus ftp-anon), I though it would be a good idea to
write an "ls" module to handle both the common options and the outputs
(structured and "readable") of those scripts.

I have implemented such a module and started to update the scripts to
use the module.

The most important benefit in the end is the unified output (and
particularly the structured output for me).

It's not totally complete but should be usable (I'm running continous
scans against random addresses to test the scripts). For now, I have
modified afp-ls, nfs-ls and smb-ls to use the module, and I am working
on ftp-anon. I also have added a script http-ls to gather information
from "index of" pages.

Do you think it could be of some interest? If so, should I create a PR
on Github or post the patch here?

Pierre,

I like this idea. Please create a PR so that we can more easily review the patch, but feel free to start a new message thread on the mailing list to discuss your work.

Dan

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Pierre LALET
Hi Dan, hi list,

On Fri, May 01, 2015 at 07:47:16AM -0500, Daniel Miller wrote:
> I like this idea. Please create a PR so that we can more easily review the
> patch, but feel free to start a new message thread on the mailing list to
> discuss your work.

PR #106 (https://github.com/nmap/nmap/pull/106) is ready, with, as
suggested, NSEDoc updated for each module.

--
Pierre
http://pierre.droids-corp.org/
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Henri Doreau
2015-07-30 15:05 GMT+02:00 Pierre LALET <[hidden email]>:

> Hi Dan, hi list,
>
> On Fri, May 01, 2015 at 07:47:16AM -0500, Daniel Miller wrote:
>> I like this idea. Please create a PR so that we can more easily review the
>> patch, but feel free to start a new message thread on the mailing list to
>> discuss your work.
>
> PR #106 (https://github.com/nmap/nmap/pull/106) is ready, with, as
> suggested, NSEDoc updated for each module.
>
> --
> Pierre
> http://pierre.droids-corp.org/

Hi,

I'd like someone else to have a look but these patches look good to me.

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [NSE] smb-ls fixes and improvements

Pierre LALET
Hi,

I have updated the nsedoc sections of the scripts as Henri has
suggested in a comment on a previous commit.

Henri: is it better now?

--
Pierre
http://pierre.droids-corp.org/

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/