Circular redirection support

[Gerwout Van der Veen]

Hi all,

Let me first introduce myself, because this is my first post to this group. 

I am Gerwout (yep, that name is impossible to pronounce if Dutch isn't your native language...) and I have worked in the security field for almost 10 years. I have done quite some development (mainly in other languages than JAVA), but I am not a JAVA expert, so please keep that in mind while reading through this post.

I was looking into the attack module and I've noticed for a specific domain that I was testing (can't mention the name, it's internal anyway) that I would always get the "Failed to attack the URL" message. I increased the verbosity of debugging in log4j.properties and that didn't give me any pointers as well. So, I checked out all the code, got my IDE up and running and checked myself what was going on for this specific domain. I've identified the problem and I was planning to implement a fix, however I need some advice, so let me explain the problem. I will use a dummy domain in my example.

http://www.webapplication.com redirects to http://www.webapplication.com/index.php. The index.php page forwards to http://www.webapplication.com/index.php?redirect_to=blah and that page redirects to http://www.webapplication.com/index.php?redirect_to=final_blah. So for whatever unknown reason this web application really likes redirects to the same page. If your site works like this, it's for ZAP currently not possible to use the attack module. The reason for this is that the Paros HttpSender class uses the Apache HttpClient. This client considers a redirect to the same page (even though the query parameter differs) a circular redirect. An exception will be thrown and ZAP will stop. So, I was thinking to implement the following:

- If this use case happens, there should be a reason mentioned in the Failed to attack message.

- Create a configuration option (maybe under connection?) that enables the usage of circular redirects and give the option to define the maximum number of allowed redirects to the same page (i.e. ClientPNames.ALLOW_CIRCULAR_REDIRECTS and ClientPNames.MAX_REDIRECTS). This should fix this problem.

I have another question as well. Is there any reason that the private method getHttpSender in the QuickStart extension is stateless by default? I think it makes sense for an automated module to have cookie support enabled by default. 

And last, but not least: are there already plans to move to a newer version of that Apache HTTP library?

Cheers,

Gerwout

[thc...@gmail.com]

Hi.

Welcome! :)

This client considers a redirect to the same page (even though the query parameter differs) a circular redirect.

Yeah, the query component is not taken into account when checking for already accessed URIs. We should change that to fix the issue.

So, I was thinking to implement the following:

- If this use case happens, there should be a reason mentioned in the Failed to attack message.

- Create a configuration option (maybe under connection?) that enables the usage of circular redirects and give the option to define the maximum number of allowed redirects to the same page (i.e. ClientPNames.ALLOW_CIRCULAR_REDIRECTS and ClientPNames.MAX_REDIRECTS). This should fix this problem.

Both changes seem good to me and "Connection" panel is the best place for the new options.

Just a quick note, we are using Apache Commons HttpClient not Apache HttpComponents (Client). The constants should be HttpClientParams.ALLOW_CIRCULAR_REDIRECTS and HttpClientParams.MAX_REDIRECTS.

I have another question as well. Is there any reason that the private method getHttpSender in the QuickStart extension is stateless by default? I think it makes sense for an automated module to have cookie support enabled by default. 

You mean that because of "allowState" being true and the option "Enable Session Tracking (cookie)" being disabled?

And last, but not least: are there already plans to move to a newer version of that Apache HTTP library?

There are no plans to move to Apache HttpComponents in the following weeks (at least that I'm aware of), though that was talked about some time ago [1].


[1] https://groups.google.com/d/topic/zaproxy-develop/h0dXzApY49Y/discussion

Best regards.

--
You received this message because you are subscribed to the Google Groups "OWASP ZAP Developer Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to zaproxy-devel...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[psiinon]

Yes, welcome to the ZAP Dev group, and thanks for doing so much investigating before getting in touch :)

I'm good with your suggested configuration enhancements as well - do you fancy having a go at them?

We should try to keep up to date with all of the libraries we use as long as it wont cause compatibility problems.
We're working towards ZAP 1.2.4 so this is actually good time to update them, as we'll regenerate all of the add-ons when we do the release.

