Stephen,
On Thu, May 3, 2012 at 12:10 PM, Stephen Breen <breen.mach...@gmail.com> wrote:
> Thanks for the code review, really appreciate it. I've attached the updated
> file and made some inline comments:
>
>> * Line 2 incorrectly says "crossDomainXMLChecker.py"
>
>
> Oops, that one escaped my notice after copy/paste.
>
>>
>> * Remove "_testCheck"
>>
>
> Done. Another brain fart.
>
>>
>> * Might be a good idea to move "self._themeList = {" to a different
>> file if we plan to make it grow.
>>
>
> Good idea, I'm just not sure where to put it in the framework? Under
> core.data.constants.wordpressThemes.py maybe?
Usually we keep things like that in
"plugins/discovery/<plugin_name>/foo.bar" , in your case it would be
something like "plugins/discovery/wordpress_theme_auditor/" (please
note the change in the plugin name I'm also suggesting).
>>
>> * I would only set "self._exec" to false after identifying a valid
>> wordpress base. It might be the case that in the first run of the
>> plugin the base is not found but then because of enabling webSpider +
>> wordpress_themeauditor in the second call to the wordpress plugin
>> (with a different URL) the blog is found.
>>
>
> Thanks, that makes sense, changed.
Great,
>>
>> * What if the site is https? response.getURL().url_string[7:] , there
>> should be a method in urlParser.py that returns what you need without
>> depending on [...]
>
>
> You're right, there is. I remember looking through it briefly when I wrote
> this and must have missed it. Replaced with response.getURL().allButScheme()
Good,
>>
>> * Check if this is done somewhere else in the other discovery plugins
>> we have and either improve the existing one or replace it with yours
>> if it is far better :)
>>
>
> I don't think any other plugins check wordpress themes. I looked through the
> existing wordpress plugins and did a few greps of the plugins directory and
> came up with nothing. Hopefully someone can correct me if I'm missing
> something here.
I wasn't clear enough, sorry. I was referring to the process of
finding the wordpress root. We don't want to try to find the wordpress
root in all wordpress plugins, we just want to do it in plugin "A",
have that data stored in the KB, and then the rest of the plugins that
depend on "A" will simply read the data from the kb.
>>
>> * Maybe this regex is too open and could generate false positives?
>> regex_str = 'Log\s*In\s*<\s*/title\s*>'
>>
>
> Agreed, it is very open. I thought it would be ok since it will only check
> that regex if the page is called wp-login.php.
Ahh! All right then, I missed that part.
> Even still I suppose you
> could end up with some sort of 401 page where the URL would not be modified
> and this regex would match. I've updated to to be a little more selective to
> wordpress login pages, it now looks for the string 'wp-admin'.
>
>> * This doesn't look good,
>> response.getURL().getDomainPath().url_string[:-1].rpartition('/')[2] ,
>> all the hardcoded [:-1] and [2] tend to crash with special cases
>>
>
> Fixed with a more elegant solution, the theme name is no longer pulled from
> the url, just passed in to the function
>
>>
>> * In "def _checkStyleCss(self,response):" and in order to make it
>> stronger and don't depend only on the response not being a 404, what
>> about looking for a very common string like "size" or "font" or
>> something like that?
>>
>
> I've got a regex to check if it's actually returning CSS. Didn't want to go
> with a common string just because you never know how someone might write
> their CSS. It is (?:\s*\S+\s*{[^}]*})+. It seems to work well.
Not sure about the regex, didn't had time to review it, but I'll
believe you if you say it works :) Also, if possible please compile
regular expressions only once and then use them. An example would look
like this:
class pluginx(...):
CSS_RE = re.compile(...)
def search_css(...):
...
self.CSS_RE.search( some_text )
>>
>> * getLongDesc() returns info from crossdomain.xml
>>
>
> Fixed.
>
>>
>> Except from those small details, it looks very good! What do you
>> think, should it be merged into our pre-existing wordpress plugins or
>> should it live as an independent one? If so, can we relate your plugin
>> with the others using "getPluginDeps()" ?
>>
>
> I like the idea of merging it with the wordpress_fingerprint plugin. I don't
> think it makes sense to have it separate since it really is just doing some
> more wordpress fingerprinting and having a lot of plugins that all do small
> wordpress specific tasks seems like a waste.
Yep, but yours also checks for a vuln... we want to keep that. I would
recommend merging the identification of the theme into
wordpress_fingerprint and then having the detection of the
vulnerability in a second plugin that depends on
wordpress_fingerprint.
Regards,
>>
>> >
>> > ------------------------------------------------------------------------------
>> > Live Security Virtual Conference
>> > Exclusive live event will cover all the ways today's security and
>> > threat landscape has changed and how IT managers can respond.
>> > Discussions
>> > will include endpoint security, mobile security and the latest in
>> > malware
>> > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>> > _______________________________________________
>> > W3af-develop mailing list
>> > W3af-develop@lists.sourceforge.net
>> > https://lists.sourceforge.net/lists/listinfo/w3af-develop
>> >
>>
>>
>>
>> --
>> Andrés Riancho
>> Project Leader at w3af - http://w3af.org/
>> Web Application Attack and Audit Framework
>> Twitter: @w3af
>> GPG: 0x93C344F3
>
>
--
Andrés Riancho
Project Leader at w3af - http://w3af.org/
Web Application Attack and Audit Framework
Twitter: @w3af
GPG: 0x93C344F3
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
W3af-develop mailing list
W3af-develop@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/w3af-develop
