fix UAF in VanetNic

[hagau]

While checking the MCO implementation with valgrind we found a UAF:

==8623== Invalid read of size 8
==8623==    at 0x127C613D: InetMobility::getConstraintAreaMax() const (InetMobility.cc:68)
==8623==    by 0x16F12F5A: inet::physicallayer::MediumLimitCache::computeMaxConstreaintArea() const (MediumLimitCache.cc:238)
==8623==    by 0x16F120D3: inet::physicallayer::MediumLimitCache::updateLimits() (MediumLimitCache.cc:118)
==8623==    by 0x16F123FA: inet::physicallayer::MediumLimitCache::removeRadio(inet::physicallayer::IRadio const*) (MediumLimitCache.cc:139)
==8623==    by 0x16F268E1: inet::physicallayer::RadioMedium::removeRadio(inet::physicallayer::IRadio const*) (RadioMedium.cc:456)
==8623==    by 0x16F1622B: inet::physicallayer::Radio::~Radio() (Radio.cc:44)
==8623==    by 0x16EDAC4B: inet::physicallayer::NarrowbandRadioBase::~NarrowbandRadioBase() (NarrowbandRadioBase.h:28)
==8623==    by 0x16EDABCB: inet::physicallayer::FlatRadioBase::~FlatRadioBase() (FlatRadioBase.h:27)
==8623==    by 0x16FA084B: inet::physicallayer::Ieee80211Radio::~Ieee80211Radio() (Ieee80211Radio.h:33)
==8623==    by 0x16FA06CF: inet::physicallayer::Ieee80211Radio::~Ieee80211Radio() (Ieee80211Radio.h:33)
==8623==    by 0x16FA0708: inet::physicallayer::Ieee80211Radio::~Ieee80211Radio() (Ieee80211Radio.h:33)
==8623==    by 0x5F4E079: omnetpp::cModule::deleteModule() (cmodule.cc:1209)
==8623==  Address 0x1adf5738 is 104 bytes inside a block of size 448 free'd
==8623==    at 0x4C2E5DB: operator delete(void*) (vg_replace_malloc.c:576)
==8623==    by 0x127C6F01: InetMobility::~InetMobility() (InetMobility.h:11)
==8623==    by 0x5F4E079: omnetpp::cModule::deleteModule() (cmodule.cc:1209)
==8623==    by 0x5F881E3: omnetpp::cSimpleModule::deleteModule() (csimplemodule.cc:326)
==8623==    by 0x5F48107: omnetpp::cModule::~cModule() (cmodule.cc:106)
==8623==    by 0x5F486E8: omnetpp::cModule::~cModule() (cmodule.cc:82)
==8623==    by 0x5F4E079: omnetpp::cModule::deleteModule() (cmodule.cc:1209)
==8623==    by 0x12F6268C: traci::BasicNodeManager::removeNodeModule(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (BasicNodeManager.cc:156)
==8623==    by 0x12F621A2: traci::BasicNodeManager::removeVehicle(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (BasicNodeManager.cc:111)
==8623==    by 0x12F61C8A: traci::BasicNodeManager::traciStep() (BasicNodeManager.cc:74)
==8623==    by 0x12F782CE: traci::Listener::receiveSignal(omnetpp::cComponent*, int, omnetpp::SimTime const&, omnetpp::cObject*) (Listener.cc:45)
==8623==    by 0x5EC834B: void omnetpp::cComponent::fire<omnetpp::SimTime>(omnetpp::cComponent*, int, omnetpp::SimTime, omnetpp::cObject*) (ccomponent.cc:629)
==8623==  Block was alloc'd at
==8623==    at 0x4C2D51F: operator new(unsigned long) (vg_replace_malloc.c:334)
==8623==    by 0x127C66C3: __factoryfunc_14() (InetMobility.cc:14)
==8623==    by 0x5EC2738: omnetpp::cObjectFactory::createOne() const (cobjectfactory.cc:60)
==8623==    by 0x5EC2781: omnetpp::cObjectFactory::createOne(char const*) (cobjectfactory.cc:66)
==8623==    by 0x5ED6393: omnetpp::cModuleType::instantiateModuleClass(char const*) (ccomponenttype.cc:357)
==8623==    by 0x6085F2F: omnetpp::cDynamicModuleType::createModuleObject() (cdynamicmoduletype.cc:66)
==8623==    by 0x5ED5E0B: omnetpp::cModuleType::create(char const*, omnetpp::cModule*, int, int) (ccomponenttype.cc:293)
==8623==    by 0x5ED5BEC: omnetpp::cModuleType::create(char const*, omnetpp::cModule*) (ccomponenttype.cc:265)
==8623==    by 0x6089EC5: omnetpp::cNedNetworkBuilder::addSubmodule(omnetpp::cModule*, omnetpp::nedxml::SubmoduleElement*) (cnednetworkbuilder.cc:763)
==8623==    by 0x608991E: omnetpp::cNedNetworkBuilder::addSubmodulesAndConnections(omnetpp::cModule*) (cnednetworkbuilder.cc:474)
==8623==    by 0x608973B: omnetpp::cNedNetworkBuilder::buildRecursively(omnetpp::cModule*, omnetpp::cNedDeclaration*) (cnednetworkbuilder.cc:466)
==8623==    by 0x6089724: omnetpp::cNedNetworkBuilder::buildRecursively(omnetpp::cModule*, omnetpp::cNedDeclaration*) (cnednetworkbuilder.cc:462)

I'm having trouble reproducing this with the master branch right now, but stepping through with gdb shows the destructor for InetMobility being called before Ieee80211Radio's destructor.

OMNet++ initializes the submodules of the module's super type first and
adds them to the list of the module's submodules (sim/netbuilder/cnednetworkbuilder.cc:462),
then does the same for the module's submodules (sim/netbuilder/cnednetworkbuilder.cc:466).

OMNet++ destroys submodules in the order they are listed in this list (sim/cmodule.cc:106), thus destroying the super types' submodules first.

This leads to a UAF if a submodule needs to access the mobility module in the destructor, like VanetNic's radio submodule.

To fix this, add the mobility module last, leading to it getting destroyed last.

[riebl]

I am not convinced that moving mobility module is a proper fix for this issue: When MediumLimitCache::updateMaxConstreaintArea() loops over all (remaining) radios, the radio of the calling destructor should already be removed from the iterated vector. Thus, I assume InetMobility::getConstraintAreaMax() is not called for the previously destructed InetMobility object at all.

Moving mobility may just incidentally hide the actual bug. Since you cannot reproduce this issue on master branch, I hesitate to merge this PR.

[hagau]

Sorry, I forgot to link to the discussion on the OMNet++ mailing list.

You are absolutely right, it's not the removed radio that accesses the already destructed mobility, but the second, still existing, radio does upon removal of the first. (My mistake, someone on the mailing list corrected me)

As to the reproducibility, I was unable to get valgrind to find the UAF in the master branch because there is none when there's only one radio per mobility.
So for now, this patch is not necessary for master, but once MCO has landed, it will be. That, or OMNet++ reverses destruction order 😁.

I'd suggest postponing dealing with this issue for now until MCO is ready for merge.

[riebl]

@hagau Thanks for pointing to the related mailing list discussion. It seems everything we can do in Artery is in fact just a workaround. Moving modules to craft a particular destruction order is prone to fail again in the future. I hope we can solve this upstream, i.e. in INET/OMNeT++.

[hagau]

@riebl Yeah, I don't like this fix either, since I'm relying on undocumented internal behaviour of OMNeT++ :).

[riebl]

Merged as per ebfdf32.