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

[zapbot]

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 psiinon on 2015-03-20 12:39:25

[zapbot]

Hi Simon,

I'm taking a crack at this now :)

Original issue reported on code.google.com by pdpman88 on 2015-03-25 14:03:43

[zapbot]

Cool :)

Original issue reported on code.google.com by psiinon on 2015-03-25 14:04:43

[zapbot]

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 issue reported on code.google.com by colm.p.oflaherty on 2015-03-25 14:24:21

[zapbot]

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 issue reported on code.google.com by psiinon on 2015-03-25 14:29:19

[zapbot]

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 issue reported on code.google.com by colm.p.oflaherty on 2015-03-25 14:50:01

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-03-25 18:59:07

[zapbot]

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

Original issue reported on code.google.com by colm.p.oflaherty on 2015-03-25 20:00:14

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-03-26 16:37:52

[zapbot]

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

Original issue reported on code.google.com by THC202 on 2015-03-27 15:03:21

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-03-27 16:07:25

---

• Attachment: issue1573.zip

[zapbot]

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 issue reported on code.google.com by THC202 on 2015-03-30 13:53:44

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-03-31 15:59:20

[zapbot]

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 issue reported on code.google.com by THC202 on 2015-04-02 03:25:55

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-04-02 14:32:48

---

• Attachment: patch.txt

[zapbot]

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 issue reported on code.google.com by THC202 on 2015-04-07 05:10:31

[zapbot]

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

Original issue reported on code.google.com by pdpman88 on 2015-04-07 18:29:38

---

• Attachment: patch.txt

[zapbot]

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 issue reported on code.google.com by THC202 on 2015-04-08 02:51:04

[zapbot]

All set :)

Original issue reported on code.google.com by pdpman88 on 2015-04-08 14:00:35

[zapbot]

Thanks!

Original issue reported on code.google.com by THC202 on 2015-04-09 15:54:53

[zapbot]

Fixed in 2.4.0

Original issue reported on code.google.com by psiinon on 2015-04-14 11:03:54

[zapbot]

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

Original issue reported on code.google.com by THC202 on 2015-04-14 13:57:38

[zapbot]

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 issue reported on code.google.com by pdpman88 on 2015-04-15 14:57:40

[zapbot]

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 issue reported on code.google.com by psiinon on 2015-04-15 15:01:27

[zapbot]

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

Merged the changes in.

Original issue reported on code.google.com by pdpman88 on 2015-04-15 15:42:21

[thc202]

Fixed in f5e1bfcf713e242b764b40fab2804b1e5921acec

[thc202]

Included in 2.4.1.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.