Re: [dev] [PATCH] bgpd: fix length calculation of multi-segment AS_PATH UPDATE messages
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@lists.frrouting.org> wrote:
_______________________________________________ dev mailing list dev@lists.frrouting.org https://lists.frrouting.org/listinfo/dev
---------- Forwarded message ---------- From: Andreas Jaggi <aj@open.ch> To: quagga-dev@lists.quagga.net, dev@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@open.ch
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@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@lists.frrouting.org> wrote:
_______________________________________________ dev mailing list dev@lists.frrouting.org https://lists.frrouting.org/listinfo/dev
---------- Forwarded message ---------- From: Andreas Jaggi <aj@open.ch> To: quagga-dev@lists.quagga.net, dev@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@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@open.ch http://www.open.ch
participants (2)
-
Andreas Jaggi -
Donald Sharp