[dev] [PATCH] bgpd: fix length calculation of multi-segment AS_PATH UPDATE messages
Andreas Jaggi
aj at open.ch
Sat Sep 9 02:10:43 EDT 2017
Hi Donald
Yes, that's fine.
Cheers
Andreas
bgpd: fix length calculation of multi-segment AS_PATH UPDATE messages
Signed-off-by: Andreas Jaggi <aj at open.ch>
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 */
On Fri, Sep 08, 2017 at 07:53:36AM -0400, Donald Sharp wrote:
> 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
> >
> >
--
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