CID 1570969 Overrun
/bgpd/bgp_snmp_bgp4v2.c: 534 in bgp4v2PathAttrLookup()
/bgpd/bgp_snmp_bgp4v2.c: 575 in bgp4v2PathAttrLookup()
/bgpd/bgp_snmp_bgp4v2.c: 514 in bgp4v2PathAttrLookup()
>>> CID 1570969: (OVERRUN)
>>> Overrunning array "bgp->rib" of 4 64-byte elements at element index 4 (byte offset 319) using index "afi" (which evaluates to 4).
Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
Let's use the natural data structure in bgp for the prefix display
instead of a bunch of places where we call a translator function.
The %pBD does this and actually ensures data is correct.
Also fix a few spots in bgp_zebra.c where the cast to a NULL
pointer causes the catcher functionality to not work and fix
the resulting crash that resulted.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
I recieve the following error with GCC 9.4.0:
```
In file included from /usr/include/string.h:495,
from ./lib/zebra.h:23,
from bgpd/bgp_snmp_bgp4v2.c:7:
In function ‘memset’,
inlined from ‘bgp4v2PathAttrLookup’ at bgpd/bgp_snmp_bgp4v2.c:605:3,
inlined from ‘bgp4v2PathAttrTable’ at bgpd/bgp_snmp_bgp4v2.c:747:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:71:10: error: ‘__builtin_memset’ offset [9, 20] from the object at ‘paddr’ is out of the bounds of referenced subobject ‘_v4_addr’ with type ‘struct in_addr’ at offset 4 [-Werror=array-bounds]
71 | return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Add guard for `zlog_debug` when bgpd is not connected to zebra
or zebra does not know the bgp instance.
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
With a BGP configuration with ipv4 peering, and ipv6 peering, an snmpwalk
is stopped while walking over the bgp4v2NlriTable
snmpwalk -c TEST -v2c -On -Ln 1.1.1.2 .1.3.6.1.3.5.1.1.4
[...]
.1.3.6.1.3.5.1.1.4.1.2.1.2.32.0.0.0.0.0.0.0.0.0.0.0.0.0.0.1 = Gauge32: 13380
.1.3.6.1.3.5.1.1.9.1.1.1.1.1.1.1.0.24.0.0.0.0 = Gauge32: 0
.1.3.6.1.3.5.1.1.9.1.1.1.1.1.1.1.0.24.0.0.0.0 = Gauge32: 0
>= .1.3.6.1.3.5.1.1.9.1.1.1.1.1.1.1.0.24.0.0.0.0
The walk stopped because the index used in the NlriTable entries is
decrementing, and this is against the snmp specifications. Also, the
computed index is wrong, and does not match the provided
draft-ietf-idr-bgp4-mibv2-1 specification.
Fix this by computing a valid index, and by finding out the next
consecutive prefix.
The resulting changes do not break the walk, and the output is changed:
root@dut-vm:~# snmpwalk -v 2c -c public -Ln -On localhost 1.3.6.1.3.5.1.1.9.1
.1.3.6.1.3.5.1.1.9.1.1.1.1.1.1.10.200.0.0.24.1.10.125.0.2.1 = Gauge32: 0
.1.3.6.1.3.5.1.1.9.1.1.1.1.1.1.10.244.0.0.24.1.10.125.0.2.1 = Gauge32: 0
.1.3.6.1.3.5.1.1.9.1.2.1.1.1.1.10.200.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.2.1.1.1.1.10.244.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.3.1.1.1.1.10.200.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.3.1.1.1.1.10.244.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.4.1.1.1.1.10.200.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.4.1.1.1.1.10.244.0.0.24.1.10.125.0.2.1 = INTEGER: 1
.1.3.6.1.3.5.1.1.9.1.5.1.1.1.1.10.200.0.0.24.1.10.125.0.2.1 = Hex-STRING: 0A C8 00 00
.1.3.6.1.3.5.1.1.9.1.5.1.1.1.1.10.244.0.0.24.1.10.125.0.2.1 = Hex-STRING: 0A F4 00 00
Fixes: c681e937d7 (bgpd: Implement SNMP
BGP4V2-MIB (bgp4V2NlriTable), part 1)
Fixes: 2ce69011c4 (bgpd: Implement SNMP
BGP4V2-MIB (bgp4V2NlriTable), part 2)
Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
We send this capability for iBGP peers by default. Recently OAD support was
merged, and we should adopt sending the capability according to OAD as well.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
We shouldn't set it blindly once the packet is received, but first we have to
do some sanity checks.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
The headers associated with netlink code
really only belong in those that need it.
Move these headers out of lib/zebra.h and
into more appropriate places. bgp's usage
of the RT_TABLE_XXX defines are probably not
appropriate and will be cleaned up in future
commits.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
use CHECK_FLAG
fix comment spaces
change zlog_debug to zlog_warn
safeguard on updated_route
added doc/developer/bmp.rst to subdir.am
other qol changes
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
changed result type of bmp_get_peer_distinguisher to int
added result pointer parameter to bmp_get_peer_distinguisher
bmp_get_peer_distinguisher returns 0 means the result is valid else
error occured do not use result
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
moved loc-rib uptime field "bgp_rib_uptime" to struct bgp_path_info_extra for memory concerns
moved logic into bgp_route_update's callback bmp_route_update
written timestamp in per peer header
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
bgp_afi_node_lookup calls bgp_node_lookup which locks the node, unlocking it safely after function is finished
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
TODOs that are done/un-necessary now deleted
refactored bmp_route_update to use a modified bmp_process_one function call instead of duplicating similar code
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
cleaner implementation and use of the new get peer distinguisher function
can be now used for other cases of RFC7854 that are not supported atm
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
added time_t field to bgp_path_info
set value before bgp dp hook is called
value not set in the msg yet, testing and double checking is needed before
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
set timestamp to 0 for loc rib monitoring messages as path selection time is not available atm
this is temporary and tv is meant to be set to the path selection/install time at some point
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
vrf_id_to_name is used for display values only and returns "Unknown" when the vrf is not found
doing a manual lookup and not providing any tlv when the vrf is not found should be better
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
set field peer bgp id to the peer's remote id in every case except loc-rib (RFC9069 case) in which we put the bgp instance's router-id if available or 0-filled if not available
set field peer asn to local primary bgp asn in case of loc-rib instance (RFC9069) else it's set to the peer's asn
set field peer address to 0 in loc-rib instance (RFC9069 case) and to the peer's address in other cases
had to pass struct bgp reference to bmp_per_peer_hdr to access router-id and such, but it's always safely accessed when used
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
added afi/safi monitoring synchronisation for loc-rib
added peer_type_flag to bmp_eor signature, only set to BMP_PEER_TYPE_LOC_RIB and to 0 in other cases like it was before
updated tracelog to include peer_type_flag value
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
set peer distinguisher to vrf id temporarily until i find out how to use the rd set for export on the vrf instance associated to this bgp instance
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
set peer type flag to 3 for loc rib monitoring
leave to 0 in other cases like before, even though RFC7854 tells us to set it to 0 1 or 2 depending on the case global/rd/local instance
Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>
...so that multiple functions can be subscribed.
The create/destroy hooks are renamed to real/unreal because that's what
they *actually* signal.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
If we receive MP_UNREACH_NLRI, we should stop handling remaining NLRIs if
no mandatory path attributes received.
In other words, if MP_UNREACH_NLRI received, the remaining NLRIs should be handled
as a new data, but without mandatory attributes, it's a malformed packet.
In normal case, this MUST not happen at all, but to avoid crashing bgpd, we MUST
handle that.
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Treat-as-withdraw, otherwise if we just ignore it, we will pass it to be
processed as a normal UPDATE without mandatory attributes, that could lead
to harmful behavior. In this case, a crash for route-maps with the configuration
such as:
```
router bgp 65001
no bgp ebgp-requires-policy
neighbor 127.0.0.1 remote-as external
neighbor 127.0.0.1 passive
neighbor 127.0.0.1 ebgp-multihop
neighbor 127.0.0.1 disable-connected-check
neighbor 127.0.0.1 update-source 127.0.0.2
neighbor 127.0.0.1 timers 3 90
neighbor 127.0.0.1 timers connect 1
!
address-family ipv4 unicast
neighbor 127.0.0.1 addpath-tx-all-paths
neighbor 127.0.0.1 default-originate
neighbor 127.0.0.1 route-map RM_IN in
exit-address-family
exit
!
route-map RM_IN permit 10
set as-path prepend 200
exit
```
Send a malformed optional transitive attribute:
```
import socket
import time
OPEN = (b"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff"
b"\xff\xff\x00\x62\x01\x04\xfd\xea\x00\x5a\x0a\x00\x00\x01\x45\x02"
b"\x06\x01\x04\x00\x01\x00\x01\x02\x02\x02\x00\x02\x02\x46\x00\x02"
b"\x06\x41\x04\x00\x00\xfd\xea\x02\x02\x06\x00\x02\x06\x45\x04\x00"
b"\x01\x01\x03\x02\x0e\x49\x0c\x0a\x64\x6f\x6e\x61\x74\x61\x73\x2d"
b"\x70\x63\x00\x02\x04\x40\x02\x00\x78\x02\x09\x47\x07\x00\x01\x01"
b"\x80\x00\x00\x00")
KEEPALIVE = (b"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff"
b"\xff\xff\xff\xff\xff\xff\x00\x13\x04")
UPDATE = bytearray.fromhex("ffffffffffffffffffffffffffffffff002b0200000003c0ff00010100eb00ac100b0b001ad908ac100b0b")
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('127.0.0.2', 179))
s.send(OPEN)
data = s.recv(1024)
s.send(KEEPALIVE)
data = s.recv(1024)
s.send(UPDATE)
data = s.recv(1024)
time.sleep(100)
s.close()
```
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
It's been for a while disabled by default, but this seems reasonable to flip it.
We had `bgp enforce-first-as` as a global BGP knob to enable/disable this
behavior globally, later we introduced `enforce-first-as` per neighbor, with disabled
by default. Now let's enable this by default by bringing a global `bgp enforce-first-as`
command back.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
In zebra, the import check table and the nexthop check tables
were combined. This leaves an issue where when bgp happens
to have a tracked address in both the import check table
and the nexthop track table that are the same address.
When the the item is removed from one table the call
to remove it from zebra removes tracking for the other
table.
Combine the two tables together and keep track where
they came from for processing in bgpd.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
