Re: [nmap-svn] r12911 - nmap

[Brandon Enright <bmenrigh () ucsd edu>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, 9 Apr 2009 01:23:34 -0700
Fyodor <fyodor () insecure org> wrote:

> On Tue, Apr 07, 2009 at 02:51:36PM -0700, commit-mailer () insecure org
> wrote:
> > Author: bmenrigh
> > Date: Tue Apr  7 14:51:36 2009
> > New Revision: 12911
>
> Thanks Bradon!  I have a few questions/commonts about some of these
> changes.

By all means.  You bring up several good points which I hope to address
below.

> > -match http m|^HTTP/1\.1 \d\d\d .*\r\n\r\n.*var
> > l1=\"([^"]+)\"\n.*document\.write\(\"D-Link DI-\"\+l1\)|s p/D-Link
> > DI-$1 router http config/ d/router/ +match http m|^HTTP/1\.1
> > \d\d\d .*var l1=\"([^"]+)\"\n.*document\.write\(\"D-Link
> > DI-\"\+l1\)|s p/D-Link DI-$1 router http config/ d/router/
>
> Here the point of the "\r\n\r\n" was to make sure the variable
> declaration happened in the HTTP body rather than the headers.  But in
> this case the match is unique enough that there should be no problem.
> We're not going to see that stuff in the header anyway.

I too had a similar concern regarding dropping \r\n\r\n because it no
longer enforced the content to be matched in the body.  Unfortunately,
while the header only has the one \r\n\r\n and it's at the end, the
body can contain many and often does.  I had several services
with .*\r\n\r\n.* hitting the MATCHLIMIT because they were matching all
of the double line breaks in the body and backtracking too much.

> > -match gnutella m|^HTTP/1\.1 \d\d\d .*\r\n.*Server: BearShare
> > ([\d.]+)\r\n|s p/BearShare Gnutella P2P client/ v/$1/ o/Windows/
> > -match gnutella m|^HTTP/1\.1 \d\d\d .*\r\n.*Server: BearShare
> > ([\d.]+) \(([^)]+)\)\r\n|s p/BearShare Gnutella P2P client/ v/$1/
> > i/$2/ o/Windows/ +match gnutella m|^HTTP/1\.1 \d\d\d .*Server:
> > BearShare ([\d.]+)\r\n|s p/BearShare Gnutella P2P client/ v/$1/
> > o/Windows/ +match gnutella m|^HTTP/1\.1 \d\d\d .*Server: BearShare
> > ([\d.]+) \(([^)]+)\)\r\n|s p/BearShare Gnutella P2P client/ v/$1/
> > i/$2/ o/Windows/
>
> These seem reasonable.  They would match something like "Server:
> BearShare 2\r\n" even if it appeared in the middle of a line in the
> HTTP body rather than headers, but that was true of the match lines
> before your chanage as well.

Yeah as Doug pointed out, these were probably originally constructed
without the DOTALL (s) modifier and so it was necessary to keep track
of intermediate headers between "HTTP..." and "Server:".  When we went
and added the 's' modifier to our match lines trying to keep track of
intermediate headers actually backfires on us.

> > -match http m|^HTTP/1\.[01] \d\d\d .*\r\nDate: .*\r\n\r\n.*<META
> > NAME=\"GENERATOR\" CONTENT=\"WebLogic Server\">\n|s p/WebLogic
> > httpd/ +match http m|^HTTP/1\.[01] \d\d\d .*\r\nDate: .*<META
> > NAME=\"GENERATOR\" CONTENT=\"WebLogic Server\">\n|s p/WebLogic
> > httpd/
>
> Here again we lose the distinguishing between the header and body, but
> I think that is easily worth it to avoid the PCRE MATCHLIMIT problem.

Agreed.  All of the modifications are less exact now but I didn't see
any cases in which I thought that there could be a false positive.  The
body content being matched generally could never appear in any header.

> > -match http m|^HTTP/1\.0 \d\d\d .*\r\n.*: Quick 'n Easy Web
> > Server\r\n| p/Quick 'n Easy Web Server httpd/ o/Windows/ +match
> > http m|^HTTP/1\.0 \d\d\d .*: Quick 'n Easy Web Server\r\n|s p/Quick
> > 'n Easy Web Server httpd/ o/Windows/
>
> I don't understand the reasoning between ones like this which didn't
> have the s-modifier in the first place.  Previously, it enforced that
> the Quick 'n Easy line occurred on the 2nd line of the HTTP response.
> But with the change, it could even occur in the HTTP body.

Correct.

> Do you
> really get the PCRE MATCHLIMIT error in this sort of case where the .*
> cannot match \r\n in the first place?

No, although this was the only one.

> I guess I don't understand how
> this particular signature change is an improvement, though I'm not any
> sort of PCRE expert.

With this match I had the impression that the omission of 's' was
actually an oversight/mistake.  We had 30+ matches with the same
construction as this one that had the 's'.

Your absolutely right that the new match is likely to be slow and less
exact (strict overmatch).  I felt that these drawbacks were outweighed
by increasing our consistency in the file by sticking 's' on the end
and making it look like the rest of the matches.

> I've not checked how many other cases there are
> like this where the \r\n elimination changes are done on a signature
> without /s.

This was the only one.

Overall you're bringing up the same concerns I had with making these
changes.  At first it seemed like using \r\n\r\n to enforce matching
content in the body or \r\n to make sure there are intermediate headers
was useful and contributed to a more exact match.  This is why I
originally suggested the "(?>.*?\r\n).*" construction.  It keeps the
extra header/body content enforcement at the cost of additional
complexity understanding the match lines.

Based on Doug's comments and additional thought, I don't think we
really lose anything useful by eliminating these constructs.  The
chance of false positives do to the change is so marginal I don't
think it can be taken seriously.  We do gain a reduced chance of hitting
MATCHLIMIT though.

Hopefully this helps better explain my methodology and thinking behind
these changes.  So far I'm happy to report that I've scanned 50,000
machines with these new changes and haven't hit a MATCHLIMIT yet.

Brandon

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.10 (GNU/Linux)

iEYEARECAAYFAkneTEcACgkQqaGPzAsl94KEBgCdF58M4+3Yv0kHGkXxmkALWU/5
vNIAn2d+BFR1PatCYi3cWZI4hIlpls9X
=ZjVo
-----END PGP SIGNATURE-----

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org