Merge pull request #5919 from qlyoung/fix-vrrp-mvl-uaf

vrrpd: Fix heap uaf when handling interface deletions
This commit is contained in:
Donatas Abraitis 2020-03-09 08:03:34 +02:00 committed by GitHub
commit 4c2a712d93
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -185,6 +185,11 @@ static bool vrrp_ifp_has_vrrp_mac(struct interface *ifp)
* is used to look up any existing instances that match the interface. It does
* not matter whether the instance is already bound to the interface or not.
*
* Note that the interface linkages must be correct for this to work. In other
* words, the macvlan must have a valid VRRP MAC, and its link_ifindex must be
* be equal to the ifindex of another interface in the interface RB trees (its
* parent). If these conditions aren't satisfied we won't find the VR.
*
* mvl_ifp
* Interface pointer to use to lookup. Should be a macvlan device.
*
@ -1646,7 +1651,7 @@ static int vrrp_shutdown(struct vrrp_router *r)
r->vr->vrid, family2str(r->family),
vrrp_event_names[VRRP_EVENT_SHUTDOWN],
vrrp_state_names[VRRP_STATE_INITIALIZE]);
break;
return 0;
}
/* Cancel all timers */
@ -2214,18 +2219,57 @@ void vrrp_if_del(struct interface *ifp)
{
struct listnode *ln;
struct vrrp_vrouter *vr;
struct list *vrs = vrrp_lookup_by_if_any(ifp);
vrrp_if_down(ifp);
/*
* You think we'd be able use vrrp_lookup_by_if_any to find interfaces?
* Nah. FRR's interface management is insane. There are no ordering
* guarantees about what interfaces are deleted when. Maybe this is a
* macvlan and its parent was already deleted, in which case its
* ifindex is now IFINDEX_INTERNAL, so ifp->link_ifindex - while still
* valid - doesn't match any interface on the system, meaning we can't
* use any of the vrrp_lookup* functions since they rely on finding the
* base interface of what they're given by following link_ifindex.
*
* Since we need to actually NULL out pointers in this function to
* avoid a UAF - since the caller will (might) free ifp after we return
* - we need to look up based on pointers.
*/
struct list *vrs = hash_to_list(vrrp_vrouters_hash);
for (ALL_LIST_ELEMENTS_RO(vrs, ln, vr)) {
if ((vr->v4->mvl_ifp == ifp || vr->ifp == ifp)
&& vr->v4->fsm.state != VRRP_STATE_INITIALIZE) {
if (ifp == vr->ifp) {
vrrp_event(vr->v4, VRRP_EVENT_SHUTDOWN);
vr->v4->mvl_ifp = NULL;
} else if ((vr->v6->mvl_ifp == ifp || vr->ifp == ifp)
&& vr->v6->fsm.state != VRRP_STATE_INITIALIZE) {
vrrp_event(vr->v6, VRRP_EVENT_SHUTDOWN);
/*
* Stands to reason if the base was deleted, so were
* (or will be) its children
*/
vr->v4->mvl_ifp = NULL;
vr->v6->mvl_ifp = NULL;
/*
* We shouldn't need to lose the reference if it's the
* primary interface, because that was configured
* explicitly in our config, and thus will be kept as a
* stub; to avoid stupid bugs, double check that
*/
assert(ifp->configured);
} else if (ifp == vr->v4->mvl_ifp) {
vrrp_event(vr->v4, VRRP_EVENT_SHUTDOWN);
/*
* If this is a macvlan, then it wasn't explicitly
* configured and will be deleted when we return from
* this function, so we need to lose the reference
*/
vr->v4->mvl_ifp = NULL;
} else if (ifp == vr->v6->mvl_ifp) {
vrrp_event(vr->v6, VRRP_EVENT_SHUTDOWN);
/*
* If this is a macvlan, then it wasn't explicitly
* configured and will be deleted when we return from
* this function, so we need to lose the reference
*/
vr->v6->mvl_ifp = NULL;
}
}