JWT Fuzzer

[preetkaran20]

This is the part 2 of the JWT Addon where Fuzzer Capabilities are added for Fuzzing JWT token.

[preetkaran20]

Added PR to reduce JWTFuzzerHandler code that is copied from HttpFuzzer. https://github.com/zaproxy/zap-extensions/pull/2421

[preetkaran20]

Hi @kingthorin and @thc202 ,

Following is the view of fuzzer: I was in impression that in case of multiple message location each one message location replacer only modifies one http message at a time i.e. say if 2 message locations with one one value is there then 2 http messages will be generated and hence I introduced a field for specifying signature operation which is no signature, generate new signature and use old signature only. But as this is not the case hence this UI becomes invalid so i need to have this field applicable for all the message locations and hence it cannot be represent in message location so i was thinking of adding this field either in Options panel or in fuzzer policy. please suggest.

thanks, Karan

[preetkaran20]

Hi @kingthorin and @thc202 ,

As i have already mentioned that Http view now contains the JWT view and it looks like this:

I am not sure if it is ok or we want to remove it and how can we remove it.

thanks, Karan

[preetkaran20]

Hi @thc202 and @kingthorin ,

Gentle reminder regarding queries.

thanks, Karan

[thc202]

https://github.com/SasanLabs/owasp-zap-jwt-addon/pull/6#issuecomment-640070200

Right, the locations pertain to the same message and they are replaced at the same time (that's to allow to fuzz several "fields" at the same time, e.g. common one, username/password combo). IMO the option can be in the view (maybe show before the field listing?), that info can be passed to the replacer through the message location (e.g. the replacer can check the first message location to determine what/how the signature will be). That's more flexible/usable than move to other place.

https://github.com/SasanLabs/owasp-zap-jwt-addon/pull/6#issuecomment-640070531

Per previous comment, I'm checking that, will try raise the PR in the following days (to allow to add the components/views just to the fuzzer).

[preetkaran20]

#6 (comment)

Right, the locations pertain to the same message and they are replaced at the same time (that's to allow to fuzz several "fields" at the same time, e.g. common one, username/password combo). IMO the option can be in the view (maybe show before the field listing?), that info can be passed to the replacer through the message location (e.g. the replacer can check the first message location to determine what/how the signature will be). That's more flexible/usable than move to other place.

#6 (comment)

Per previous comment, I'm checking that, will try raise the PR in the following days (to allow to add the components/views just to the fuzzer).

Hi @thc202 this makes sense, i think we can add the signature field to all message locations and not typical to first message location. However i think this might be some kind of workaround and i am not sure how can we solve this problem generically. we can build something like Common Props which can go with the message locations and implementer can rely on them that common props for these kind of properties which are applicable to all the message locations.

[kingthorin]

Was there a reason to change all the file permissions?

[preetkaran20]

Was there a reason to change all the file permissions?

Actually i was facing issues locally and hence updated the permissions but somehow git flag for tracking files is enabled so disabled that flag and again checkedin.

[preetkaran20]

Hi @thc202 and @kingthorin ,

I have done the changes. Please find below screenshot of the new fuzzer UI:

Also below is the Options panel UI changes:

Please review.

thanks, Karan

[preetkaran20]

Hi @thc202 and @kingthorin ,

Please review this PR and if you think it doesn't require more changes to Fuzzer Addon, please release that addon to maven repository.

thanks, Karan

[preetkaran20]

Hi @kingthorin and @thc202, I have done the changes as suggested. please review.

thanks, Karan

[preetkaran20]

Hi @thc202 and @kingthorin ,

I have done the changes. Please find below screenshot of the new fuzzer UI:

Also below is the Options panel UI changes:

Please review.

thanks, Karan

Hi @kingthorin and @thc202 ,

I was struggling with what should be the Private Key encoding ? I have for now added PEM encoding for Private key because PKCS encoding has password and for each private key in p12 file has its own password hence i have gone with PEM file with assumption that private key will not have password. However truststore configuration currently has password so i was thinking of changing that too to PEM format. What do you guys think ?

Which format we should stick with ? Shall we go by simple PEM format ? please suggest

thanks, Karan

[kingthorin]

There's nothing written in stone. I'd have to research to see what's more used/supported in the industry.

Security wise, requiring a password is obviously a good choice. However, that's not always the way things go.

If you have something working I'd say stick with it. You can always adapt to community input later as well. If there's request(s) for other formats/support then change/add later.

[preetkaran20]

Hi @thc202 ,

Did we released Fuzzer to Maven Repo ? also please add the review comments you were mentioning so that i can incorporate them.

thanks, Karan

[preetkaran20]

Hi @thc202 ,

Did we released Fuzzer to Maven Repo ? also please add the review comments you were mentioning so that i can incorporate them.

thanks, Karan

@thc202 , Gentle reminder regrading queries.

thanks, Karan

[preetkaran20]

Hi @thc202 @kingthorin ,

Gentle reminder regarding the queries.

thanks, Karan

[thc202]

No news since the last meeting, the add-on was not yet released.

[preetkaran20]

No news since the last meeting, the add-on was not yet released.

Hi @thc202 / @kingthorin ,

Sorry to interrupt you guys again, I was thinking on releasing this addon asap. What do you think ? shall we can release it ? also @thc202 as you had mentioned about review comments please add those and while i will incorporate them you guys can help in releasing the Fuzz addon.

thanks, Karan

[kingthorin]

As far as I understand @thc202 is trying to find a good opportunity to tackle allowing add-ons to declare or push views for the fuzzer.

The first bit with just the scanners (not the fuzzer) could be released?

[thc202]

I'll get the fuzz changes ready for the next weekly and will add the review comments this weekend.

[preetkaran20]

@thc202 @kingthorin ,

Thanks a lot guys, i think lets release the addon then fully else we need to release it in 2-3 weeks again.

[preetkaran20]

Hi @thc202 ,

Gentle reminder.

thanks, Karan

[preetkaran20]

Hi @thc202 ,

Gentle reminder.

thanks, Karan

Hi @thc202 @kingthorin

thanks a lot. Build is now successful.

thanks, Karan

[preetkaran20]

Hi @thc202 , Gentle reminder. thanks, Karan

Hi @thc202 @kingthorin

thanks a lot. Build is now successful.

thanks, Karan

How ever waiting for the review comments :)

[preetkaran20]

Hi @thc202 , Gentle reminder. thanks, Karan

Hi @thc202 @kingthorin thanks a lot. Build is now successful. thanks, Karan

How ever waiting for the review comments :)

