Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG: NetScaler CIP handling is incorrect

Posted by Andreas Mahnke 
Andreas Mahnke
[PATCH] BUG: NetScaler CIP handling is incorrect
July 06, 2017 01:40PM
Hello,

this patch resolves a bug inside the "NetScaler CIP handling"
implementation.
The issue is explained deeper in the existing mailling list thread under
[1].

[1]: https://www.mail-archive.com/[email protected]/msg25100.html

Best regards,
Andreas

From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
From: mahnke <[email protected]>
Date: Thu, 6 Jul 2017 12:56:10 +0200
Subject: [PATCH] BUG: NetScaler CIP handling is incorrect

The "NetScaler CIP handling" implementation was not working as expected.

Based on tcp dump analysis of the CIP data and the specificaton from
citrix:

- https://support.citrix.com/article/CTX205670
-
https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/

some adjustments had to be done in the code in order to get the feature
working as expected.

The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
version "NS11.1: Build 54.14.nc"
---
src/connection.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 3629094..492fbfe 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
if (trash.len < 28)
goto missing;

- line += 8;
+ line += 12;

/* Get IP version from the first four bits */
ip_v = (*line & 0xf0) >> 4;
@@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
/* The protocol does not include a TCP header */
conn->err_code = CO_ER_CIP_BAD_PROTO;
goto fail;
- } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
+ } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
/* Fail if buffer length is not large enough to contain
* CIP magic, CIP length, IPv4 header, TCP header */
goto missing;
@@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
goto fail;
}

- line += cip_len;
+ line += (cip_len - 12);
trash.len = line - trash.str;

