[sqlmap-users] Code review :)

[Andres R. <and...@gm...>]

List,

   During PHDays we had a really good idea with Miroslav: "I review
sqlmap's code and send you some comments about it, and Miroslav will
review some w3af code and do the same". So, while I had some spare
minutes at the airport I performed some initial review:

* I like the idea of using psyco, what's your experience with it? Do
you guys recommend it?

* Liked the concept in "def smokeTest():", which sounds interesting to
have also in w3af

* lib/core/testing.py : shouldn't most/all of this be migrated to
unit-tests and run using "nosetests" or some other tool like that?

* As Miroslav mentioned, we're using the same keepalive.py module,
I'll have to run a diff between w3af's and sqlmap's and see what we
changed; since we both made modifications to "make it work".

* Using rangehandler.py is a great idea for speeding up (A LOT) the
extraction of information, it seems that you guys add it to the
urlopener but don't use it?

* Could you please explain me the first part of this if? "if
conf.hostname in ('localhost', '127.0.0.1') or conf.ignoreProxy:" does
it really make sense? Aren't you ignoring the user's wish?

* heh, I also use gprof2dot for profiling, but instead of having it
inside w3af, I simply call it from the command line and have it
generate a PNG. Note, where is "start()" defined for this line?
cProfile.run("start()", profileOutputFile)

* Read this comment:
   """
   # Set kb.partRun in case "common prediction" feature (a.k.a. "good
   # samaritan") is used
   """

Good samaritan was a feature I added many years ago to w3af's sqlmap,
and the name came from the idea that the user could help the blind sql
injection process by completing the word that was being extracted.
Example: "If sqlmap extracted -hello w- the user could type -orld- in
the console and have it checked with a SELECT statement". According to
the pieces of code I was able to find, that was replaced by a more
automatic idea where a file feeds common strings to the process,
correct? The idea sounds good, but maybe users still want to
contribute to the process?

* dataToStdout() is a handy function, but I think that you should
consider migrating to something more generic like python's logging
module. If in the future you want to provide options to storing the
data in a file, or similar, it might come handy. In w3af we have the
outputManag

- From our talks I understood that sqlmap used multiprocessing for
cracking hashes (or something like that) but I can't find any
reference to the multiprocessing module in the latest version. Could
you point me in the right direction so I can analyze that code?

- Not sure how usable it is for you guys, but in some cases the
charset is set in a meta tag; you're ignoring that here:
   if contentType and (contentType.find('charset=') != -1):
       charset = checkCharEncoding(contentType.split('charset=')[-1])

       if charset:
           page = getUnicode(page, charset)

 See w3af's httpResponse.py for an example on how we're doing it.

- Not thread safe?

       if conf.delay is not None and isinstance(conf.delay, (int,
float)) and conf.delay > 0:
           time.sleep(conf.delay)

 Maybe move the "kb.locks.reqLock.acquire()" some lines before?

- Doesn't this kill the keepalive.py handler? Should try to capture packets.

           if not req.has_header("Connection"):
               requestHeaders += "\nConnection: close"

I know that many of these are questions, but I hope they trigger some
good ideas :)

PS: I only used 2h for reading code. 2h left.

Regards,
--
Andrés Riancho
Project Leader at w3af - http://w3af.org/
Web Application Attack and Audit Framework
Twitter: @w3af
GPG: 0x93C344F3