Add option to inject plugin ID in header for all ascan requests

[GoogleCodeExporter]

Proposed in the User Group: 
https://groups.google.com/d/msg/zaproxy-users/7lJXZ4hqmFk/Bi8GJ1tfSMgJ

This should be an option, default off.
I think its pretty easy to implement:
In AbstractPlugin.getNewMsg() check for the option, and if turned on then add a 
header like: X-ZAP-SCAN-ID with a value of getId()
All ascan rules should use this method when creating new messages to send - if 
they dont then they need to change :)

Original issue reported on code.google.com by psii...@gmail.com on 20 Mar 2015 at 12:39

[GoogleCodeExporter]

Hi Simon,

I'm taking a crack at this now :)

Original comment by pdpma...@gmail.com on 25 Mar 2015 at 2:03

[GoogleCodeExporter]

Cool :)

Original comment by psii...@gmail.com on 25 Mar 2015 at 2:04

[GoogleCodeExporter]

What about using msg.setNote() instead? It won't be sent to the web server, and 
should not need any configuration options as a consequence. The use of headers 
will just help WAFs to fingerprint ZAP, resulting in real vulnerabilities being 
masked, and making zap less effective

It should also be possible to make this change without changes to any of the 
scanners, by using the getId() method.

Original comment by colm.p.o...@gmail.com on 25 Mar 2015 at 2:24

[GoogleCodeExporter]

I'm not so keen on that.
The notes are really meant for user notes and so having loads of generated ones 
might be confusing and make it much harder for users to find the manual ones.
Its also extra db calls - not an issue right now, but it may be in the future.
I think some people will find it more useful actually having the info in the 
headers so that they can correlate the requests with logs (maybe;)
I suppose we could have options to do either and see which works better in 
practice?
Note that using a header as suggested shouldnt require any changes to the 
scanners either ;)

Original comment by psii...@gmail.com on 25 Mar 2015 at 2:29

[GoogleCodeExporter]

Headers are not typically logged by web/app servers, so setting them is 
typically not going to be of much benefit. I take your point on dB calls, and 
performance though.

If the header option is off by default, and changes are not required to the 
scanners, I don't really see much issue with it, aside from the fingerprinting 
aspect -giving WAFS the ability to isolate and analyse zap traffic. I care 
about that because I see it as a risk of neutralising the usefulness of some of 
the scanners I've worked on. 

Original comment by colm.p.o...@gmail.com on 25 Mar 2015 at 2:50

[GoogleCodeExporter]

Quick question regarding Messages.properties -- I've edited the default file 
with the label for the new option using RBE, but do I need to worry about 
internationalization?

Original comment by pdpma...@gmail.com on 25 Mar 2015 at 6:59

[GoogleCodeExporter]

Nope. Not unless you want to provide translations yourself. Its up to you.

Original comment by colm.p.o...@gmail.com on 25 Mar 2015 at 8:00

[GoogleCodeExporter]

Sounds good, that's unfortunately outside of my skill set :). I've tested this 
option and it looks like I've got it working. I don't have commit access for 
zaproxy, should I attach the files I've changed here for someone to look over?

Original comment by pdpma...@gmail.com on 26 Mar 2015 at 4:37

[GoogleCodeExporter]

I wouldn't mind looking at a patch in the meantime ;)

Original comment by THC...@gmail.com on 27 Mar 2015 at 3:03

[GoogleCodeExporter]

Cool! Attached are some screenshots of the option as it appears in the menu, as 
well as a request from the path traversal scan when the option is turned on, 
along with the java files I changed (not sure if you actually wanted to see the 
code but figured it wouldn't hurt to send along). Thanks!

Original comment by pdpma...@gmail.com on 27 Mar 2015 at 4:07

Attachments:

• issue1573.zip

[GoogleCodeExporter]

Yes, I wanted to take a look at the code changes :) (though I preferred a patch 
as it's a lot easier to see the changes, but that's ok too ;)

Following some comments about the changes:
 - Generic comments:
   - Code comments like "// issue 1573" and "ZAP" comments, in the middle of the code, are not necessary;
   - Avoid adding empty JavaDoc comments or, as the structure is already there, why not document the methods added? ;)
 - AbstractPlugin
   - The header is being added to the original request not the cloned one, was that intended?
 - ScannerParam
   - The getter for "injectPluginIdInHeader" should start with "is" instead of "get".

Was the help page updated? (not an initial requirement, just a good thing to 
have)

@all:
Regarding the name of the header, shouldn't be "Scanner" (or "Plugin") instead 
of "Scan"? With "Scan" it seems to me that's referring to the ID of the scan 
not the ID of the scanners/plugins.

