Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move time based attacks to their own scan rules #4316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wapmon
Copy link
Contributor

@wapmon wapmon commented Dec 19, 2022

Fix zaproxy/zaproxy#7341
Breaking out time based attack from list in issue into their own rules. Most of the RDBMS specific SqlInjection rules were already exclusively performing time based attacks, so these simply got renamed, with the exception of the SqlLite rule, which was split up and the new timing rule was given its own new plugin ID. Most other rules were also split up in this manner to create a new timing based version with its own plugin ID. There is mention in the issue of modifying the Alert tags that correspond to these rules, so I would appreciate some feedback on what to do around that.

@thc202 thc202 changed the title Break out timing based attacks Move time based attacks to their own scan rules Dec 20, 2022
@thc202
Copy link
Member

thc202 commented Dec 22, 2022

The help and changelogs should be updated.

@wapmon
Copy link
Contributor Author

wapmon commented Dec 27, 2022

I've addressed the renaming and the changelog. As for the help section, I'm not so familiar with this part. Do I just need to make an update the Message.properties files for both packages as well? If so what updates are you looking for? Just name updates?

@kingthorin
Copy link
Member

@kingthorin
Copy link
Member

@psiinon @thc202 should we be reserving new scan rule IDs for these?

@kingthorin
Copy link
Member

The changes need a spotlessApply

@wapmon
Copy link
Contributor Author

wapmon commented Dec 28, 2022

I've updated the help.html of both packages and fixed some merge conflicts. Let me know if this doesn't look right. Thanks!

*
* ZAP is an HTTP/HTTPS proxy for assessing web application security.
*
* Copyright 2020 The ZAP Development Team
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2022?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this applies to all of the "new" Timing classes? Though I guess addressing it can wait till and be dependant on whether or not we're adding plugin IDs.

@wapmon
Copy link
Contributor Author

wapmon commented Jan 10, 2023

Hello. Just wanted to check in and see if there's anything else you want me to address on this PR. Thanks!

Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psiinon or @thc202 please confirm that new scan rule IDs should be reserved for these and that having duplicate IDs will be an issue in the UI/policies.

@thc202
Copy link
Member

thc202 commented Jan 10, 2023

New rules should have new IDs.

@kingthorin
Copy link
Member

@wapmon Scan rule IDs for the timing rules can be reserved via a PR against: https://github.com/zaproxy/zaproxy/blob/main/docs/scanners.md

@wapmon
Copy link
Contributor Author

wapmon commented Jan 11, 2023

zaproxy/zaproxy#7687

@kingthorin
Copy link
Member

@wapmon are you able to finish this and address the conflict?

@FiveOFive
Copy link
Contributor

@wapmon - I could pick up finishing this, if you don't have time right now. Would that be helpful?

I'm looking forward to this change. Thanks for all the work getting it going!

@thc202
Copy link
Member

thc202 commented Apr 20, 2023

This will be handled by the core team as discussed in the last team meeting.

@thc202 thc202 force-pushed the break-out-timing-based-attacks branch 2 times, most recently from f4bb7b6 to bff9ea8 Compare April 22, 2023 16:23
@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
## Unreleased
### Changed
- Maintenance changes.
- Move time based attacks to their own scan rules (Issue 7341).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be more explicit and mention the new rules/IDs.

@thc202 thc202 force-pushed the break-out-timing-based-attacks branch from 94ca253 to e749a3d Compare April 23, 2023 17:12
Most of the RDMS-specific SQL injection rules were already only doing
Time Based, so I simply renamed those. The SqlLite Rule was broken into
2 rules, but the original rule has the union based attack deactivated
due to a bug, so it now will not do anything.

Signed-off-by: wapmon <[email protected]>
@thc202 thc202 force-pushed the break-out-timing-based-attacks branch from e749a3d to 31898a9 Compare June 9, 2023 13:21
@thc202
Copy link
Member

thc202 commented Jan 18, 2024

Review that the new rules:

  • Have example alerts.
  • Validate references.
  • Include link to the alert ID and have anchor to link back.
  • Extend CommonActiveScanRuleInfo.
  • Specific names.
  • ?

@thc202
Copy link
Member

thc202 commented Jan 18, 2024

I'm going to split rules that can already be merged into other PRs, starting with MongoDbInjectionScanRule.

@kingthorin
Copy link
Member

Sounds great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants