Submitting the Attack Surface Detector plugin to the Marketplace

[matthewD-AVI]

Add-on repo
https://github.com/secdec/attack-surface-detector-zap

Your contact details
matthew.deletto@securedecisions.com

Are you one of the authors
Yes

Licence
in repo

Build instructions
Install Maven: https://maven.apache.org/install.html
Clone Attack Surface Detector repository: https://github.com/secdec/attack-surface-detector-zap
Navigate to the source code Directory, open terminal and run the command mvn clean package
The plugin will be located in the target folder named: attacksurfacedetector-release-#.zap.

also in readme

Link to more information
https://github.com/secdec/attack-surface-detector-zap/wiki

Twitter handle for tool or author(s)
https://twitter.com/secdec

Promote to Beta or Release?
Note that all new add-ons start at Alpha status.

Anything else we should know
During web application penetration testing, it is important
to enumerate your application's attack surface. While Dynamic Application Security Testing (DAST) tools (such as Burp Suite and ZAP) are good at spidering to identify application attack surfaces, they will often fail to identify unlinked endpoints and optional parameters. These endpoints and parameters not found often go untested, which can leave your application open to an attacker. This tool is the Attack Surface Detector, a plugin for OWASP ZAP. This tool figures out the endpoints of a web application, the parameters these endpoints accept, and the data type of those parameters. This includes the unlinked endpoints a spider won't find in client-side code, or optional parameters totally unused in client-side code. The plugin then imports this data into ZAP so you view the results, or work with the detected endpoints and parameters from the target site map

[psiinon]

A few issues from me:

• Its using the ZAP 2.2.2 jar - that will need updating to 2.7.0
• The file should be ...alpha-1.zap not ..release-1.10.zap
• Is the description right?
• I'd recommend changing not-before-version to 2.7.0 - we wont release it for old versions of ZAP in any case
• Logging at info should only be done for significant and infrequent events - 'debug' is much more suitable for frequent events
• Is the url right? Not sure this is still associated with Denim
• Likewise the Denim icon

fyi we tend to use the fugue icons: https://github.com/yusukekamiyamane/fugue-icons - you can see them all here: https://github.com/yusukekamiyamane/fugue-icons/blob/master/all-preview.png :)

[thc202]

Worth noting that ZAP is already available in Maven Central:
http://search.maven.org/#search|gav|1|g%3A%22org.zaproxy%22%20AND%20a%3A%22zap%22

[matthewD-AVI]

@psiinon I will be addressing most if not all of your issues today. Some issues such as the icon may require a more collaborative effort here. I will update the plugin and let you know when these changes have been made.

[matthewD-AVI]

I am trying to address your concerns but while upgrading to ZAP version 2.7.0 I am receiving this exception while installing the plugin
[AWT-EventQueue-0] ERROR AddOnLoaderUtils - Declared "extension" is not of type "org.parosproxy.paros.extension.Extension": org.zaproxy.zap.extension.attacksurfacedetector.AttackSurfaceDetector

I can not seem to find any documentation on how to address this and was hoping you could lead me in the right direction.

[thc202]

Is that the whole error, no stack trace? Is the code pushed (to take a look)?

EDIT: Yeah, no stack trace for that error.

[matthewD-AVI]

There is no stack trace for that error.
The code is not pushed since it is not working and I did not want to upload broken code. The only changes made where to the

        <dependency>
            <groupId>com.owasp.zap</groupId>
            <artifactId>zap</artifactId>
            <version>${zap.version}</version>
            <scope>provided</scope>
        </dependency>

in the pom files which was replaced by

<dependency>
    <groupId>org.zaproxy</groupId>
    <artifactId>zap</artifactId>
    <version>2.7.0</version>
</dependency>

[thc202]

It's missing the scope in the update? And to elaborate, that would cause problems since it would bundle and use a different Extension class.

[matthewD-AVI]

I was no longer providing the Jar file and was pulling directly from maven. I have never needed a scope before when the jar file is not provided.
https://mvnrepository.com/artifact/org.zaproxy/zap/2.7.0
That is what I used

[thc202]

Yes, but you still need to declare it as provided, those classes will be available at runtime.

[matthewD-AVI]

@thc202 thank you for the prompt response. The issue has been rectified and I can now address the rest of the concerns

[matthewD-AVI]

commit: 7d1927acb1aac2bf00b1f3e2b1c46ceb614dc290 should address all of @psiinon concerns.

[thc202]

In the AttackThread it should use the new API instead, something like:

Target spiderTarget = new Target(startNode);
spiderTarget.setRecurse(true);
int id = extSpider.startScan(spiderTarget, null, null);
// Give some time to the spider to finish to setup and start itself.
sleep(1500);
SpiderScan spiderScan = extSpider.getScan(id);
while (!spiderScan.isStopped()) {
    // Should sleep some time here too otherwise the thread will be too much active.
    if (this.stopAttack) {
        spiderScan.stopScan();
        break;
    }
}

this way it avoids conflicts with other running spider scans.

