From e8645dc5ade3f6dfbe31ca999526764ab31cb3db Mon Sep 17 00:00:00 2001 From: Nanosonde <2073569+nanosonde@users.noreply.github.com> Date: Sun, 5 Jul 2020 16:18:59 +0200 Subject: [PATCH] Further cleanup. --- src/knx/bau_systemB.cpp | 26 ++++++------- src/knx/secure_application_layer.cpp | 18 ++------- src/knx/secure_application_layer.h | 4 -- src/knx/security_interface_object.cpp | 53 +++++++++++++++------------ src/knx/security_interface_object.h | 12 ++++-- 5 files changed, 56 insertions(+), 57 deletions(-) diff --git a/src/knx/bau_systemB.cpp b/src/knx/bau_systemB.cpp index 7a99b10..32f1965 100644 --- a/src/knx/bau_systemB.cpp +++ b/src/knx/bau_systemB.cpp @@ -83,25 +83,23 @@ void BauSystemB::sendNextGroupTelegram() continue; SecurityControl goSecurity; - goSecurity.toolAccess = false; + goSecurity.toolAccess = false; // Secured group communication never uses the toolkey. ETS knows all keys, also the group keys. + +#ifdef USE_DATASECURE + // Get security flags from Security Interface Object for this group object + goSecurity.dataSecurity = _secIfObj.getGroupObjectSecurity(asap); +#else goSecurity.dataSecurity = DataSecurity::none; +#endif if (flag == WriteRequest && go.transmitEnable()) { -#ifdef USE_DATASECURE - // Get security flags from Security Interface Object for this group object - goSecurity.dataSecurity = _secIfObj.getGroupObjectSecurity(asap, true); -#endif uint8_t* data = go.valueRef(); _appLayer.groupValueWriteRequest(AckRequested, asap, go.priority(), NetworkLayerParameter, goSecurity, data, go.sizeInTelegram()); } else if (flag == ReadRequest) { -#ifdef USE_DATASECURE - // Get security flags from Security Interface Object for this group object - goSecurity.dataSecurity = _secIfObj.getGroupObjectSecurity(asap, false); -#endif _appLayer.groupValueReadRequest(AckRequested, asap, go.priority(), NetworkLayerParameter, goSecurity); } @@ -215,8 +213,10 @@ uint8_t BauSystemB::masterReset(EraseCode eraseCode, uint8_t channel) println(eraseCode == EraseCode::FactoryReset ? "FactoryReset with IA" : "FactoryReset without IA"); #ifdef USE_DATASECURE // If erase code is FactoryReset or FactoryResetWithoutIA, set FDSK as toolkey again - // and disable security mode - _secIfObj.factoryReset(); + // and disable security mode. + // FIXME: the A_RestartResponse PDU has still to be sent with the current toolkey. + // Idea: use local confirmation of sent A_RestartResponse PDU to trigger writing the FDSK afterwards + _secIfObj.masterReset(eraseCode); #endif return successCode; } @@ -609,7 +609,7 @@ void BauSystemB::groupValueReadIndication(uint16_t asap, Priority priority, HopC DataSecurity requiredGoSecurity; // Get security flags from Security Interface Object for this group object - requiredGoSecurity = _secIfObj.getGroupObjectSecurity(asap, false); + requiredGoSecurity = _secIfObj.getGroupObjectSecurity(asap); if (secCtrl.dataSecurity != requiredGoSecurity) { @@ -644,7 +644,7 @@ void BauSystemB::groupValueWriteIndication(uint16_t asap, Priority priority, Hop DataSecurity requiredGoSecurity; // Get security flags from Security Interface Object for this group object - requiredGoSecurity = _secIfObj.getGroupObjectSecurity(asap, true); + requiredGoSecurity = _secIfObj.getGroupObjectSecurity(asap); if (secCtrl.dataSecurity != requiredGoSecurity) { diff --git a/src/knx/secure_application_layer.cpp b/src/knx/secure_application_layer.cpp index 1b8a920..cb2e3e2 100644 --- a/src/knx/secure_application_layer.cpp +++ b/src/knx/secure_application_layer.cpp @@ -782,8 +782,8 @@ bool SecureApplicationLayer::decrypt(uint8_t* plainApdu, uint16_t plainApduLengt bool syncReq = service == kSecureSyncRequest; bool syncRes = service == kSecureSyncResponse; - //const uint8_t* key = dstAddrIsGroupAddr ? securityKey(dstAddr, dstAddrIsGroupAddr) : toolAccess ? toolKey(srcAddr == _deviceObj.induvidualAddress() ? dstAddr : srcAddr) : securityKey(srcAddr, false); - const uint8_t* key = dstAddrIsGroupAddr && (dstAddr != 0) ? securityKey(dstAddr, dstAddrIsGroupAddr) : toolAccess ? _secIfObj.toolKey(srcAddr == _deviceObj.induvidualAddress() ? dstAddr : srcAddr) : securityKey(srcAddr, false); + //const uint8_t* key = dstAddrIsGroupAddr ? securityKey(dstAddr, dstAddrIsGroupAddr) : toolAccess ? toolKey() : securityKey(srcAddr, false); + const uint8_t* key = dstAddrIsGroupAddr && (dstAddr != 0) ? securityKey(dstAddr, dstAddrIsGroupAddr) : toolAccess ? _secIfObj.toolKey() : securityKey(srcAddr, false); if (key == nullptr) { print("Error: No key found. toolAccess: "); @@ -1047,7 +1047,7 @@ bool SecureApplicationLayer::secure(uint8_t* buffer, uint16_t service, uint16_t } } - const uint8_t* key = toolAccess ? _secIfObj.toolKey(_syncReqBroadcastIncoming ? _deviceObj.induvidualAddress() : dstAddr) : securityKey(dstAddr, dstAddrIsGroupAddr); + const uint8_t* key = toolAccess ? _secIfObj.toolKey() : securityKey(dstAddr, dstAddrIsGroupAddr); if (key == nullptr) { print("Error: No key found. toolAccess: "); @@ -1231,16 +1231,6 @@ bool SecureApplicationLayer::createSecureApdu(APDU& plainApdu, APDU& secureApdu, return false; } -void SecureApplicationLayer::setSecurityMode(bool enabled) -{ - _securityModeEnabled = enabled; -} - -bool SecureApplicationLayer::isSecurityModeEnabled() -{ - return _securityModeEnabled; -} - void SecureApplicationLayer::clearFailureLog() { println("clearFailureLog()"); @@ -1282,4 +1272,4 @@ bool SecureApplicationLayer::isSyncService(APDU& secureApdu) return false; } -#endif \ No newline at end of file +#endif diff --git a/src/knx/secure_application_layer.h b/src/knx/secure_application_layer.h index 60684eb..546b2f1 100644 --- a/src/knx/secure_application_layer.h +++ b/src/knx/secure_application_layer.h @@ -30,8 +30,6 @@ class SecureApplicationLayer : public ApplicationLayer */ SecureApplicationLayer(DeviceObject& deviceObj, SecurityInterfaceObject& secIfObj, AssociationTableObject& assocTable, AddressTableObject& addrTab, BusAccessUnit& bau); - void setSecurityMode(bool enabled); - bool isSecurityModeEnabled(); void clearFailureLog(); void getFailureCounters(uint8_t* data); uint8_t getFromFailureLogByIndex(uint8_t index, uint8_t* data, uint8_t maxDataLen); @@ -137,8 +135,6 @@ class SecureApplicationLayer : public ApplicationLayer void encryptAesCbc(uint8_t* buffer, uint16_t bufLen, const uint8_t* iv, const uint8_t* key); void xcryptAesCtr(uint8_t* buffer, uint16_t bufLen, const uint8_t* iv, const uint8_t* key); - bool _securityModeEnabled {false}; - bool _syncReqBroadcastIncoming{false}; bool _syncReqBroadcastOutgoing{false}; uint32_t _lastSyncRes; diff --git a/src/knx/security_interface_object.cpp b/src/knx/security_interface_object.cpp index 1e65850..b3f85ce 100644 --- a/src/knx/security_interface_object.cpp +++ b/src/knx/security_interface_object.cpp @@ -52,7 +52,7 @@ SecurityInterfaceObject::SecurityInterfaceObject() resultLength = 1; return; } - obj->_secAppLayer->setSecurityMode(mode == 1); + obj->setSecurityMode(mode == 1); resultData[0] = ReturnCodes::Success; resultData[1] = serviceId; resultLength = 2; @@ -74,7 +74,7 @@ SecurityInterfaceObject::SecurityInterfaceObject() { resultData[0] = ReturnCodes::Success; resultData[1] = serviceId; - resultData[2] = obj->_secAppLayer->isSecurityModeEnabled() ? 1 : 0; + resultData[2] = obj->isSecurityModeEnabled() ? 1 : 0; resultLength = 3; return; } @@ -171,6 +171,7 @@ void SecurityInterfaceObject::secureApplicationLayer(SecureApplicationLayer& sec uint8_t* SecurityInterfaceObject::save(uint8_t* buffer) { buffer = pushByte(_state, buffer); + buffer = pushByte(_securityModeEnabled, buffer); return InterfaceObject::save(buffer); } @@ -181,12 +182,28 @@ const uint8_t* SecurityInterfaceObject::restore(const uint8_t* buffer) buffer = popByte(state, buffer); _state = (LoadState)state; + uint8_t securityModeEnabled = 0; + buffer = popByte(securityModeEnabled, buffer); + _securityModeEnabled = securityModeEnabled; + return InterfaceObject::restore(buffer); } uint16_t SecurityInterfaceObject::saveSize() { - return 1 + InterfaceObject::saveSize(); + return 2 + InterfaceObject::saveSize(); +} + +void SecurityInterfaceObject::setSecurityMode(bool enabled) +{ + print("Security mode set to: "); + println(enabled ? "enabled" : "disabled"); + _securityModeEnabled = enabled; +} + +bool SecurityInterfaceObject::isSecurityModeEnabled() +{ + return _securityModeEnabled; } bool SecurityInterfaceObject::isLoaded() @@ -314,16 +331,17 @@ void SecurityInterfaceObject::errorCode(ErrorCode errorCode) prop->write(data); } -void SecurityInterfaceObject::factoryReset() +void SecurityInterfaceObject::masterReset(EraseCode eraseCode) { + // TODO handle different erase codes println("Factory reset of security interface object requested."); - _secAppLayer->setSecurityMode(false); + setSecurityMode(false); property(PID_TOOL_KEY)->write(1, 1, _fdsk); } -const uint8_t* SecurityInterfaceObject::toolKey(uint16_t devAddr) +const uint8_t* SecurityInterfaceObject::toolKey() { - //TODO: check if multiple tool keys possible, leave it for now + // There is only one tool key const uint8_t* toolKey = propertyData(PID_TOOL_KEY); return toolKey; } @@ -341,7 +359,7 @@ const uint8_t* SecurityInterfaceObject::p2pKey(uint16_t addressIndex) uint8_t elementSize = propertySize(PID_P2P_KEY_TABLE); // Search for address index - uint8_t entry[elementSize]; // 2 bytes index + keysize (16 bytes) + 2 bytes(for what?) = 20 bytes + uint8_t entry[elementSize]; // 2 bytes index + keysize (16 bytes) + 2 bytes(roles) = 20 bytes for (int i = 1; i <= numElements; i++) { property(PID_P2P_KEY_TABLE)->read(i, 1, entry); @@ -498,7 +516,7 @@ void SecurityInterfaceObject::setLastValidSequenceNumber(uint16_t deviceAddr, ui } } -DataSecurity SecurityInterfaceObject::getGroupObjectSecurity(uint16_t index, bool isWrite) +DataSecurity SecurityInterfaceObject::getGroupObjectSecurity(uint16_t index) { // security table uses same index as group object table @@ -508,20 +526,9 @@ DataSecurity SecurityInterfaceObject::getGroupObjectSecurity(uint16_t index, boo if (count > 0) { - bool conf; - bool auth; - if (isWrite) - { - // write access flags, draft spec. p.68 - conf = (data[0] & 2) == 2; - auth = (data[0] & 1) == 1; - } - else - { - // Read access flags, draft spec. p.68 - conf = (data[0] & 8) == 8; - auth = (data[0] & 4) == 4; - } + // write access flags, approved spec. AN158, p.97 + bool conf = (data[0] & 2) == 2; + bool auth = (data[0] & 1) == 1; return conf ? DataSecurity::authConf : auth ? DataSecurity::auth : DataSecurity::none; } diff --git a/src/knx/security_interface_object.h b/src/knx/security_interface_object.h index ea7e3cf..5cf9cd7 100644 --- a/src/knx/security_interface_object.h +++ b/src/knx/security_interface_object.h @@ -15,11 +15,13 @@ public: void secureApplicationLayer(SecureApplicationLayer& secAppLayer); - void factoryReset(); + void masterReset(EraseCode eraseCode); + + bool isSecurityModeEnabled(); bool isLoaded(); - const uint8_t* toolKey(uint16_t devAddr); // returns single tool key (ETS) + const uint8_t* toolKey(); // returns single tool key (ETS) const uint8_t* p2pKey(uint16_t addressIndex); // returns p2p key for IA index const uint8_t* groupKey(uint16_t addressIndex); // returns group key for group address index @@ -29,7 +31,7 @@ public: uint64_t getLastValidSequenceNumber(uint16_t deviceAddr); void setLastValidSequenceNumber(uint16_t deviceAddr, uint64_t seqNum); - DataSecurity getGroupObjectSecurity(uint16_t index, bool isWrite); + DataSecurity getGroupObjectSecurity(uint16_t index); LoadState loadState(); uint8_t* save(uint8_t* buffer) override; @@ -39,6 +41,8 @@ public: private: SecureApplicationLayer* _secAppLayer = nullptr; + void setSecurityMode(bool enabled); + void errorCode(ErrorCode errorCode); void loadEvent(const uint8_t* data); @@ -50,6 +54,8 @@ private: void loadState(LoadState newState); LoadState _state = LS_UNLOADED; + bool _securityModeEnabled {false}; + uint16_t getNumberOfElements(PropertyID propId); // Our FDSK