Release quality passive scan rule tweeks

[psiinon]

I've been focusing an automation and in particular passive scanning recently.
I've come up with a set of relatively minor changes to the release quality passive scan rules which I think help.
In most cases they just result in fewer issues being raised at MEDIUM threshold with the current behaviour being maintained at LOW threshold.
I think this makes the default behaviour more useful, but would appreciate alternative points of view.
The changes I'm proposing are:

CookieHttpOnlyScanner
https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/CookieHttpOnlyScanner.java
Specify just the cookie name and not its value when reporting issues. This will prevent alerts being raised for each value.

CookieSecureFlagScanner
https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/CookieSecureFlagScanner.java
Specify just the cookie name and not its value when reporting issues. This will prevent alerts being raised for each value.

CrossDomainScriptInclusionScanner
https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/CrossDomainScriptInclusionScanner.java
Change to ignore scripts if an ‘integrity’ value is present. No checks will be made on the validity of the integrity value as that could require an extra request (which pscan rules should not make) and in any case the browser should validate this.
Also change to treat cross domain requests to other sites in the same Context as ok at MEDIUM threshold as well as HIGH (which is the current check)

HeaderXssProtectionScanner
https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/HeaderXssProtectionScanner.java
Change to only apply to HTML files by default. Still apply to all text responses (as now) when using the LOW threshold.

XFrameOptionScanner
https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/XFrameOptionScanner.java
Change to ignore error responses plus redirects and only apply to HTML files by default. Include all text responses including error responses plus redirects (as now) when using the LOW threshold.

My plan was to submit these all as one PR, but as they affect so many release quality rules I thought I'd ask here as I suspect most people here dont keep an eye on the PRs ;)

All feedback appreciated,

Simon

[kingthorin+owaspzap]

My only concern is the XFO one:

• For content type I suggest not limiting to HTML but excluding css/js. There are framing attacks which occur for swf, pdf (reports), html, images (maybe less so), etc.
• I agree with ignoring redirect responses, hopefully the target of the redirect is protected.
• As for error responses I think we might need to implement a Threshold check. It's handy for an attacker to be able to frame error pages/messages in some circumstances.

[psiinon]

At the moment this rule only considers text responses: https://github.com/zaproxy/zap-extensions/blob/master/src/org/zaproxy/zap/extension/pscanrules/XFrameOptionScanner.java#L63

So presumably this means it wont currently detect framing attacks on swf, pdf or images.
At the moment error responses are checked when using a HIGH threshold, and I'm not proposing we change that.
Maybe we should also consider _all_ responses (not just text ones) at HIGH?

Cheers,

Simon

[kingthorin+owaspzap]

Hmm, looks like the isText check was always there, I should have known that.... I was the one that implemented the threshold handling. If we're going to include more than text responses we should do it at low, or by default? (Remember the threshold scale is backwards...)

https://github.com/zaproxy/zap-core-help/wiki/HelpUiDialogsOptionsPscanrules#threshold

[psiinon]

I've created a PR for these changes https://github.com/zaproxy/zap-extensions/pull/473
And it includes unit tests!

I havnt changed the XFO one to handle more than text messages - happy to do that but I think it should be only at low.
Also v happy to make other changes - now is a good time to get these right...

[kingthorin+owaspzap]

Low sounds good.

[psiinon]

I've pushed a new set of changes in https://github.com/zaproxy/zap-extensions/pull/473 to include fixes for https://github.com/zaproxy/zaproxy/issues/2732
I think they are mostly uncontroversial with the exception of the CacheControlScanner changes.
In there at LOW threshold we check all responses apart from images and CSS files, as before.
However at MEDIUM and HIGH threshold we now ignore error and redirect responses. We also only consider non JavaScript text responses, so this includes HTML, XML, JSON and plain TEXT.
That sound reasonable?

Cheers,

Simon