I've noticed that since WI added the CSRF module, it almost always flags login forms as CSRF vulnerable. The post data for such HTTP requests typically has &password=foo, so am I right in thinking that this is a false positive? While the form may be technically susceptible to CSRF, it requires the password to execute, in which case I don't see how tricking someone into logging in (and potentially then taking other actions) is valid CSRF when you already have the password and could just login as them and go wild.
I just want to make sure my assumption here is correct and that I'm not missing something.
The CSRF engine does a variety of CSRF attacks, only a subsection of which requires actually logging into the site. So though it may still be a false positive, there still is a very good chance that the CSRF engine is finding issues with your site.
What I've found is that a false positive for CSRF forgery on a login form often yields actual CSRF when I manually start prodding around the rest of the website, so it's still useful as a "look over here" flag. I just wanted to make sure that the scenario WI flags up CSRF for the most, login.php?user=foo&pass=bar, isn't anything of note because you'd need the password to execute that attack, rendering it pointless. Once you're authenticated, if further URLs can cause other actions to be processed by the server, that is of course a different issue.
I believe you are correct. I talked to the engineer who created the CSRF engine, and he says that if a site is vulnerable, the engine will flag both the login page and the restricted pages it found the vulnerability on.
I have gone back to the engineer for clarification on this matter, since there seems to be a lot of confusion in this area. Though logically, having CSRF flag on the login page may appear to be a false positive, because the login page is not in a restricted area, there is a specific CSRF case that allows an attacker to trick a user into loging in as the attacker. We are updating the vulnerability write-up in WebInspect to reflect this. Hopefully this will clear up confusion in the future.