This post follows on from Simple Scanner Released
After porting the Perl code over to Ruby in a line by line fashion from the book I noticed that the code was although functional, it was also ugly. Before progressing on to the Extended Scanner I thought it a good idea to try and clean up what was written using some fundamental coding guidelines. Well, I guess they’re guidelines (and fundamental) as they are intuitively off the top of my head…
1. Remove all procedural code into objects
Problem: The main piece of code ’simplescanner.rb’, is one long procedural statement separated by a few functions: testSQL, testXSS, testDir, etc. One of the challenges I noticed was that once within a functions scope you could not access variables within the file scope. As such, the functions have a large number of parameters [e.g., xssTest(file_name, request, status, response, proxy, proxy_port, cookie) ] in order to pass on the values submitted by the end-user. Also, I had to order my functions from top to bottom, because if I called a function that was defined below, to Ruby it was out of scope and would cause an error.
The real cause of this problem is that I decided to use Ruby’s GetOptLong class in order to accept input from the end-user via the command line. This inturn meant I had to use a procedural layout and ran into the problems listed above.
Solution: In order to remove the reliance on GetOptLong, I instead focused on Ruby’s OptParse class. As the API states, “OptionParser is a class for command-line option analysis. It is much more advanced, yet also easier to use, than GetoptLong, and is a more Ruby-oriented solution“. By doing this I was able to capture the command line arguments into one instance.
I then created a ExtendedScanner class so as to capture all the functions listed above (testSQL, testXSS, testDir) and placed this variable as an instance of the class and hence within the classes global scope. By default this reduced all the necessity to pass the parameters, so the function calls looked slightly more elegant [e.g., xssTest(request, status, response) ]. Also, I didn’t have the problem of where I defined my functions as they were also now in class scope.
2. Why create an instance of a class when you can just call a static method of the class
Problem: I was creating an instance of BurpParser in order to call one function…parse(). This meant that I was using unnecessary memory and additional lines of code for no additional benefit. The code could look like the folowing if I wanted to save memory:
bp = Parser::BurpParser.new(ARGV[0])
request_array = bp.requests
bp = nil
Solution: Recode the BurpParser so that it has a static method (self key word) with no defined initialize() method…
class BurpParser
def self.parse(file_name)
...
end
Now I can shorten the above 3 lines of code to 1…
request_array = Parser::BurpParser.parse(ARGV[0])
3. Repetative functions can be moved into other classes with a parent to join them
Problem: The sqlTest, XSSTest, and DirTest functions all appear to take the same arguments as well as return a similar result (i.e., vulnerable to exploit, not vulnerable to exploit). If I had many developers working on this code then everytime there was a new exploit they would have to update the ExtendedScanner class. Not only causing sharing conflicts (ok, cvs can help, but also causing an update in the code for the unit and regression test.
Solution: Move the functions out to their own classes and group all common features into a parent class that these child classes inherit from.
Note: Haven’t done this step yet but will complete this at a later date once the scanner becomes more mature. But in 3 short steps I have cleaned up the code to look almost ready for the OSS community to at least not laugh at.