I've just tried updating the main Apache libs and they seem to work ok (with basic testing).
And that includes moving to the HttpComponenets client library :)
I'll start checking these in (unless anyone knows of good reasons not to) and will start working through the other libs as well.
Yes, they will need more testing, but I think the most effective testing will be to get them into the trunk asap so that everyone using it (and the weekly releases) will start using them.
If anyone else feels like helping update some of the other libs then please do ;)

Note that we shouldnt update the libs in the zap-extensions project until much closer to 2.4.1 as it will prevent us from releasing add-ons on 2.3.1.

Cheers,

Simon

To unsubscribe from this group and stop receiving emails from it, send an email to zaproxy-develop+unsubscribe@googlegroups.com.

[Gerwout Van der Veen]

Just a quick note, we are using Apache Commons HttpClient not Apache HttpComponents (Client). The constants should be HttpClientParams.ALLOW_CIRCULAR_REDIRECTS and HttpClientParams.MAX_REDIRECTS.

Well spotted! Even though it was working with the other constants as well, so they probably have the same value anyway.

 

You mean that because of "allowState" being true and the option "Enable Session Tracking (cookie)" being disabled?

Hmmm, I am a little bit confused right now. I can recall that I was testing the site that does all these redirects and the initial redirect did set a cookie as well. In the next request, it didn't reuse that cookie. However I did look at the code again (the allowState parameter) and the AtttackThread.java does set this to true. I was in the impression that it was false, but code does not lie, so, it's probably just me :-) Once I've finalized this new circular redirect feature, I will have a look at this again and see if I can replicate it at that point in time. 

[Gerwout Van der Veen]

I'm good with your suggested configuration enhancements as well - do you fancy having a go at them?

I am already implementing this functionality. I will probably be feature complete somewhere tomorrow. 

I am interested in how you guys want me to send this patch. I could quite easily create a diff based on the latest trunk and you guys can have a proper code review and submit it accordingly. I did have a quick read through the developer docs (coding guidelines, etc.) and I don't have any problems with them. I still think it makes kind of sense that a more mature JAVA developer has a look as well. Even though I did do a couple of those SUN certifications in the past, I wouldn't call my self a senior JAVA developer.

Anyway, just let me know how you want to receive the patch and I will fix this small annoyance.

[psiinon]

Excellent.

You can attach a patch to the latest trunk on either this thread or onto a new issue.
We'll give it a once over and once its ok give you commit access :)

Many thanks,

Simon

[thc...@gmail.com]

Hi.

Just a quick note, we are using Apache Commons HttpClient not Apache HttpComponents (Client). The constants should be HttpClientParams.ALLOW_

CIRCULAR_REDIRECTS and HttpClientParams.MAX_REDIRECTS.

Well spotted! Even though it was working with the other constants as well, so they probably have the same value anyway.

Yes, the constants have the same values in both versions :)

You mean that because of "allowState" being true and the option "Enable Session Tracking (cookie)" being disabled?

Hmmm, I am a little bit confused right now. I can recall that I was testing the site that does all these redirects and the initial redirect did set a cookie as well. In the next request, it didn't reuse that cookie. However I did look at the code again (the allowState parameter) and the AtttackThread.java does set this to true. I was in the impression that it was false, but code does not lie, so, it's probably just me :-) Once I've finalized this new circular redirect feature, I will have a look at this again and see if I can replicate it at that point in time. 

Sorry if I was not clear, the parameter "allowState" even if true does not necessarily means that the cookies will be sent, that will depend on the state of the option "Enable Session Tracking (cookie)". So, unless you enabled it before "attacking", the behaviour that you're describing (i.e. does not handle cookies) is the expected behaviour.

