Re: BUG/MAJOR: dns: overflowed dns name start position causing invalid dns error

2018-12-21 Thread Willy Tarreau
On Fri, Dec 21, 2018 at 04:17:20PM +0530, Nikhil Agrawal wrote:
> Great, Thanks .
> 
> May you backport the fix to 1.7 too?

Sure, that's why I added the info at the end of the commit message.

Willy



Re: BUG/MAJOR: dns: overflowed dns name start position causing invalid dns error

2018-12-21 Thread Nikhil Agrawal
Great, Thanks .

May you backport the fix to 1.7 too?

On Fri, Dec 21, 2018 at 4:07 PM Willy Tarreau  wrote:

> On Fri, Dec 21, 2018 at 03:40:53PM +0530, Nikhil Agrawal wrote:
> > Hi Willy,
> >
> > Thanks for response and correcting the mistake.
> >
> > Its OK for you to fix this.
> > marking this as MEDIUM is also fine :).
>
> OK thanks, now merged. It will be included into next 1.8 ASAP.
>
> Willy
>


Re: BUG/MAJOR: dns: overflowed dns name start position causing invalid dns error

2018-12-21 Thread Willy Tarreau
On Fri, Dec 21, 2018 at 03:40:53PM +0530, Nikhil Agrawal wrote:
> Hi Willy,
> 
> Thanks for response and correcting the mistake.
> 
> Its OK for you to fix this.
> marking this as MEDIUM is also fine :).

OK thanks, now merged. It will be included into next 1.8 ASAP.

Willy



Re: BUG/MAJOR: dns: overflowed dns name start position causing invalid dns error

2018-12-21 Thread Nikhil Agrawal
Hi Willy,

Thanks for response and correcting the mistake.

Its OK for you to fix this.
marking this as MEDIUM is also fine :).

Regards,
Nikhil Agrawal



On Fri, Dec 21, 2018 at 2:58 PM Willy Tarreau  wrote:

> Hi,
>
> On Fri, Dec 21, 2018 at 11:02:24AM +0530, Nikhil Agrawal wrote:
> > Dear Haproxy Maintainers,
> >
> > In dns_read_name() when dns name is used with name compression and start
> > position of name is greater than 255, name is read from incorrect
> position
> > ( actual position%256). This causes "Invalid dns error" and backend is
> > marked as down permanently.
> > eg: hexadecimal value at start of dns "0xc1 1b". "0xc" specifies name
> > compression being used. in this scenario only "1b" (27) is taken as the
> > start of the name but actual name starts from "11b" (283).
> >
> >
> > This is a regression scenario starting from version 1.7.0 and is present
> in
> > current version.
> >
> > Resolution:
> > Include nibble from byte used for checking compression. i.e take "11b" as
> > start position of dns name.
> >
> > i am attaching patch to fix this bug.
>
> It's indeed a bug, however after checking RFC1035, your patch is still
> bogus as it only includes 4 of the 6 bits :
>
>   https://tools.ietf.org/html/rfc1035#section-4.1.4
>
> The pointer takes the form of a two octet sequence:
>
> +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> | 1  1|OFFSET   |
> +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> ^ ^
> | |
> | +- your patch starts here
> +--- the standard says the length starts here
>
> If you're OK with this I can simply fix it, just let me know. Also I'd
> relabel it as medium, as it's an annoyance that requires some efforts to
> be worked around, but not a major loss of functionality.
>
> Thanks for the test case by the way ;-)
>
> Willy
>


Re: BUG/MAJOR: dns: overflowed dns name start position causing invalid dns error

2018-12-21 Thread Willy Tarreau
Hi,

On Fri, Dec 21, 2018 at 11:02:24AM +0530, Nikhil Agrawal wrote:
> Dear Haproxy Maintainers,
> 
> In dns_read_name() when dns name is used with name compression and start
> position of name is greater than 255, name is read from incorrect position
> ( actual position%256). This causes "Invalid dns error" and backend is
> marked as down permanently.
> eg: hexadecimal value at start of dns "0xc1 1b". "0xc" specifies name
> compression being used. in this scenario only "1b" (27) is taken as the
> start of the name but actual name starts from "11b" (283).
>
> 
> This is a regression scenario starting from version 1.7.0 and is present in
> current version.
> 
> Resolution:
> Include nibble from byte used for checking compression. i.e take "11b" as
> start position of dns name.
> 
> i am attaching patch to fix this bug.

It's indeed a bug, however after checking RFC1035, your patch is still
bogus as it only includes 4 of the 6 bits :

  https://tools.ietf.org/html/rfc1035#section-4.1.4

The pointer takes the form of a two octet sequence:

+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| 1  1|OFFSET   |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
^ ^
| |
| +- your patch starts here
+--- the standard says the length starts here

If you're OK with this I can simply fix it, just let me know. Also I'd
relabel it as medium, as it's an annoyance that requires some efforts to
be worked around, but not a major loss of functionality.

Thanks for the test case by the way ;-)

Willy