Re: [NSE] Several changes to mssql.lua and SQL Server scripts

[Chris Woodbury <woodbusy () gmail com>]

I like the idea of stopping to clean up and focus on testing and merging.
I've done some testing on my own and gotten some other people to test as
well this week.

Based on all of that, I have a few comments:
* Now that we have instance-name and instance-port, I'm not sure that it's
intuitive that the way to hit all instances is by specifying
instance-name=all, especially considering the case where there are instances
we don't know the names of. How about having a script argument like
mssql.all-instances?
* I really like the idea that it's now possible to use SQL Server as a proxy
for doing Windows password testing. However, I think we should put up some
hurdles for the user. Virtually all the Windows networks I've worked with
have had account lockout policies in place, and I'm concerned that someone
could accidentally end up brute-forcing a domain and locking out a bunch of
accounts. I think we ought to add a script argument to enable it.
* I'm not sure I understand the rule changes for ms-sql-info. Going back to
a point you made previously about not forcing the user to know about how SQL
Server works, I think we ought to remove the portrule for 1434/udp and just
have it function as a host-script. If the user specified instances via
script-args, it will just show those; otherwise, it will show all instances.
How about that?

Also, it looks like ms-sql-discover didn't get changed by my last patch.
There should have been several changes there; can you take a look at that?

Let me know what you think about all of that. In the meantime, I'm going to
do some documentation.

-chris
On Sat, Feb 19, 2011 at 2:42 AM, Patrik Karlsson <patrik () cqure net> wrote:

> On Feb 18, 2011, at 23:23 , Chris Woodbury wrote:
>
> > So, I made some of the changes I talked about before. To summarize:
> > * I factored the typical hostrule and portrule logic into
> Helper.GetHostrule_Standard() and Helper.GetHostrule_Standard(). All but
> -info and the -discover scripts use these.
> >    * Typical scripts now run as the following:
> >       * No instance target specified (i.e. the mssql.instance-x script
> args): run as a port-script against ms-sql-s ports
> >       * Instance target(s) specified: run as a host-script against the
> targeted instance(s)
> >    * ms-sql-info now runs as the following:
> >       * No instance target specified: run as a host-script against all
> discoverable instance(s)
> >       * Instance target(s) specified: run as a host-script against the
> targeted instance(s)
> >    * ms-sql-discover will always run (as a host-script), unless
> restricted by mssql.scanned-ports-only (as before)
> > * I moved discovery logic from ms-sql-discover to Helper.Discover()
> (which now does full discovery, not just SSRP).
> > * I changed Helper.DiscoverByTcp() to return a table of instances, for
> consistency with the other DiscoverBy___ functions.
> > * I renamed Helper.GetInstanceByName() to Helper.GetTargetInstances() and
> expanded it to handle instance-port and to handle tables in instance-name
> and instance-port. Names from instance-name are also case-insensitive now.
> > The individual scripts are a lot cleaner and smaller now, and I think
> that the scripts are easier to use as well (e.g. not having to include
> ms-sql-discover in order to get good results).
> > On Thu, Feb 17, 2011 at 3:27 PM, Patrik Karlsson <patrik () cqure net>
> wrote:
> > There might be a problem with this approach. As ms-sql-discover runs it
> uses the information retrieved from the browser to determine what tcp ports
> exist.
> > WIth this information it then sets each of the ports as being
> fingerprinted as ms-sql-s. When the next script runs the portrule is
> triggered for each of these ports.
> > Unfortunately, I don't think there's currently a way to first connect to
> the browser, mark the ports as ms-sql-s and then trigger the portrule within
> the same script.
> > One way could be to place the discovery code within the hostrule or
> portrule function, however doing this causes the following stacktrace:
> > stack traceback:
> >        ./nse_main.lua:654: in function <./nse_main.lua:653>
> >        [C]: in function 'socket_lock'
> >        [string "local connect, socket_lock = ...;..."]:3: in function
> 'connect'
> > Another way is going down the path of forced dependencies, which I
> believe has been discussed in the past.
> >  In the version I'm sending, I think this works out. The only way that
> you would "miss" instances is if they were on a non-standard port and you
> didn't target them (e.g. mssql.instance-port=1435) and you didn't run
> ms-sql-discover. Otherwise, if it's on 1433, we'll find it; if the user uses
> mssql.instance[-name|-port], we'll find it; if ms-sql-discover is run and
> the instance is in the SQL Browser, we'll find it.
> > > > Taking a instance oriented approach, where it's up to Nmap (or the
> script really) to determine the correct ports to use, the commands for the
> above use cases would end up like this instead:
> > > > 1. No change
> > > > 2. ./nmap -p 1435 --script ms-sql-query 10.0.200.111 --script-args
> mssql.instance-name='SQLEXPRESS'
> > > > 3. ./nmap -p 1435 --script ms-sql-query 10.0.200.111 --script-args
> mssql.instance-port=1435
> > > > I realize that the port argument in the 3rd example is
> redundant/misleading/strange, but unfortunately necessary as we're running
> as a host rule at this point.
> > > > I guess that use case 3 could also be achieved through Martin Swende's
> proposed force patch [3] that would force the script to run against the port
> supplied using -p.
> > > I'm not completely convinced one way or the other, but I think I might
> > > prefer having an argument like mssql.force (we'd need a better name,
> > > though).
> >
> > I agree.
> >
> > In the version I'm sending, mssql.Helper.Discover() will take any ports
> specified by instance-port and will run DiscoverByTcp() on them. So, you
> could actually run it without specifying the port in the -p (in fact, it
> even works if you do a nmap -sn). Does that make sense to do it that way?
> > > * Am I wrong, or is it unnecessary to have 1433 in the portrule? Is it
> > > possible to have a situation in which there is a SQL Server available
> > > on 1433 and to have Nmap not classify it as "ms-sql-s"?
> >
> > The 1433 will allow the script to run even though no version/service
> scanning has been performed.
> > Actually, I tested it out, and I'm more sure now. Even if you don't do
> -sV, Nmap will still tag 1433 as "ms-sql-s." I changed the portrule for now
> to use shortport.service( "ms-sql-s" ). If we find that it needs to be the
> old way, it's easy to change back, since it's just in one place now
> (mssql.Helper.GetPortrule_Standard() ).
> > Let me know your thoughts. If you'll have time, let's also coordinate
> projects.
> > -chris
> > <20110218.patch>
>
> Thanks for the patch Chris. I've had a chance to do some testing and made a
> few changes:
> * fixed a typo in dependencies for the ms-sql-brute script that would make
> it fail to run
> * fixed a bug in the ms-sql-brute script that would make it abort password
> guessing if integrated authentication was used
> * changed so that if mssql.domain was specified without a domain a default
> domain is chosen
> * added script argument documentation to mssql.lua
> * removed the support for the mssql.instance argument in favor for the
> mssql.instance-name
>
> I also made some changes to the port- and host-rules of ms-sql-info, let me
> know if these makes sense?
> I've commited these changes and would like to focus on merging this branch
> to trunk now, before adding more functionality.
> In my opinion we need to complete the following before doing so:
> * decide on the host-, port-rule change I made to ms-sql-info
> * go over the scripts and library and add any missing documentation
> * do some additional testing
> * anything else?
>
> Unless we find something major during testing I think it would be possible
> to merge in the coming week.
> I can do some more testing today and tomorrow and fix any bugs that crop
> up.
>
> //Patrik
>
> --
> Patrik Karlsson
> http://www.cqure.net
> http://www.twitter.com/nevdull77
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/