The question was more about if the "stateless" problem was with the option "Enable Session Tracking (cookie)" being disabled by default. But I now see that's not what you were asking/talking and agree that the "AttackThread" should handle (or allow to handle) the cookies by default independently of the "Enable Session Tracking (cookie)" option.
In that case the parameter "allowState" should be false (yes, that's misleading name it should be (something like) "useGlobalState"), the only problem (if we can call it a problem) is that the user will no longer be able to control if the cookies should be handled or not, they will always (unless we do more changes ;)
In any case this requires more thoughts/opinions.

Best regards.

[Gerwout Van der Veen]

Hi Simon,

In the attachments you will find 2 patches.

The first one if for the quickstart extension and will give nice messages if you attack a site that has circular redirects. In addition to that, I now log the exception details in the accessNode method. I've handled the CircularRedirectException and the RedirectException. All other exceptions will be logged, so that we can get some proper feedback if something unknown goes wrong. I am quite certain that this will give better bug tickets. I already stumbled upon one of those edge cases. I had a typo in my outgoing proxy server and I got a HostNotFoundException in the same code. Even though it was my own mistake, it took me a while to figure it out, because I assumed that it had something to do with the code that I just changed. 

The second patch is the bulk of the work. It adds the 2 new options on the connections tab and implements the logic so that the options are in use. I've added a section to the help page for connections.html as well. Couldn't get it to show in the help itself, but I think that I just need to do something (i.e. weird build command?) that I am not aware of yet. I am quite certain that I did edit the correct help page. So if you could tell me what I should do -or what I have forgotten-, that would be appreciated.

Anyway let me know your thoughts.

Gerwout

p.s. did read an older post that you guys considered using Github, but at that point in time you didn't have a lot of git experience. Are you already a git guru? Because you should :-) Not trying to start a flame war, but using SVN feels like using Windows 3.11 as your desktop environment if you have used git for several years already.

[kingthorin+owaspzap]

Gerwout, for the help item you need to open zaproxy\build\build.xml and run the ant task "generate-help-jars".

[thc...@gmail.com]

Also mentioned in the "Authoring help files" wiki page [1] ;)


[1] https://code.google.com/p/zaproxy/wiki/AuthoringHelp

Best regards.

On 10/09/14 19:55, kingthorin+owaspzap wrote:

Gerwout, for the help item you need to open zaproxy\build\build.xml and run the ant task "generate-help-jars".

--
You received this message because you are subscribed to the Google Groups "OWASP ZAP Developer Group" group.

To unsubscribe from this group and stop receiving emails from it, send an email to zaproxy-devel...@googlegroups.com.

[Gerwout Van der Veen]

Thanks for mentioning that. It all works :-).

In the attachment you will find an additional patch for the wiki.

If you guys want to test this new functionality you can do that using this ip as your target: 37.34.60.95. It's my own VM that shows the problem. It's not the fastest VM, but if you apply the patches you will be able to attack this.

[Gerwout Van der Veen]

Hi all,

Did anyone have time to review my patches? Would like to have them included in the next release if possible.

Cheers,

Gerwout

[thc...@gmail.com]

Hi.

Yes, I did. I'll send the reviews asap (probably this afternoon).

Best regards.

[Thiago Porciúncula]

Hello,

Are there any plans to include this in ZAProxy? I can't seem to find any pull requests or commits related to this.

All I could find is this related issue, where thc202 simply states that there is no option to allow circular redirects.

Thank you.

[thc...@gmail.com]

Hi.

Yes, that's on the plans. The proposed change has almost one year and,
unfortunately, it stalled, that's why there's no commit (or a PR).

The support for allowing circular redirections will be available in the
next release (not 2.4.2), if everything goes as expected.

Best regards.

On 01/09/15 14:38, Thiago Porciúncula wrote:
> Hello,
>
> Are there any plans to include this in ZAProxy? I can't seem to find any
> pull requests or commits related to this.
>
> All I could find is this related issue

> <https://github.com/zaproxy/zaproxy/issues/1710#issuecomment-123366215>,

> where thc202 simply states that there is no option to allow circular
> redirects.
>
> Thank you.
>

> --
> You received this message because you are subscribed to the Google
> Groups "OWASP ZAP Developer Group" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to zaproxy-devel...@googlegroups.com

> <mailto:zaproxy-devel...@googlegroups.com>.

[Thiago Porciúncula]

I'm glad to hear that!

Thank you.

> <mailto:zaproxy-develop+unsub...@googlegroups.com>.