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.

Sign up for GitHub

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

Jump to bottom

Break out Blind/Timing Based Injections To Separate Rules #7341

Open
10 tasks
sgerlach opened this issue Jun 13, 2022 · 13 comments · May be fixed by zaproxy/zap-extensions#4316
Open
10 tasks

Break out Blind/Timing Based Injections To Separate Rules #7341

sgerlach opened this issue Jun 13, 2022 · 13 comments · May be fixed by zaproxy/zap-extensions#4316
Labels
add-on enhancement

Comments

@sgerlach
Copy link
Contributor

sgerlach commented Jun 13, 2022 •
edited by psiinon

Is your feature request related to a problem? Please describe.

In at least the SQL Injection (40018) and OS Command Injection (90020) plugins, there are definitive attack/response detections and non-definitive attack/responses usually assigned with blind injection attacks. Those attacks attempt to perform a sleep injection to control the applications timing and perform StdDev work on the responses to see if they were successful.

The timing based alerts consistently generate False Positives due to a handful of factors, one of them being the application is under heavy load. This makes troubleshooting and diagnosis very hard.

Describe the solution you'd like

Proposal is to break out those blind attacks to their owns plugins so they can be optimized.

  • Allows the rule to be disabled and the other parts of the core rule to run
  • Allows the Blind attack to have better retest/validation logic
  • Will integrate better with the upcoming retest work to only have to run the blind rules or the non-blind rules

Describe alternatives you've considered

  • Adjustments on timing
  • Running ONLY these rules to keep load off the app (does not scale well for automation)
  • Adding additional test logic to the current rules to validate alerts

Screenshots

No response

Additional context

No response

Would you like to help fix this issue?

  • Yes

Update - the rules that perform timing based attacks

  • 40019 (limited unit tests)
  • 40020 (limited unit tests)
  • 40021 (limited unit tests)
  • 40022 (limited unit tests)
  • 40024 (limited unit tests)
  • 40027 (limited unit tests, might only do timing attacks?)
  • 40033 (limited unit tests)
  • 90018 (no unit tests)
  • 90020 (limited unit tests)
@thc202 thc202 added the add-on label Jun 13, 2022
@psiinon
Copy link
Member

psiinon commented Jun 15, 2022

I definitely like the idea of people being able to disable all of these checks in one go.
I do worry that moving them all into a new add-on will be difficult, but lets discuss what options we have.
One possibility would be to abuse the 'technology' configs - maybe having a new "Attacks" / "Timing" option which would then work with minimal changes. We might be able to use this for other things?

@thc202
Copy link
Member

thc202 commented Jun 15, 2022

Just splitting into their own scan rules and keeping them in the same add-ons sounds better.

@psiinon
Copy link
Member

psiinon commented Jun 15, 2022

We could tag them as well?

@thc202
Copy link
Member

thc202 commented Jun 15, 2022

Definitely.

@kingthorin
Copy link
Member

Do you mean with Alert tags? Or do you mean with Tech as you originally suggested?

If you mean Alert tags then yes definitely.

@thc202
Copy link
Member

thc202 commented Jun 15, 2022

I thought that was for the rules themselves, but in the alerts it works as well.

@psiinon
Copy link
Member

psiinon commented Jun 15, 2022

We've discussed this on IRC and we like the idea of splitting the timing checks into different rules, but keeping them in the same add-ons. The alert tags would be a bonus...

@psiinon
Copy link
Member

psiinon commented Jun 15, 2022

I've updated the first comment with a list of all the rules that I could find which perform timing based attacks - anyone think of any that I've missed?

@DiogoMRSilva
Copy link
Contributor

In my opinion that shouldn't be the solution to the time attacks fps. My suggested solutions are: improving the time delay detection logic by making an unique method in the common lib that would deal with the maths and number of requests, don't run time based if confidence is high, use several detection methods (delay, reflection, error) simultaneously and if more than one work increase confidence.

Some disadvantages of splitting the rules:

  • no having different types of detections together in the same rule to increase confidence.
  • loss of control on the amount of request the user wants to do on each vulnerability, since strength is +- correlated to the number of request if we have both at high we have twice the number of allowed requests
  • Have many entries to chose in the scan policies

@sgerlach
Copy link
Contributor Author

Thanks for the conversation here. I'm going to bounty each of these tasks to get some 💵 associated with this work.

@wapmon wapmon linked a pull request Dec 19, 2022 that will close this issue
Open
@FiveOFive
Copy link

Great to see the PR (zaproxy/zap-extensions#4316) is making it's way through. I'm also seeing a high number of false positives from timing based attacks and looking forward to this change. Let me know if there are outstanding items to get this done that need to be picked up and I can try to help out with them. Thanks!

@FiveOFive
Copy link

Hi ZAP team - checking in to see if this is still planned?

@thc202
Copy link
Member

thc202 commented Oct 10, 2023

Yes but still pending on fixes being done to the scan rules.

@thc202 thc202 mentioned this issue Jan 19, 2024
Merged
thc202 pushed a commit to thc202/zap-extensions that referenced this issue Jan 19, 2024
@wapmon @thc202
ascanrulesAlpha: add rule for MongoDB time tests
b7127fb 
Move time based tests to its own rule, ID 90039.

Part of zaproxy/zaproxy#7341.

Signed-off-by: wapmon <wapmon@aol.com>
thc202 pushed a commit to thc202/zap-extensions that referenced this issue Jan 19, 2024
@wapmon @thc202
ascanrulesAlpha: add rule for MongoDB time tests
283fc44 
Move time based tests to its own rule, ID 90039.

Part of zaproxy/zaproxy#7341.

Signed-off-by: wapmon <wapmon@aol.com>
thc202 pushed a commit to thc202/zap-extensions that referenced this issue Jan 19, 2024
@wapmon @thc202
ascanrulesAlpha: add rule for MongoDB time tests
34e029f 
Move time based tests to its own rule, ID 90039.

Part of zaproxy/zaproxy#7341.

Signed-off-by: wapmon <wapmon@aol.com>
thc202 pushed a commit to thc202/zap-extensions that referenced this issue Jan 19, 2024
@wapmon @thc202
ascanrulesAlpha: add rule for MongoDB time tests
6bfbe44 
Move time based tests to its own rule, ID 90039.

Part of zaproxy/zaproxy#7341.

Signed-off-by: wapmon <wapmon@aol.com>
thc202 pushed a commit to thc202/zap-extensions that referenced this issue Jan 22, 2024
@wapmon @thc202
ascanrulesAlpha: add rule for MongoDB time tests
fdd1241 
Move time based tests to its own rule, ID 90039.

Part of zaproxy/zaproxy#7341.

Signed-off-by: wapmon <wapmon@aol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
add-on enhancement
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move time based attacks to their own scan rules wapmon/zap-extensions
6 participants
@psiinon @FiveOFive @sgerlach @thc202 @DiogoMRSilva @kingthorin