/* remove the NetScaler Client IP header from the request. For this
--
2.7.4
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:30PM
Hi Andreas,

[ Ccing Bertrand ]

On Thu, Jul 06, 2017 at 01:31:27PM +0200, Andreas Mahnke wrote:
> Hello,
>
> this patch resolves a bug inside the "NetScaler CIP handling"
> implementation.
> The issue is explained deeper in the existing mailling list thread under
> [1].
>
> [1]: https://www.mail-archive.com/[email protected]/msg25100.html

We didn't get feedback from Bertrand who was trying to contact Citrix
regarding this. However, I agree that your changes match the examples
shown on the support links you provided. Maybe Bertrand is/was using
a different version ?

Thanks,
Willy

----
> From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
> From: mahnke <[email protected]>
> Date: Thu, 6 Jul 2017 12:56:10 +0200
> Subject: [PATCH] BUG: NetScaler CIP handling is incorrect
>
> The "NetScaler CIP handling" implementation was not working as expected.
>
> Based on tcp dump analysis of the CIP data and the specificaton from
> citrix:
>
> - https://support.citrix.com/article/CTX205670
> -
> https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/
>
> some adjustments had to be done in the code in order to get the feature
> working as expected.
>
> The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
> version "NS11.1: Build 54.14.nc"
> ---
> src/connection.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index 3629094..492fbfe 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> if (trash.len < 28)
> goto missing;
>
> - line += 8;
> + line += 12;
>
> /* Get IP version from the first four bits */
> ip_v = (*line & 0xf0) >> 4;
> @@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> /* The protocol does not include a TCP header */
> conn->err_code = CO_ER_CIP_BAD_PROTO;
> goto fail;
> - } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
> + } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
> /* Fail if buffer length is not large enough to contain
> * CIP magic, CIP length, IPv4 header, TCP header */
> goto missing;
> @@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> goto fail;
> }
>
> - line += cip_len;
> + line += (cip_len - 12);
> trash.len = line - trash.str;
>
> /* remove the NetScaler Client IP header from the request. For this
> --
> 2.7.4
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:40PM
Hi Willy,

I had some direct mail conversations with him and he wanted to create a
patch based in the findings.
In the meantime we got it working using the patch I provided, therefore I
sent it yesterday so that it gets integrated and we do not need to patch
haproxy on our end everytime a new version comes out.

I wrote him yesterday that the patch was sent by me, but he seems to be out
of office until monday - so maybe he will get back to us then.

Regards,
Andreas

On Fri, Jul 7, 2017 at 2:25 PM, Willy Tarreau <[email protected]> wrote:

> Hi Andreas,
>
> [ Ccing Bertrand ]
>
> On Thu, Jul 06, 2017 at 01:31:27PM +0200, Andreas Mahnke wrote:
> > Hello,
> >
> > this patch resolves a bug inside the "NetScaler CIP handling"
> > implementation.
> > The issue is explained deeper in the existing mailling list thread under
> > [1].
> >
> > [1]: https://www.mail-archive.com/[email protected]/msg25100.html
>
> We didn't get feedback from Bertrand who was trying to contact Citrix
> regarding this. However, I agree that your changes match the examples
> shown on the support links you provided. Maybe Bertrand is/was using
> a different version ?
>
> Thanks,
> Willy
>
> ----
> > From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
> > From: mahnke <[email protected]>
> > Date: Thu, 6 Jul 2017 12:56:10 +0200
> > Subject: [PATCH] BUG: NetScaler CIP handling is incorrect
> >
> > The "NetScaler CIP handling" implementation was not working as expected.
> >
> > Based on tcp dump analysis of the CIP data and the specificaton from
> > citrix:
> >
> > - https://support.citrix.com/article/CTX205670
> > -
> > https://www.citrix.com/blogs/2016/04/25/how-to-enable-
> client-ip-in-tcpip-option-of-netscaler/
> >
> > some adjustments had to be done in the code in order to get the feature
> > working as expected.
> >
> > The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
> > version "NS11.1: Build 54.14.nc"
> > ---
> > src/connection.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/connection.c b/src/connection.c
> > index 3629094..492fbfe 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > if (trash.len < 28)
> > goto missing;
> >
> > - line += 8;
> > + line += 12;
> >
> > /* Get IP version from the first four bits */
> > ip_v = (*line & 0xf0) >> 4;
> > @@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > /* The protocol does not include a TCP header */
> > conn->err_code = CO_ER_CIP_BAD_PROTO;
> > goto fail;
> > - } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
> > + } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
> > /* Fail if buffer length is not large enough to contain
> > * CIP magic, CIP length, IPv4 header, TCP header */
> > goto missing;
> > @@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > goto fail;
> > }
> >
> > - line += cip_len;
> > + line += (cip_len - 12);
> > trash.len = line - trash.str;
> >
> > /* remove the NetScaler Client IP header from the request. For this
> > --
> > 2.7.4
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:50PM
On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> Hi Willy,
>
> I had some direct mail conversations with him and he wanted to create a
> patch based in the findings.
> In the meantime we got it working using the patch I provided, therefore I
> sent it yesterday so that it gets integrated and we do not need to patch
> haproxy on our end everytime a new version comes out.
>
> I wrote him yesterday that the patch was sent by me, but he seems to be out
> of office until monday - so maybe he will get back to us then.

Yep I noticed the out-of-office notification as well. Let's wait for him to
get back. I'm pretty fine with merging your patch, I just want to be certain
that everything is OK on his side as well, especially before backporting it
(since it's a bug fix).

Thanks!
Willy
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 07, 2017 02:50PM
Hello everybody,

Is there an update regarding the merging of the patch? We are thinking to
switch to version 1.8 in the near future and it would be nice to have the
patch merged, so that we do not need to merge each update on our own.

Best regards,
Andreas

On Fri, Jul 7, 2017 at 2:43 PM, Willy Tarreau <[email protected]> wrote:

> On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> > Hi Willy,
> >
> > I had some direct mail conversations with him and he wanted to create a
> > patch based in the findings.
> > In the meantime we got it working using the patch I provided, therefore I
> > sent it yesterday so that it gets integrated and we do not need to patch
> > haproxy on our end everytime a new version comes out.
> >
> > I wrote him yesterday that the patch was sent by me, but he seems to be
> out
> > of office until monday - so maybe he will get back to us then.
>
> Yep I noticed the out-of-office notification as well. Let's wait for him to
> get back. I'm pretty fine with merging your patch, I just want to be
> certain
> that everything is OK on his side as well, especially before backporting it
> (since it's a bug fix).
>
> Thanks!
> Willy
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 07, 2017 07:50PM
Hi Andreas,

On Thu, Dec 07, 2017 at 02:41:29PM +0100, Andreas Mahnke wrote:
> Hello everybody,
>
> Is there an update regarding the merging of the patch? We are thinking to
> switch to version 1.8 in the near future and it would be nice to have the
> patch merged, so that we do not need to merge each update on our own.

It seems nobody involved had any time to get back on this unfortunately.

Bertrand, just as a quick refresh, Andreas proposed a patch in this
thread changing the way you parse the NS CIP. If you're using it and
it works for you, then I suspect there might be several incompatible
versions in the wild and the fix could break other use cases. Otherwise
maybe it only affects a part you don't rely on. So I just need your
ACK/NACK on his patch (the first one in this thread started in July).

Thanks!
Willy
Bertrand Jacquin
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 11, 2017 04:10PM
Hi Andreas,

I got this really side tracked, my apology. Let me take a look at that
this evening again. Some corps need to be unburied.

I'm afraid the patch, as is, will break compatibility with other version
of the CIP protocol, I'd like haproxy to support both of them.

Cheers,
Bertrand

On 07/12/17 13:41, Andreas Mahnke wrote:
> Hello everybody,
>
> Is there an update regarding the merging of the patch? We are thinking
> to switch to version 1.8 in the near future and it would be nice to have
> the patch merged, so that we do not need to merge each update on our own.
>
> Best regards,
> Andreas
>
> On Fri, Jul 7, 2017 at 2:43 PM, Willy Tarreau <[email protected]
> <mailto:[email protected]>> wrote:
>
> On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> > Hi Willy,
> >
> > I had some direct mail conversations with him and he wanted to create a
> > patch based in the findings.
> > In the meantime we got it working using the patch I provided, therefore I
> > sent it yesterday so that it gets integrated and we do not need to patch
> > haproxy on our end everytime a new version comes out.
> >
> > I wrote him yesterday that the patch was sent by me, but he seems to be out
> > of office until monday - so maybe he will get back to us then.
>
> Yep I noticed the out-of-office notification as well. Let's wait for
> him to
> get back. I'm pretty fine with merging your patch, I just want to be
> certain
> that everything is OK on his side as well, especially before
> backporting it
> (since it's a bug fix).
>
> Thanks!
> Willy
>
>

--
Bertrand
Payments Infrastructure Engineering, Amazon

Amazon Data Services Ireland Limited registered office: One Burlington Plaza, Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 390566.
Sorry, only registered users may post in this forum.

Click here to login