Add CSP helper add-on

[chutchut]

Pending tasks:

• [ ] Show the current/latest CSP policy of the site;
• [x] Show in the CSP Helper tab, not Output tab;
• [ ] Indicate in Sites tree (with an icon?) that the CSP Helper is enabled for the site;
• [ ] Use ExtensionCallback (instead of API).

[thc202]

Added some quick/obvious comments, will pull the changes and check the code more thoroughly.

[psiinon]

@chutchut are you going to be able to look at these comments?

[chutchut]

@thc202 @psiinon

Apologies for the delay! Got most of these sorted now. Going to have a look at how to implement the PassiveScanner

[kingthorin]

Please do an interactive rebase and fix-up or squash into a single "atomic" commit: https://github.com/zaproxy/zaproxy/blob/develop/CONTRIBUTING.md#what-we-zap-team-expect-from-you .

• https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html#_interactive_mode
• https://wiki.eclipse.org/EGit/User_Guide#Interactive_Rebase

[chutchut]

Will get this done ASAP, probably over the weekend.

 

Cheers

 

Sent: Tuesday, February 14, 2017 at 2:23 PM From: kingthorin [email protected] To: zaproxy/zap-extensions [email protected] Cc: chutchut [email protected], Mention [email protected] Subject: Re: [zaproxy/zap-extensions] Committing CSP helper to alpha (#668)

Please do an interactive rebase and fix-up or squash into a single "atomic" commit: https://github.com/zaproxy/zaproxy/blob/develop/CONTRIBUTING.md#what-we-zap-team-expect-from-you .

https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html#_interactive_mode
https://wiki.eclipse.org/EGit/User_Guide#Interactive_Rebase

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

 

[kingthorin]

FYI if this is rebased on the latest Alpha branch the Salvation library is now included.

[thc202]

The library does not seem to be in use (yet), it could be dropped from this pull request.

[kingthorin]

Is someone still working on this? @chutchut?

[thc202]

@chutchut ping :)

[thc202]

@chutchut, @psiinon, @kingthorin any objections to move this to master branch? It would be less work when merging all the branches...

[kingthorin]

Sure, if we are getting close to doing that for the gradle stuff anyway got for it. Whatever makes things easier IMHO.

[psiinon]

+1

[kingthorin]

I just put together a bunch of fixes for this, but for some reason it wouldn't let me push from a local feature branch to chutchut's alpha :(

[thc202]

Which error are you getting?

[kingthorin]

I'm gonna try fetching to alpha instead of a feature beanch then pushing some changes,if that fails I'll get u the text of the error.

[kingthorin]

Issue resolved. Needed to specify the repo URL as https

(Devil in the detail....)

[thc202]

Cool, do you have any other changes pending? Otherwise could move this into master in the following days.

[kingthorin]

Nope, that's it for now. I only tackled those few changes because I had some time available, it wasn't really planned.

[thc202]

Moved to master branch.

[thc202]

Pushed some tweaks, they are split into smaller commits for easier review (once OKed we can squash or drop them).

[thc202]

The UI still needs to be improved(?):

• Show the current/latest CSP policy of the site (currently it's just shown when the CSP helper is enabled/disabled, which makes it hard to know what's doing);
• Show in the CSP Helper tab, not Output tab;
• Indicate in Sites tree (with an icon?) that the CSP Helper is enabled for the site.

[kingthorin]

I'm not sure how this is supposed to work.

What's supposed to appear in the CSP Helper tab?

I can see that it injects a report-uri, but I'm unclear how it's supposed to work. report-uri http://example.org/zapCallBackUrl/1351365933686127951; was inserted into the CSP of one of the sites I visited. If I change example.org to localhost:8080 I still get "Bad Format" as the result. If I assemble it based on the current Callback Address details (something like: http://localhost:62036/zapCallBackUrl/1351365933686127951) it fails*. Granted that's a corporate machine so maybe there's a local FW that's interfering but..... I still don't see how the original form can ever work. Shouldn't it at least be assembled based on the current callback settings?

*

846252 [ZAP-ProxyThread-150] ERROR org.zaproxy.zap.extension.callback.ExtensionCallback - No callback handler for URL : http://localhost:62036/zapCallBackUrl/1351365933686127951 from /127.0.0.1

Ignoring the callback part. Is it suggesting CSP? Is it in addition to any CSP that exists already? (i.e.: Does it build on top of existing policies?) Per page? Per host?

[thc202]

What's supposed to appear in the CSP Helper tab?

Nothing, that's one of the pending changes/enhancements. For now the latest CSP is shown in the output tab, when you disable the CSP helper for the site.

Shouldn't it at least be assembled based on the current callback settings?

It is, that's how API URL callbacks are formed, they just need to have the /zapCallBackUrl/ (note that with the current callback implementation used by the add-on it needs to have access to the API). Didn't you get warnings about that?

[kingthorin]

Nothing, that's one of the pending changes/enhancements. For now the latest CSP is shown in the output tab, when you disable the CSP helper for the site.

Ok I thought maybe the callbacks were supposed to be shown there.

It is, that's how API URL callbacks are formed, they just need to have the /zapCallBackUrl/ (note that with the current callback implementation used by the add-on it needs to have access to the API). Didn't you get warnings about that?

By dialog or log? Definitely not by dialog, I didn't go through the logs in much detail, but nothing stuck out. I did add .* to the API access list while I was futzing around with it, didn't make a difference.

Edit: Maybe a ZAP restart was needed. I see callbacks via the site tree now, that weren't initiated by me....

[thc202]

In the log, but if you allowed .* then it's normal that it didn't show anything (and should be working as expected).

That's odd, it should not show up there, they should be processed internally :/ Guess the browser was still using an older report-uri?

[thc202]

Another thing to change, use ExtensionCallback.

[thc202]

Updated PR description with some tasks (feel free to add/tweak them).

[kingthorin]

GAH! Sorry it looks like I amended instead of adding separate. (If it's undoable let me know. I'm not going to fiddle because I don't want to make it worse.)

Edit: Is the build failure due to the target (master) having different lib directory content than the branch itself (core 2.7 vs 2.6?).

Edit2: This was discussed in IRC. The build failure is due to branch lib differences. We decided there's no need to unwind the amend vs separate commit as this will all ultimately get squashed anyway.

[thc202]

The pull request is now updated (after zap-extensions build changes, zaproxy/zaproxy#5302), also, changed to target ZAP 2.7.0 (it was already using it) and normalised the license and style/format of the code.

[thc202]

Rebased and fixed conflict in settings.gradle.kts file.