From 8da04b5c6567907540a54d440aecc2b7d70e079b Mon Sep 17 00:00:00 2001 From: Eduardo Ramos Testillano Date: Sat, 21 Mar 2015 19:17:25 +0100 Subject: [PATCH] Fixed multiple AVP error. Missing fix RFC 6733 section 7.5 regarding avps within grouped ones. At the moment, only leaf wrong avp is stored in Failed-AVP. --- example/diameter/launcher/main.cpp | 4 +- .../ProtocolErrors/BadAARtoServer/case_1.tc | 2 +- .../ft-client/tests/experiment2/go.sh | 1 + include/anna/diameter/codec/Avp.hpp | 2 +- include/anna/diameter/codec/Message.hpp | 33 ++++---- source/diameter/codec/Avp.cpp | 32 +++++--- source/diameter/codec/Message.cpp | 82 +++++++++++++++++-- 7 files changed, 116 insertions(+), 40 deletions(-) diff --git a/example/diameter/launcher/main.cpp b/example/diameter/launcher/main.cpp index 280624c..fdb69d3 100644 --- a/example/diameter/launcher/main.cpp +++ b/example/diameter/launcher/main.cpp @@ -1466,8 +1466,8 @@ throw(anna::RuntimeException) { std::string address; int port; anna::functions::getAddressAndPortFromSocketLiteral(cl.getValue("httpServer"), address, port); - //const anna::comm::Device* device = network.find(Device::asAddress(address)); // aqui hay que proporcionar una IP ! - const anna::comm::Device* device = *((network.resolve(address)->device_begin())); // Artimaña para resolver (IP o hostname) + //const anna::comm::Device* device = network.find(Device::asAddress(address)); // here provide IP + const anna::comm::Device* device = *((network.resolve(address)->device_begin())); // trick to solve a_httpServerSocket = new anna::comm::ServerSocket(anna::comm::INetAddress(device, port), cl.exists("httpServerShared") /* shared bind */, &anna::http::Transport::getFactory()); } diff --git a/example/diameter/launcher/resources/ft-client/tests/experiment2/ProtocolErrors/BadAARtoServer/case_1.tc b/example/diameter/launcher/resources/ft-client/tests/experiment2/ProtocolErrors/BadAARtoServer/case_1.tc index 0007882..2fe63df 100644 --- a/example/diameter/launcher/resources/ft-client/tests/experiment2/ProtocolErrors/BadAARtoServer/case_1.tc +++ b/example/diameter/launcher/resources/ft-client/tests/experiment2/ProtocolErrors/BadAARtoServer/case_1.tc @@ -5,5 +5,5 @@ SENDHEX2E aar-bad.hex WAIT4MESSAGE # Check Failed-AVP & Subscription-Id within: -CHECKPATTERN +CHECKPATTERN CHECKPATTERN ( *)" +[ ! -f "$1" -a ! -d "$1" ] && _exit "Invalid file or directory '$1' !!" # Gather .tc files to be processed: [ ! -f $1 ] && echo -e "\nGathering list of test cases from '`readlink -f $1`' ..." diff --git a/include/anna/diameter/codec/Avp.hpp b/include/anna/diameter/codec/Avp.hpp index 1e5c529..bdf2227 100644 --- a/include/anna/diameter/codec/Avp.hpp +++ b/include/anna/diameter/codec/Avp.hpp @@ -727,7 +727,7 @@ public: Diameter Relay and redirect agents MUST NOT reject messages with unrecognized AVPs. Default implementation launch alarm and counter indicating the anomaly but don't launch exception (traces at warning level). - Realy and Redirect agents could reimplement this method to avoid oam management (another way is avoid alarm/counter registration on + Relay and Redirect agents could reimplement this method to avoid oam management (another way is avoid alarm/counter registration on these applications). Result-Code DIAMETER_AVP_UNSUPPORTED will be stored for possible answer message. @param answer Answer built for request decoding/validation diff --git a/include/anna/diameter/codec/Message.hpp b/include/anna/diameter/codec/Message.hpp index 5944c63..fb519b5 100644 --- a/include/anna/diameter/codec/Message.hpp +++ b/include/anna/diameter/codec/Message.hpp @@ -146,7 +146,9 @@ class Message { bool flagsOK(int &rc) const throw(); // flags coherence regarding dictionary. Only must be called when Message is identified at the dictionary. int addChild(Avp *avp) throw() { return Avp::addChild(a_avps, a_insertionPositionForChilds, avp); } const anna::diameter::stack::Command *getStackCommand(CommandId id) const throw(anna::RuntimeException); - Avp * addFailedAVP() throw(); // returns Failed-AVP if exists, creates it when missing + Avp * addTheFailedAVP() throw(); // returns the Failed-AVP if exists, creates it when missing. The method could be named 'addFailedAVP' + // but we consider this better because only one instance (as RFC 6733 says in section 7.5) will be + // added by internal procedures (although, the application could obviously add more). protected: @@ -406,32 +408,31 @@ public: /** - Adds a new AVP within a Failed-AVP over an answer message (for requests, do nothing). + Adds the wrong AVP within the Failed-AVP over an answer message (for requests, do nothing). If Failed-AVP AVP doesn't exists, is added and then filled (added within) with the value provided (empty AVP id representantion). - If Failed-AVP AVP already exists, is filled (added within) with the value provided (empty AVP id representantion). + If Failed-AVP AVP already exists, is probably filled by a previous found error, but anyway this is verified and if empty then is + filled (added within) with the value provided (empty AVP id representantion). - This method is internally used during #decode and/or #valid procedures in order to build automatic answers. + This method is internally used during #decode and/or #valid procedures in order to build automatic answers, but application + could call this for set another Failed-AVP content no detected by these methods, for example: DIAMETER_CONTRADICTING_AVPS or + DIAMETER_INVALID_AVP_BIT_COMBO). Also, application could add more Failed-AVP avps with other wrong avps, or accumulate wrong + avps inside the one and only Failed-AVP managed by the stack (see section 7.5 of RFC 6733). @param id Avp identifier as pair (code,vendor-id). - @return Pointer to the new AVP added within Failed-AVP, to make easy data-part accessif needed. + @return Pointer to the new AVP added within Failed-AVP, to make easy data-part access if needed. */ - Avp * setNewFailedAvp(AvpId id) throw(anna::RuntimeException) { if(isRequest()) return NULL; return (addFailedAVP()->addAvp(id)); } + Avp * setFailedAvp(AvpId id) throw(anna::RuntimeException); /** - Adds a new AVP within a Failed-AVP over an answer message (for requests, do nothing). - If Failed-AVP AVP doesn't exists, is added and then filled (added within) with the value provided (empty AVP id representantion). - If Failed-AVP AVP already exists, is filled (added within) with the value provided (empty AVP id representantion). - - This method is internally used during #decode and/or #valid procedures in order to build automatic answers, but application - could call this for set another Failed-AVP content no detected by these methods, for example: DIAMETER_CONTRADICTING_AVPS or - DIAMETER_INVALID_AVP_BIT_COMBO). + Same as #setFailedAvp(AvpId id) but providing an avp pointer with the needed information - @param id Avp identifier as pair (code,vendor-id). + @param avp Pointer to the added wrong avp - @return Pointer to the new AVP added within Failed-AVP, to make easy data-part accessif needed. + @return Pointer to the new AVP added within Failed-AVP, to make easy data-part access if needed. */ - Avp * setNewFailedAvp(Avp *avp) throw() { if(!avp || isRequest()) return NULL; return (addFailedAVP()->addAvp(avp)); } + Avp * setFailedAvp(Avp *avp) throw(anna::RuntimeException); + /** diff --git a/source/diameter/codec/Avp.cpp b/source/diameter/codec/Avp.cpp index 19a0780..7f33851 100644 --- a/source/diameter/codec/Avp.cpp +++ b/source/diameter/codec/Avp.cpp @@ -644,7 +644,7 @@ void Avp::unknownAvpWithMandatoryBit(Message *answer) const throw(anna::RuntimeE if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_AVP_UNSUPPORTED); - answer->setNewFailedAvp((Avp*)this); + answer->setFailedAvp((Avp*)this); } } @@ -679,6 +679,12 @@ void Avp::decodeDataPart(const char * buffer, int size, Message *answer) throw(a if((size % 4) != 0) { oamModule.activateAlarm(OamModule::Alarm::AvpDecode__IncorrectLength); oamModule.count(OamModule::Counter::AvpDecode__IncorrectLength); + + if(answer) { + answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_LENGTH); + //answer->setFailedAvp(a_id); + } + throw anna::RuntimeException("Avp format error, the avp length is incorrect (must be multiple of 4 on grouped type)", ANNA_FILE_LOCATION); } @@ -724,7 +730,7 @@ void Avp::decode(const anna::DataBlock &db, Message *answer) throw(anna::Runtime if(db.getSize() < HeaderLengthVinactive) { oamModule.activateAlarm(OamModule::Alarm::AvpDecode__NotEnoughBytesToCoverAvpHeaderLength); oamModule.count(OamModule::Counter::AvpDecode__NotEnoughBytesToCoverAvpHeaderLength); - // DIAMETER_INVALID_AVP_LENGTH; // no podré construir un avp fiable, así que no registro el result-code + // DIAMETER_INVALID_AVP_LENGTH; // no podr� construir un avp fiable, as� que no registro el result-code throw anna::RuntimeException("Not enough bytes to cover avp header length", ANNA_FILE_LOCATION); } @@ -752,7 +758,7 @@ void Avp::decode(const anna::DataBlock &db, Message *answer) throw(anna::Runtime if(db.getSize() < HeaderLengthVactive) { oamModule.activateAlarm(OamModule::Alarm::AvpDecode__NotEnoughBytesToCoverAvpHeaderLength); oamModule.count(OamModule::Counter::AvpDecode__NotEnoughBytesToCoverAvpHeaderLength); - // DIAMETER_INVALID_AVP_LENGTH; // no podré construir un avp fiable, así que no registro el result-code + // DIAMETER_INVALID_AVP_LENGTH; // no podr� construir un avp fiable, as� que no registro el result-code throw anna::RuntimeException(anna::functions::asString("Not enough bytes to cover avp header length (avp code = %u)", code), ANNA_FILE_LOCATION); } @@ -794,7 +800,7 @@ void Avp::decode(const anna::DataBlock &db, Message *answer) throw(anna::Runtime if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_LENGTH); - answer->setNewFailedAvp(a_id); + //answer->setFailedAvp(a_id); } throw anna::RuntimeException(anna::functions::asString("Avp format error, the avp length is incorrect (avp code = %u)", code), ANNA_FILE_LOCATION); @@ -808,7 +814,7 @@ void Avp::decode(const anna::DataBlock &db, Message *answer) throw(anna::Runtime if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_VALUE); // unspecified error ... - answer->setNewFailedAvp((Avp*)this); + //answer->setFailedAvp((Avp*)this); } throw anna::RuntimeException(anna::functions::asString("Internal Avp decoding error (avp code = %u): %s", code, ex.getText().c_str()), ANNA_FILE_LOCATION); @@ -885,7 +891,7 @@ void Avp::fix() throw() { } if(!stackFormat || !stackFormat->isGrouped()) { - //LOGDEBUG(anna::Logger::debug("Avp is not grouped. Nothing fix.", ANNA_FILE_LOCATION)); + //LOGDEBUG(anna::Logger::debug("Avp is not grouped. Nothing to fix.", ANNA_FILE_LOCATION)); return; } @@ -939,7 +945,7 @@ bool Avp::validLevel(const avp_container &avps, anna::diameter::stack::const_avp if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_MISSING_AVP); - answer->setNewFailedAvp((*rule_it).second.getId()); + answer->setFailedAvp((*rule_it).second.getId()); } engine->validationAnomaly(anna::functions::asString("Missing fixed rule %s inside %s", STRING_WITH_QUOTATION_MARKS__C_STR((*rule_it).second.asString(false /*ommit dots & pair*/)), STRING_WITH_QUOTATION_MARKS__C_STR(parentDescription))); @@ -973,14 +979,14 @@ bool Avp::validLevel(const avp_container &avps, anna::diameter::stack::const_avp if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_MISSING_AVP); - answer->setNewFailedAvp(id); + answer->setFailedAvp(id); } } else { oamModule.count(OamModule::Counter::LevelValidation__FailedRuleForCardinalityMoreThanNeeded); if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_AVP_OCCURS_TOO_MANY_TIMES); - answer->setNewFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); + answer->setFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); } } @@ -1030,7 +1036,7 @@ bool Avp::validLevel(const avp_container &avps, anna::diameter::stack::const_avp if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_AVP_NOT_ALLOWED); - //answer->setNewFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); // NO SENSE... what to put ? + //answer->setFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); // NO SENSE... what to put ? } engine->validationAnomaly(anna::functions::asString("Failed Generic AVP rule %s for cardinality (found %d disregarded items inside %s)", STRING_WITH_QUOTATION_MARKS__C_STR((*generic_rule_it).second.asString(false /*ommit dots & pair*/)), amount, STRING_WITH_QUOTATION_MARKS__C_STR(parentDescription))); @@ -1051,7 +1057,7 @@ bool Avp::validLevel(const avp_container &avps, anna::diameter::stack::const_avp // We wouldn't know where are these disregarded, but... if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_AVP_NOT_ALLOWED); - answer->setNewFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); + answer->setFailedAvp((Avp*)firstAvp(avps, id) /* first instance */); } } @@ -1101,7 +1107,7 @@ bool Avp::valid(const std::string & parentDescription, Message *answer) const th if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_BITS); - //answer->setNewFailedAvp((Avp*)this); RFC 6733 says nothing about Failed-AVP in this case... + answer->setFailedAvp((Avp*)this); // RFC 6733 says nothing about Failed-AVP in this case... } getEngine()->validationAnomaly(anna::functions::asString("The AVP %s flags (%d) does not fulfill the defined flag rules: %s", STRING_WITH_QUOTATION_MARKS__C_STR(me), (int)a_flags, STRING_WITH_QUOTATION_MARKS__C_STR(stackAvp->getFlagRulesDescription()))); @@ -1119,7 +1125,7 @@ bool Avp::valid(const std::string & parentDescription, Message *answer) const th if(answer) { answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_VALUE); - answer->setNewFailedAvp((Avp*)this); + answer->setFailedAvp((Avp*)this); } getEngine()->validationAnomaly(anna::functions::asString("Enumerated AVP %s with value %d does not comply to restriction: %s", STRING_WITH_QUOTATION_MARKS__C_STR(me), a_Enumerated->getValue(), stackAvp->getEnums())); diff --git a/source/diameter/codec/Message.cpp b/source/diameter/codec/Message.cpp index 9deaf2e..7fc92e8 100644 --- a/source/diameter/codec/Message.cpp +++ b/source/diameter/codec/Message.cpp @@ -300,7 +300,7 @@ void Message::decode(const anna::DataBlock &db, Message *ptrAnswer) throw(anna:: if(db.getSize() < HeaderLength) { oamModule.activateAlarm(OamModule::Alarm::MessageDecode__NotEnoughBytesToCoverMessageHeaderLength); oamModule.count(OamModule::Counter::MessageDecode__NotEnoughBytesToCoverMessageHeaderLength); - // DIAMETER_INVALID_MESSAGE_LENGTH; // no podré construir un answer fiable, así que no registro el result-code + // DIAMETER_INVALID_MESSAGE_LENGTH; // no podr� construir un answer fiable, as� que no registro el result-code throw anna::RuntimeException("Not enough bytes to cover message header length (20 bytes)", ANNA_FILE_LOCATION); } @@ -376,10 +376,9 @@ void Message::decode(const anna::DataBlock &db, Message *ptrAnswer) throw(anna:: avp -> decode(db_aux, answer); } catch(anna::RuntimeException &ex) { if(answer) { - answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_VALUE); // unspecified error ... - answer->setNewFailedAvp(avp->getId()); + //answer->setResultCode(helpers::base::AVPVALUES__Result_Code::DIAMETER_INVALID_AVP_VALUE); // unspecified error ... + answer->setFailedAvp(avp->getId()); } - getEngine()->releaseAvp(avp); LOGWARNING( anna::Logger::warning(ex.getText(), ANNA_FILE_LOCATION); @@ -457,17 +456,85 @@ int Message::getResultCode() const throw() { //------------------------------------------------------------------------------ -//------------------------------------------------------ Message::addFailedAVP() +//--------------------------------------------------- Message::addTheFailedAVP() //------------------------------------------------------------------------------ -Avp * Message::addFailedAVP() throw() { +Avp * Message::addTheFailedAVP() throw() { Avp *result = getAvp(helpers::base::AVPID__Failed_AVP, 1, anna::Exception::Mode::Ignore); + // Section 7.5 RFC 6733: A Diameter message SHOULD contain one Failed-AVP AVP if(!result) result = addAvp(helpers::base::AVPID__Failed_AVP); return result; } +//------------------------------------------------------------------------------ +//------------------------------------------------------ Message::setFailedAvp() +//------------------------------------------------------------------------------ +Avp * Message::setFailedAvp(AvpId id) throw(anna::RuntimeException) { + + if(isRequest()) return NULL; + +// RFC 6733: +// +// 7.5. Failed-AVP AVP +// +// The Failed-AVP AVP (AVP Code 279) is of type Grouped and provides +// debugging information in cases where a request is rejected or not +// fully processed due to erroneous information in a specific AVP. The +// value of the Result-Code AVP will provide information on the reason +// for the Failed-AVP AVP. A Diameter answer message SHOULD contain an +// instance of the Failed-AVP AVP that corresponds to the error +// indicated by the Result-Code AVP. For practical purposes, this +// Failed-AVP would typically refer to the first AVP processing error +// that a Diameter node encounters. + + // Although the Failed-AVP definition has cardinality 1* and Failed-AVP itself is defined in + // most of the command codes as *[Failed-AVP], i think this is not a deliberate ambiguity. + // Probably the RFC wants to give freedom to the application layer, but it is recommended to + // have only one child (wrong avp) inside a unique message Failed-AVP to ease the Result-Code + // correspondence. Anyway, this behaviour could be easily opened commenting condition block (*). + Avp *theFailedAvp = addTheFailedAVP(); + LOGDEBUG( + std::string msg = "Adding wrong avp "; + msg += anna::diameter::functions::avpIdAsPairString(id); + msg += " within the message Failed-AVP ..."; + anna::Logger::debug(msg, ANNA_FILE_LOCATION); + ); + + if (theFailedAvp->countChilds()) { // (*) + LOGDEBUG(anna::Logger::debug("Discarding wrong avp. A previous wrong avp was already added into the Failed-AVP. RFC 6733 Section 7.5 recommends to store only the first.", ANNA_FILE_LOCATION)); + return NULL; + } + + return (theFailedAvp->addAvp(id)); +} + + +//------------------------------------------------------------------------------ +//------------------------------------------------------ Message::setFailedAvp() +//------------------------------------------------------------------------------ +Avp * Message::setFailedAvp(Avp *avp) throw(anna::RuntimeException) { + + if(!avp || isRequest()) return NULL; + + Avp *theFailedAvp = addTheFailedAVP(); + LOGDEBUG( + std::string msg = "Adding wrong avp "; + msg += anna::diameter::functions::avpIdAsPairString(avp->getId()); + msg += " within the message Failed-AVP ..."; + anna::Logger::debug(msg, ANNA_FILE_LOCATION); + ); + + if (theFailedAvp->countChilds()) { // (*) + LOGDEBUG(anna::Logger::debug("Discarding wrong avp. A previous wrong avp was already added into the Failed-AVP. RFC 6733 Section 7.5 recommends to store only the first.", ANNA_FILE_LOCATION)); + return NULL; + } + + return (theFailedAvp->addAvp(avp)); +} + + //------------------------------------------------------------------------------ //----------------------------------------------- Message::setStandardToAnswer() //------------------------------------------------------------------------------ @@ -480,7 +547,8 @@ void Message::setStandardToAnswer(const Message &request, const std::string &ori const Avp *reqSessionId = request.getAvp(helpers::base::AVPID__Session_Id, 1, anna::Exception::Mode::Ignore); if(reqSessionId) - addAvp(helpers::base::AVPID__Session_Id)->getUTF8String()->setValue(reqSessionId->getUTF8String()->getValue()); + if(!getAvp(helpers::base::AVPID__Session_Id, 1, anna::Exception::Mode::Ignore)) + addAvp(helpers::base::AVPID__Session_Id)->getUTF8String()->setValue(reqSessionId->getUTF8String()->getValue()); // Origin-Host & Realm if(!getAvp(helpers::base::AVPID__Origin_Host, 1, anna::Exception::Mode::Ignore)) -- 2.20.1