[WIP] Totp Active Scan Rules #6255

AliceMilshtein · 2025-03-07T14:18:10Z

Overview

Briefly describe the purpose, goals, and changes or improvements made in this pull request.

Related Issues

Specify any related issues or pull requests by linking to them.

Checklist

For more details, please refer to the developer rules and guidelines.

…nerability

…rule to check if random passcode meeting format will give access

github-actions · 2025-03-07T14:18:23Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

psiinon · 2025-03-07T15:28:44Z

Checkmarx One – Scan Summary & Details – 872bcac0-8b30-474d-b823-2d6ff9d08ebb

Great job, no security vulnerabilities found in this Pull Request

…ntication with blank code vulnerability

… work with new version changes

… 3 incorrect codes entered

psiinon · 2025-03-28T10:54:21Z

addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/internal/ClientSideHandler.java

    private AuthRequestDetails authReq;
    private HttpMessage fallbackMsg;
    private int firstHrefId;
+    private List<HttpMessage> httpMessages = new ArrayList<>();


This worries me :)
HttpMessage's can be big objects, so we try not to hold on to them for too long.
In other places we've saved the History IDs (which are just ints) and then you can read the messages from the DB when they are needed.

psiinon

Just some initial comments, will give a fuller review in a bit :)

psiinon · 2025-03-28T10:56:52Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

@@ -0,0 +1,167 @@
+package org.zaproxy.zap.extension.ascanrulesAlpha;


All code should have the standard header - you can just rip it off from any other class, just remember to change the year to 2025 😁

The spotlessApply task will add them with the proper year.

psiinon · 2025-03-28T10:58:26Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

+    }
+    @Override
+    public String getName() {
+        return "Blank code TOTP Scan Rule";


All strings should be i18ned, e.g. as per https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrulesAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/ExampleFileActiveScanRule.java#L67

psiinon · 2025-03-28T10:59:48Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

+    }
+    @Override
+    public int getCategory() {
+        return Category.INFO_GATHER;


If we're actually doing testing then maybe Server Security? Or Miscellaneous.
Thats unless we want to add a new "Authentication" category??

psiinon · 2025-03-28T11:01:24Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

+
+                }
+                else{
+                    //LOGGER.error("Authentication Method is not browser based.");


Either actually include logging (e.g. at DEBUG level) or remove the commented code

psiinon · 2025-03-28T11:03:12Z

The build is failing due to formatting issues.
Run ./gradlew :addOns:ascanrulesAlpha:spotlessApply to fix these violations.

kingthorin · 2025-03-31T19:10:02Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

+    @Override
+    public String getSolution() {
+        return "N/A";
+    }
+    @Override
+    public String getReference() {
+        return "N/A";
+    }


You could just not override these methods

…ire messages

…ome.html

…owserBasedAuthenticationMethod to remove false positive issues

…flow for active scan rules

AliceMilshtein · 2025-05-07T04:51:16Z

I have read the CLA Document and I hereby sign the CLA

kingthorin · 2025-05-07T11:33:15Z

I haven't had a detailed look but I did notice that some new classes have 2023 license headers 😉

psiinon · 2025-05-09T15:45:18Z

Just started playing with the examples in the dev add-on :)
They are all under /api/openapi which is really for OpenAPI related content.
I think they would make more sense being under something like /auth, /auth/totp or /totp.

psiinon · 2025-05-09T15:56:56Z

I also noticed that in many (all) cases pressing RETURN did not submit the TOTP token. Was that deliberate? Most reasonable web apps I've used do accept RETURN as submit, but I can definitely see the point of having some that dont work in that way.

…eration

psiinon · 2025-06-06T08:57:07Z

@AliceMilshtein this PR still has conflicts

psiinon · 2025-06-06T12:59:22Z

...rg/zaproxy/addon/dev/auth/totp/simpleAuthTotpBlankCodeVuln/OpenApiWithBlankOtpLoginPage.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.zaproxy.addon.dev.auth.totp.simpleAuthTotpBlankCodeVuln;


I would argue that TOTP is not simpleAuth 😉 So maybe packages like totpBlankCodeVuln ?

psiinon · 2025-06-06T12:59:54Z

...rg/zaproxy/addon/dev/auth/totp/simpleAuthTotpBlankCodeVuln/OpenApiWithBlankOtpLoginPage.java