I think most if not all (except ZAP) of the provided dependencies could be removed, they seem to be transitive dependencies of ZAP (which are already being included). (Not that it matters for the release to marketplace, just tidy up.)

[thc202]

Note that the spider will fail to start when the Mode does not allow spider scans. A check before starting the ASD would be better but catching the exception (IllegalStateException) is fine as well.

[thc202]

Another thing worth doing now, is declare that the extension can be unloaded (e.g. override canUnload in AttackSurfaceDetector to return true), it seems to me that it can (from ZAP side it's removing everything from core, worth checking if the dependencies (e.g. com.github.secdec.astam-correlator) can also be GC'ed). It makes life easier to the user as it does not need to restart ZAP after updating the add-on.

[matthewD-AVI]

@thc202 I am currently rewriting the spider to use the new API and sleeps. I am also overridding canUnload, but can you clarify what is meant by "worth checking if the dependencies (e.g. com.github.secdec.astam-correlator) can also be GC'ed)." So I can properly address that concern.

[thc202]

I'm referring to the usual features that might lead to memory leaks (e.g. use of ThreadLocal in a long lived thread, EDT or passive scan thread, would be the main ones to be careful with).

[matthewD-AVI]

@thc202 commit b22493616a73434ebb79b70045abec034f4024b9 should have addressed all of your concerns. Thank you for all of your input.

[thc202]

@matthewD-AVI thanks! Will take a look ASAP.

[thc202]

The AttackThread is still stopping all spider scans, is that on purpose? (The spiderScan.stopScan(); should work and does not interfere with other scans already running.)

Some comments regarding the add-on manifest (ZapAddOn.xml file):

1. The following lines can be removed (not used by ZAP): https://github.com/secdec/attack-surface-detector-zap/blob/365fa2d98d836db1f68b6d9eea5f95434d13fc43/zaproxy/src/org/zaproxy/zap/extension/attacksurfacedetector/ZapAddOn.xml#L5 and https://github.com/secdec/attack-surface-detector-zap/blob/365fa2d98d836db1f68b6d9eea5f95434d13fc43/zaproxy/src/org/zaproxy/zap/extension/attacksurfacedetector/ZapAddOn.xml#L7
2. I'd suggest just use version with the semantic version (if you really want to use that), it's no longer necessary to use both and makes the version handling easier (just one version to maintain).

That's all from me.

[matthewD-AVI]

@thc202 The stopAllScans() just slipped through, it has been changed as well ZappAddOn.xml
commit: 4f050b52038fe0c153f4219e33745455f1f557c5

[kingthorin]

@matthewD-AVI
Looking at https://github.com/secdec/attack-surface-detector-zap/releases 1.1.0 was from your original announcement (IIRC).

So https://github.com/secdec/attack-surface-detector-zap/blob/master/zaproxy/src/org/zaproxy/zap/extension/attacksurfacedetector/ZapAddOn.xml#L3 should probably be 1.2.0 or 1.1.1?

[kingthorin]

@matthewD-AVI could you email me? github username at gmail.com

Thanks

[thc202]

@matthewD-AVI, @kingthorin what's the current state?

[matthewD-AVI]

@thc202 as of commit: e18f9ba41e43d62e30d909e4b135a330c6f1e1ce all of your concerns should have been addressed.

[matthewD-AVI]

I'm just following up to see if there is anything else needed to get ASD into the Marketplace.

[kingthorin]

@matthewD-AVI the only thing I see outstanding is the version info. Looking at https://github.com/secdec/attack-surface-detector-zap/releases 1.1.0 was from your original announcement (IIRC).

So https://github.com/secdec/attack-surface-detector-zap/blob/master/zaproxy/src/org/zaproxy/zap/extension/attacksurfacedetector/ZapAddOn.xml#L3 should probably be 1.2.0 or 1.1.1?

[matthewD-AVI]

@kingthorin I will be doing a version bump soon, for now I will switch it back to 1.1.0. Thank you for the input.

[matthewD-AVI]

@kingthorin @psiinon @thc202 There is a new version available 1.1.1 that has addressed every concern that has been raised on this thread.

[thc202]

I raised an issue regarding the version, secdec/attack-surface-detector-zap#16.

[thc202]

btw, IMHO the class ZapPropertiesManager is still to verbose, I can see a lot of log messages like:

INFO org.zaproxy.zap.extension.attacksurfacedetector.ZapPropertiesManager  - Properties file is at /zap/asd.properties
INFO org.zaproxy.zap.extension.attacksurfacedetector.ZapPropertiesManager  - Successfully loaded properties.

when importing.

[matthewD-AVI]

As of the latest commit(8b1415266fd30310c79eb86fb8aceca86a78b76e), all issues raised by @thc202 have been addressed. I moved the tag to update release v1.1.1 with updated binaries

[kingthorin]

I think this is good to go?

[thc202]

Yeah, looks good to me.

[thc202]

Closing, the add-on is now available in the marketplace. Thank you! :)

[psiinon]

Yeah, thanks @matthewD-AVI :)
Also announced on twitter: https://twitter.com/zaproxy/status/1012337855555874817

[matthewD-AVI]

Thank you all

[lock]

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.