@thc202 Gentle reminder.

[preetkaran20]

Hi @thc202 , Gentle reminder. thanks, Karan

Hi @thc202 @kingthorin thanks a lot. Build is now successful. thanks, Karan

How ever waiting for the review comments :)

@thc202 Gentle reminder.

Hi @kingthorin @thc202 ,

Gentle reminder regarding review.

thanks, Karan

[preetkaran20]

Seems fine to me. Still seems to be plenty if comments to yourself that are un-resolved though. Not sure how important those are/were.

i think they are outdated. However i will look at them once again for confirmation.

[preetkaran20]

Seems fine to me. Still seems to be plenty if comments to yourself that are un-resolved though. Not sure how important those are/were.

Yes i checked and they are outdated. One was regarding testing RSA, I have checked again and it is working fine. Just waiting for @thc202 's review comments, so shall i merge these changes and track them as bugs (if it takes time) or shall i wait for some more days ? @kingthorin what is your suggestion ?

[thc202]

Some comments to build.gradle.kts:

-addOnName.set("JWT Extension")
+addOnName.set("JWT Support") // Or something like that, add-ons are more than just extensions (e.g. this provides scan rules as well).

-implementation("org.zaproxy.addon:fuzz:13.0.0")
+compileOnly("org.zaproxy.addon:fuzz:13.0.0") // Might need testImplementation if you plan to add tests.

The dependencies block needs to have:

register("fuzz") {
    version.set("13.*")
}

[thc202]

The changelog could be updated to mention the new functionality.

[thc202]

Another comment for build.gradle.kts, under manifest it could have repo.set("https://github.com/SasanLabs/owasp-zap-jwt-addon/"), that's shown in the marketplace (site and ZAP).

[preetkaran20]

Some comments to build.gradle.kts:

-addOnName.set("JWT Extension")
+addOnName.set("JWT Support") // Or something like that, add-ons are more than just extensions (e.g. this provides scan rules as well).

-implementation("org.zaproxy.addon:fuzz:13.0.0")
+compileOnly("org.zaproxy.addon:fuzz:13.0.0") // Might need testImplementation if you plan to add tests.

The dependencies block needs to have:

register("fuzz") {
    version.set("13.*")
}

what does register doing here ? i didn't got the use ? can you please explain.

[thc202]

Sorry, that was not clear, that's for the dependencies under the manifest, to declare that the JWT add-on depends on the Fuzzer add-on (and that range version). Like was done for the Common Library.

[thc202]

The add-on is not filling the "changes" entry of the manifest, since the add-on has a changelog you can add:

changesFile.set(tasks.named<ConvertMarkdownToHtml>("generateManifestChanges").flatMap { it.html })

to the manifest in build.gradle.kts. That's shown to the user to inform about the changes of the version installed or available in the marketplace.

[preetkaran20]

@thc202 while installing and working on new fuzz version i am facing null pointers while saving options panel:

[kingthorin]

The work around is referenced here: https://github.com/zaproxy/zaproxy/issues/6136

[preetkaran20]

Hi @thc202 ,

I have incorporated changes suggested. Please review.

thanks, Karan

[preetkaran20]

@thc202 done with the changes.

[preetkaran20]

Hi @thc202 @kingthorin ,

please review https://github.com/zaproxy/zaproxy-website/pull/101 also.

thanks, Karan

[thc202]

Looks good, thank you (especially for the patience)!

[preetkaran20]

Merging it now. Thanks @thc202 and @kingthorin

[preetkaran20]

@thc202 @kingthorin what next needs to be done for releasing this Project.

[thc202]

I think the following steps:

1. Update the changelog to reflect the new version, e.g.:

   -## Unreleased
   +## [1.0.0] - 2020-08-03
   
   - First version of JWT Support.
   - Contains scanning rules for basic JWT related vulnerabilities.
   - Contains JWT Fuzzer for fuzzing the JWT's present in the request.
   +
   +[1.0.0]: https://github.com/SasanLabs/owasp-zap-jwt-addon/releases/tag/v1.0.0

2. Tag (above example using v1.0.0) and push.
3. Build the add-on (with Java 8) and create a new release from the tag.

Once that's done we can release to the marketplace.

[preetkaran20]

Hi @thc202 @kingthorin

i have added the release https://github.com/SasanLabs/owasp-zap-jwt-addon/releases/tag/v1.0.0

thanks, Karan

[kingthorin]

It doesn't seem to have the actual add-on (*.zap) attached?

[thc202]

Sorry for the typo in the date.

[preetkaran20]

Added the ".zap" file too.

[thc202]

The jar can be removed, that will not work in ZAP.