+import org.zaproxy.addon.dev.TestProxyServer;
+import org.zaproxy.addon.network.server.HttpMessageHandlerContext;
+
+public class OpenApiWithBlankOtpLoginPage extends TestPage {


These classes are nothing to do with OpenApi 😁

psiinon · 2025-06-06T13:02:02Z

I fixed the conflict locally and had a go with the test apps.
With http://localhost:9091/auth/totp/simple-auth-otp-blank-code-vuln/ I submitted the creds but got an error logged:
/
Ignore that - I hadnt updated the authhelper 😁

psiinon · 2025-06-06T13:10:25Z

...esAlpha/src/main/java/org/zaproxy/zap/extension/ascanrulesAlpha/BlankTotpActiveScanRule.java

+
+            if (webSessionBlankCode) {
+                buildAlert(
+                                "Blank Passcode Vulnerability",


All text shown to the user should be i18n'ed. Use code like Constant.messages.getString(MESSAGE_PREFIX + "soln");

AliceMilshtein added 4 commits March 7, 2025 09:14

Add ZAP test app with TOTP authentication that has blank passcode vul…

cd926fc

…nerability

Added TOTP scan rule to check for blank passcode vulnerability

aed7370

Add TOTP scan rule to check for replay vulnerabilities

d608e64

Add TOTP rule to check if captcha/lockout mechanism is present & add …

aef7ddc

…rule to check if random passcode meeting format will give access

AliceMilshtein added 6 commits March 26, 2025 14:54

Add TOTp scan rule to check for lockout/captcha mechanism

13a9d1c

Add test web server for authentication with totp and server for authe…

074f98b

…ntication with blank code vulnerability

Merge branch 'main' into TOTP-feature

cbe5dc7

Fix browser based authentication message storage in client handler to…

2a53113

… work with new version changes

Add test web server with totp authentication that locks account after…

471dcb8

… 3 incorrect codes entered

Add test web server with Totp Authentication that contains a captcha

88b5266

psiinon reviewed Mar 28, 2025

View reviewed changes

psiinon requested changes Mar 28, 2025

View reviewed changes

kingthorin reviewed Mar 31, 2025

View reviewed changes

AliceMilshtein added 11 commits April 7, 2025 19:35

Add spotlessApply to scan rules

0d68e14

Add spotless Apply to scan rules

df45cf7

Update all strings in active scan rules to be i18ned

ea71f84

Update ClientSideHandler to store HTTP Message Numbers instead of ent…

e74ebb0

…ire messages

Extract common TOTP scan context logic into helper class

ae985f3

Add support for overriding TOTP value in AuthenticationStep

61784d4

Update test web server's to not allow unathenticated user access to h…

0f3ad88

…ome.html

Updated BlankTotpActiveScanRule to use wasAuthTestSucessful() from Br…

6beef13

…owserBasedAuthenticationMethod to remove false positive issues

Updated totp Active scan rules to correct false positive issue

d695aba

Fixxed HTTpMessageHandler not recording messages properly

20af733

Added support for use of custom field or TOTP step in authentication …

b2a7a13

…flow for active scan rules

kingthorin marked this pull request as draft May 7, 2025 11:33

fixed headers

bddcb68

AliceMilshtein added 7 commits May 27, 2025 01:03

Added support for return & updated test servers to implement totp gen…

45ead48

…eration

Added support for return and totp generation

ac3fae4

Fixed override of totp value in scan rule issue

a871046

Addes test server with replay vulnerability + fixed replay issues

dc189a0

replay cont

73f886e

Added commonCode vulnerability test server

b12f85e

styling

dc46b1b

psiinon requested changes Jun 6, 2025

View reviewed changes

psiinon reviewed Jun 6, 2025

View reviewed changes

AliceMilshtein added 2 commits June 6, 2025 09:38

Merge branch 'main' into TOTP-feature + resolve conflicts

e27d61f

styling

69876c9

kingthorin pushed a commit to kingthorin/cla that referenced this pull request Sep 18, 2025

@AliceMilshtein has signed the CLA in zaproxy/zap-extensions#6255

948e069

		@@ -0,0 +1,167 @@
		package org.zaproxy.zap.extension.ascanrulesAlpha;

Uh oh!

[WIP] Totp Active Scan Rules #6255

Are you sure you want to change the base?

[WIP] Totp Active Scan Rules #6255

Uh oh!

Conversation

AliceMilshtein commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Related Issues

Checklist

Uh oh!

github-actions bot commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psiinon commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job, no security vulnerabilities found in this Pull Request

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psiinon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psiinon commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliceMilshtein commented May 7, 2025

Uh oh!

kingthorin commented May 7, 2025

Uh oh!

psiinon commented May 9, 2025

Uh oh!

psiinon commented May 9, 2025

Uh oh!

psiinon commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psiinon commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

AliceMilshtein commented Mar 7, 2025 •

edited

Loading

github-actions bot commented Mar 7, 2025 •

edited

Loading

psiinon commented Mar 7, 2025 •

edited

Loading

psiinon commented Jun 6, 2025 •

edited

Loading