zaproxy / zaproxy

The ZAP core project
https://www.zaproxy.org
Apache License 2.0
12.48k stars 2.23k forks source link

Allow scanners to reset state of the app #6067

Open DiogoMRSilva opened 4 years ago

DiogoMRSilva commented 4 years ago

Is your feature request related to a problem? Please describe. Some scanners need to know the state of the application before they start to do their job. When scanners run they change the state of the application to something that is probably different from the state of the application if the requests in the sites tree had been sent. This may lead to false information about the application state.

Describe the solution you'd like Add a method that resend all the original requests. One possible location is HostProcess

Would you like to help implementing this feature? yes

psiinon commented 4 years ago

How were you thinking this could be implemented? How would you identify the relevant requests that correctly reset the state?

kingthorin commented 4 years ago

I believe the thought is "known" state not necessarily "correct" state.

Edit: To better know what param values are shown where.

DiogoMRSilva commented 4 years ago

I believe the thought is "known" state not necessarily "correct" state.

Yes @kingthorin is correct, the idea is to put the application in the state ZAP knows.

How were you thinking this could be implemented?

Each AbstractPlugin needs to declare if they need a "clean" state before running. Then when hostProcess is processing each plugin it need to check if they need the "clean" state, if so it resends all the original messages before running the plugin on any request.

psiinon commented 4 years ago

Its the "all of the original messages" part that I'm interested in - how will we identify these? Will a user be able to specify their own if they know whats required?

thc202 commented 4 years ago

The "original messages" are the ones that ZAP is currently scanning (obtained from the Sites tree), we should call them "base messages" (the name used in the scan rules).

Some background, this issue was raised following some discussions about the persistent XSS scan rules (for/from zaproxy/zap-extensions#2443).

When scanners run they change the state of the application to something that is probably different from the state of the application if the requests in the sites tree had been sent.

Just to clarify, the base messages are not changed while active scanning, the scan rules don't change the base messages but copies of those messages, so a following scan rule can still check the preconditions solely based on the response of the base message without worrying if the response is stale or not. This point is important for scan rules that check for content/changes/reflections in the response of the message as they will still work properly regardless of the actual state of the target.

Now the problem (this issue) is that some scan rules need to check other base messages than the one being scanned (case of persistent XSS) but the base messages might not be consistent between themselves. For example, lets say that the scanner has a base message that sends X and that the X is reflected in the base message A, if something/someone sends Y (the user playing with a parameter?) but did not resend the message A, the base message A will still have X. Resending all messages would restore to the expected state (i.e. the base message A would have Y) and having the expected state would allow the scan rules to analyse all the base messages before starting to scan them (assuming the HostProcess is changed to give access to that information).

Each AbstractPlugin needs to declare if they need a "clean" state before running.

I'm not sure that would be necessary, once should be enough to get the proper state, each scan rule can then use the expected state for its own analysis.

There are some details that need to be taken into account after the analysis and starting testing though. Taking the persistent XSS scan rules as example:

Even with the expected state we have a problem in the 2nd step of the scan rule which uses the base messages which would/could change back to the previous (non-unique) value (order would matter of course, e.g. if base message A is checked before resending the Y again, it would work). Changing the 2nd step to spider with the unique values instead of the base messages would prevent that. The newer implementation of the persistent XSS scan rules (mentioned PR) has a similar problem.