[dev] [PATCH] bgpd: fix length calculation of multi-segment AS_PATH UPDATE messages

Donald Sharp sharpd at cumulusnetworks.com
Fri Sep 8 07:53:36 EDT 2017


Andreas -

Thanks for taking the time and solving this AS_PATH issue!

I'm about to submit this patch for a Pull Request into FRR.  I'm
attributing the patch to you, before I can do so I need your
'Signed-off-by:...'.  I'm including the relevant part of our
documentation to see what we think it signifies:

`Signed-off-by:` this is a developer's certification that he or she has the
right to submit the patch for inclusion into the project. It is an agreement to
the Developer's Certificate of Origin (below). Code without a proper signoff
can not and will not be merged.

If you are unfamiliar with this process, you should read the [official policy
at kernel.org](http://www.kernel.org/doc/Documentation/SubmittingPatches) and
you might find this article about [participating in the Linux community on the
Linux Foundation
website](http://www.linuxfoundation.org/content/how-participate-linux-community-0)
to be a helpful resource.

In short, when you sign off on a commit, you assert your agreement to all of
the following:

> Developer's Certificate of Origin 1.1
>
> By making a contribution to this project, I certify that:
>
> (a) The contribution was created in whole or in part by me and I
>     have the right to submit it under the open source license
>     indicated in the file; or
>
> (b) The contribution is based upon previous work that, to the best
>     of my knowledge, is covered under an appropriate open source
>     license and I have the right under that license to submit that
>     work with modifications, whether created in whole or in part
>     by me, under the same open source license (unless I am
>     permitted to submit under a different license), as indicated
>     in the file; or
>
> (c) The contribution was provided directly to me by some other
>     person who certified (a), (b) or (c) and I have not modified
>     it.
>
> (d) I understand and agree that this project and the contribution
>     are public and that a record of the contribution (including all
>     personal information I submit with it, including my sign-off) is
>     maintained indefinitely and may be redistributed consistent with
>     this project or the open source license(s) involved.

thanks!

donald

On Thu, Sep 7, 2017 at 10:54 PM, Andreas Jaggi via dev
<dev at lists.frrouting.org> wrote:
> _______________________________________________
> dev mailing list
> dev at lists.frrouting.org
> https://lists.frrouting.org/listinfo/dev
>
>
> ---------- Forwarded message ----------
> From: Andreas Jaggi <aj at open.ch>
> To: quagga-dev at lists.quagga.net, dev at lists.frrouting.org
> Cc:
> Bcc:
> Date: Fri, 8 Sep 2017 04:53:37 +0200
> Subject: [PATCH] bgpd: fix length calculation of multi-segment AS_PATH UPDATE messages
> Hi
>
> There is a bug in the length calculation of mutli-segment AS_PATH UPDATE messages, which can make bgpd send out malformed BGP UPDATE packets and in turn causes that BGP sessions are reset.
>
> This thread on quagga-users shows how the problem manifests: https://lists.quagga.net/pipermail/quagga-users/2017-September/014830.html
>
> In our case we had an UPDATE message having an AS_PATH with 567 entries which was split into three segments (255+255+57).
> This results in an AS_PATH attribute of the following length: 2274 bytes
> (eg. 2 bytes segment header + 255*4 segment length + 2 bytes segment header + 255*4 segment length + 2 bytes segment header + 57*4 segment length)
> The AS_PATH attribute was written properly into the message, but the length written in the path attribute header by bgpd was 3294 bytes instead of 2274 (thus resulting in an invalid/malformed BGP UPDATE message).
>
> Turns out that the aspath_put function wrongly double-accumulates the written bytes and then ends up with these 3294 bytes instead of 2274.
> (eg. 2 bytes segment header + 255*4 segment length + 2 bytes segment header + 510*4 segment length + 2 bytes segment header + 57*4 segment length).
>
>
> The following patch (against the master branch of Quagga) fixes the length calculation in aspath_put:
>
> diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c
> index b7af5e88..d813bfba 100644
> --- a/bgpd/bgp_aspath.c
> +++ b/bgpd/bgp_aspath.c
> @@ -903,7 +903,7 @@ aspath_put (struct stream *s, struct aspath *as, int use32bit )
>                assegment_header_put (s, seg->type, AS_SEGMENT_MAX);
>                assegment_data_put (s, seg->as, AS_SEGMENT_MAX, use32bit);
>                written += AS_SEGMENT_MAX;
> -              bytes += ASSEGMENT_SIZE (written, use32bit);
> +              bytes += ASSEGMENT_SIZE (AS_SEGMENT_MAX, use32bit);
>              }
>
>            /* write the final segment, probably is also the first */
>
>
> It looks like this bug has been slumbering for 10 years since its introduction in 2007 when it was added with the AS4 support changes (commit 0b2aa3a0).
>
> Which leads us to the following affected versions:
> Quagga: 0.99.10 - 0.99.24.1, 1.0.*, 1.1.0, 1.1.1, 1.2.0, 1.2.1
> FRR: 2.0, 3.0-rc1
>
>
> Cheers
> Andreas
>
>
> --
> andreas jaggi
> lead engineer network services
>
> open systems ag
> raeffelstrasse 29
> ch-8045 zurich
> t: +41 58 100 10 10
> f: +41 58 100 10 11
> aj at open.ch
>
> http://www.open.ch
>
>



More information about the dev mailing list