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

[Chris Woodbury <woodbusy () gmail com>]

Patrik-

I had a discussion with one of my colleagues this weekend which left me
wondering whether there is any need now for the ms-sql-discover script.
Originally, of course, it did all of the discover, but since we ditched that
approach a while ago, I feel that it's basically just ms-sql-info without
the accurate version detection (in fact, you could get the same
functionality with nmap -sSU -p T:445,U:1434 --script ms-sql-info
--script-args mssql.scanned-ports-only). At the same time, I think it's
potentially confusing to users as to what it is and whether it needs to be
used. So, unless you feel otherwise, I say let's delete it.
-chris
On Sat, Feb 26, 2011 at 4:44 PM, Patrik Karlsson <patrik () cqure net> wrote:

> On Feb 26, 2011, at 21:06 , Chris Woodbury wrote:
>
> > Thanks for catching the unintended change in -brute. I was testing
> something
> > out and forgot to revert before I updated the documentation. I should do
> a
> > better job of checking my patches. Everything else sounds good, though;
> > let's merge it!
>
> Ok, it's merged into trunk. Hopefully, without braking anything.
>
>
> > -chris
> >
> > On Sat, Feb 26, 2011 at 4:04 AM, Patrik Karlsson <patrik () cqure net>
> wrote:
> > > On Feb 25, 2011, at 23:31 , Chris Woodbury wrote:
> > >
> > > > On Fri, Feb 25, 2011 at 1:53 PM, Patrik Karlsson <patrik () cqure net>
> > > wrote:
> > > > On Feb 25, 2011, at 00:15 , Chris Woodbury wrote:
> > > > > 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've added a instance-all argument to align better with the other
> names.
> > > Does that make sense?
> > > > Good idea.
> > > >
> > > > > * 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.
> > > > At first, I didn't understand the problem here, as you previously wrote
> > > that you modified the script to abort once it detected a lockout.
> > > > When I tested it I noticed what happened, and that there's no way of
> > > detecting that an account has been locked out based on the error message
> > > returned by the server.
> > > > I agree that there's a risk of locking out a number of accounts,
> > > potentially the whole domain, if you run the script without knowing what
> > > your doing or how it works.
> > > > Either we document this with a few exclamation marks, or we simply fail
> > > to start the script (if Windows authentication is used) and return an
> > > explanation why and instructions how to proceed.
> > > > We could use an argument that tells the script how many attempts to
> > > perform for each account, if unset and the authentication method is set
> to
> > > Windows, the script won't start.
> > > > How about that?
> > > >
> > > > Actually, since you mention it, it might be nice to have an "only do
> this
> > > many attempts" argument for SQL Server authentication as well, since
> those
> > > can also have lockouts. But maybe that's just something the user should
> > > control with the password database (incidentally, the brute script now
> tries
> > > the username as the password - should that be configurable?).
> > > > Anyway, I'd be fine with doing something like that for Windows at
> least,
> > > or maybe just a ms-sql-brute.brute-windows-accounts argument, so that
> it's
> > > explicit. I think either would be fine, as long as it's an extra step
> that
> > > is required and is only applicable for Windows authentication.
> > > I think controlling the amount of tries should be done by the number of
> > > entries in the password database.
> > > Since we detect account lockouts when sql authentication is used, we
> don't
> > > need an additional hurdle here in my opinion.
> > >
> > > I've made the following changes to the ms-sql-brute script:
> > > * If the script is started ONLY with the mssql.domain argument, it
> returns
> > > the following message:
> > > | ms-sql-brute:
> > > |   Windows authentication was enabled but the argument
> > > |   ms-sql-brute.brute-windows-accounts was not given. As there is
> > > currently no
> > > |   way of detecting accounts being locked out when Windows
> authentication
> > > is
> > > |   used, make sure that the amount entries in the password list
> > > |_  (passdb argument) are at least 2 entries below the lockout
> threshold.
> > > When the ms-sql-brute.brute-windows-accounts argument is given it runs
> as
> > > expected.
> > > Regarding changes to the ms-sql-brute script I would like to re-write it
> so
> > > that it uses the brute library I wrote a while back. But let's merge
> this
> > > first :)
> > >
> > > > > Let me know what you think about all of that. In the meantime, I'm
> > > going to do some documentation.
> > > > Great!
> > > >
> > > > I took a shot at this in the attached patch; let me know what you
> think.
> > > There's also a small bugfix in there in mssql.lua, where I changed a
> "host"
> > > to "instance.host", which fixed a bug for broadcast-ms-sql-discover.nse
> > > I've applied it but it failed on the ms-sql-discover script, so I did
> that
> > > one manually.
> > > Also it removed the change to ms-sql-brute which makes it work together
> > > with Windows authentication, so I added that again.
> > >
> > > I've done a bunch of testing too and unless there's something else, I
> think
> > > we should try to merge this?
> > >
> > > //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/
>
>  --
> 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/