Original comment by THC...@gmail.com on 30 Mar 2015 at 1:53

[GoogleCodeExporter]

Thanks for the feedback!

I've fixed some of the getter name, messy commenting (artifacts from trying to 
make navigating between files easier for myself :) ) and filled in the JavaDoc 
comment.

I did add the header to the original request intentionally, because I thought 
that's where it was desired, but if it makes more sense to add it in the cloned 
msg I will move it.

I have not yet updated the help page, but will do so.

Regarding a patch, forgive my ignorance but how would I go about that in the 
future other than committing through subclipse? Should I just build my own copy 
of ZAP and send that?

Cheers,
Paul

Original comment by pdpma...@gmail.com on 31 Mar 2015 at 3:59

[GoogleCodeExporter]

Thank you! ;)

If the header is to be added to the original request then it can be done only 
once, in AbstractPlugin.init(HttpMessage, HostProcess).
I think it should be added to the cloned request, but let's see what others 
have to say.

OK, that's great :)

A patch can be created using the context menu item, "Team" > "Create Patch...", 
after selecting the desired directory/files.
In this case it should be selected the "src" directory (to include the 
Messages.properties file, Java class files, help files...).

BTW, how would you like to be credited?
https://code.google.com/p/zaproxy/wiki/HelpCredits

Original comment by THC...@gmail.com on 2 Apr 2015 at 3:25

[GoogleCodeExporter]

Ok, I've attached a new patch using "Selection" as root. Is this correct?

As for credits, I suppose I'll go with my full name, Paul Pollack :)

Original comment by pdpma...@gmail.com on 2 Apr 2015 at 2:32

Attachments:

• patch.txt

[GoogleCodeExporter]

Yes, "Selection" is fine. That's used to build the file path of the changed 
files starting from the selected directory.

Regarding the actual contents of the patch, there are some "issues":
 - The Messages.properties file has unresolved conflicts, most likely caused by the same issue that have the other .properties files, that all the lines were changed (I guess that was done by RBE :/ ). The changes should be reverted and the new message added manually to avoid committing unnecessary changes;
 - In HttpHeader, there's one ZAP comment in the middle of the file, it should be removed;
 - In AbstractPlugin, the change to the original request should be moved to init(HttpMessage, HostProcess), avoids (re-)setting the header multiple times.

psiinon has already added you to the credits page :)

Original comment by THC...@gmail.com on 7 Apr 2015 at 5:10

[GoogleCodeExporter]

Attached is a new patch, which I believe addresses all issues... but I suppose 
that remains to be seen ;)

Original comment by pdpma...@gmail.com on 7 Apr 2015 at 6:29

Attachments:

• patch.txt

[GoogleCodeExporter]

Looks good to me :)
(Though there are some unnecessary indentation changes that's not a showstopper 
;)

Could you please go ahead and commit the changes (and update status of the 
issue)?

Original comment by THC...@gmail.com on 8 Apr 2015 at 2:51

[GoogleCodeExporter]

All set :)

Original comment by pdpma...@gmail.com on 8 Apr 2015 at 2:00

• Changed state: Committed

[GoogleCodeExporter]

Thanks!

Original comment by THC...@gmail.com on 9 Apr 2015 at 3:54

[GoogleCodeExporter]

Fixed in 2.4.0

Original comment by psii...@gmail.com on 14 Apr 2015 at 11:03

• Changed state: Fixed

[GoogleCodeExporter]

Changed to "Committed" since it was not merged to branch 2.4.

Original comment by THC...@gmail.com on 14 Apr 2015 at 1:57

• Changed state: Committed

[GoogleCodeExporter]

Oops, in general is it my responsibility as the commiter to merge the change 
into the version branch? Or is that generally left to a senior project member?

Original comment by pdpma...@gmail.com on 15 Apr 2015 at 2:57

[GoogleCodeExporter]

We were pretty close to releasing 2.4.0 so I didnt really want any non 
essential changes going in at that stage.
However you can commit it to the 2.4 branch now if you like - I expect we'll 
have a bug fix release on 2.4 before too long, and that can include 
enhancements like this :)

Original comment by psii...@gmail.com on 15 Apr 2015 at 3:01

[GoogleCodeExporter]

Makes perfect sense, just wanted to make sure I didn't neglect a responsibility 
:)

Merged the changes in.

Original comment by pdpma...@gmail.com on 15 Apr 2015 at 3:42

[GoogleCodeExporter]

ZAP has been migrated to github

This issue will be on github issues with the same ID: 
https://github.com/zaproxy/zaproxy/issues

Original comment by psii...@gmail.com on 5 Jun 2015 at 9:18

• Changed state: Done