From 334056203b81158859479f79ae942666a89f837e Mon Sep 17 00:00:00 2001 From: Michael Spang Date: Tue, 15 Jun 2021 14:26:42 -0400 Subject: [PATCH] Fix uninitialized memory during chip-tool pairing (#7577) Fix various uses of unininitialized memory during a simple commissioning session: CHIPOperationalCredentials: ==1087539== Conditional jump or move depends on uninitialised value(s) ==1087539== at 0x2BE628: chip::Credentials::OperationalCredentialSet::Release() (CHIPOperationalCredentials.cpp:83) ==1087539== by 0x296941: chip::Transport::AdminPairingInfo::GetCredentials(chip::Credentials::OperationalCredentialSet&, chip::Credentials::ChipCertificateSet&, chip::Credentials::CertificateKeyId&) (AdminPairingTable.cpp:284) ==1087539== by 0x249A30: chip::Controller::DeviceController::LoadLocalCredentials(chip::Transport::AdminPairingInfo*) (CHIPDeviceController.cpp:283) ==1087539== by 0x2491BC: chip::Controller::DeviceController::Init(unsigned long, chip::Controller::ControllerInitParams) (CHIPDeviceController.cpp:228) ==1087539== by 0x24BDB3: chip::Controller::DeviceCommissioner::Init(unsigned long, chip::Controller::CommissionerInitParams) (CHIPDeviceController.cpp:789) ==1087539== by 0x167E76: PairingCommand::Run(PersistentStorage&, unsigned long, unsigned long) (PairingCommand.cpp:46) ==1087539== by 0x16246D: Commands::RunCommand(PersistentStorage&, unsigned long, unsigned long, int, char**) (Commands.cpp:128) ==1087539== by 0x161DDA: Commands::Run(unsigned long, unsigned long, int, char**) (Commands.cpp:59) ==1087539== by 0x18802E: main (main.cpp:43) IPAddress: ==1087539== Thread 9: ==1087539== Conditional jump or move depends on uninitialised value(s) ==1087539== at 0x25E981: chip::Inet::IPAddress::operator!=(chip::Inet::IPAddress const&) const (IPAddress.cpp:55) ==1087539== by 0x298B18: chip::SecureSessionMgr::NewPairing(chip::Optional const&, unsigned long, chip::PairingSession*, chip::SecureSession::SessionRole, unsigned short, chip::Transport::Base*) (SecureSessionMgr.cpp:256) ==1087539== by 0x24D3BA: chip::Controller::DeviceCommissioner::OnSessionEstablished() (CHIPDeviceController.cpp:1082) ==1087539== by 0x2CC87D: chip::PASESession::HandleMsg2_and_SendMsg3(chip::System::PacketBufferHandle const&) (PASESession.cpp:653) ==1087539== by 0x2CCF15: chip::PASESession::OnMessageReceived(chip::Messaging::ExchangeContext*, chip::PacketHeader const&, chip::PayloadHeader const&, chip::System::PacketBufferHandle&&) (PASESession.cpp:809) ==1087539== by 0x269DAB: chip::Messaging::ExchangeContext::HandleMessage(chip::PacketHeader const&, chip::PayloadHeader const&, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) (ExchangeContext.cpp:400) ==1087539== by 0x26CFE3: auto chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SecureSessionHandle, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&, chip::SecureSessionMgr*)::$_1::operator()(chip::Messaging::ExchangeContext*) const (ExchangeMgr.cpp:223) ==1087539== by 0x26C4F3: bool chip::BitMapObjectPool::ForEachActiveObject(chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SecureSessionHandle, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&, chip::SecureSessionMgr*)::$_1) (Pool.h:140) ==1087539== by 0x26BFE8: chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SecureSessionHandle, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&, chip::SecureSessionMgr*) (ExchangeMgr.cpp:212) ==1087539== by 0x299D3B: chip::SecureSessionMgr::MessageDispatch(chip::PacketHeader const&, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) (SecureSessionMgr.cpp:321) ==1087539== by 0x2992DA: chip::SecureSessionMgr::OnMessageReceived(chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) (SecureSessionMgr.cpp:310) ==1087539== by 0x29BF87: chip::TransportMgrBase::HandleMessageReceived(chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) (TransportMgrBase.cpp:57) ==1087539== SerializedDevice: ==1087539== Conditional jump or move depends on uninitialised value(s) ==1087539== at 0x266FA1: chip::Base64ValToChar(unsigned char) (Base64.cpp:39) ==1087539== by 0x266EFB: chip::Base64Encode(unsigned char const*, unsigned short, char*, char (*)(unsigned char)) (Base64.cpp:150) ==1087539== by 0x26709E: chip::Base64Encode32(unsigned char const*, unsigned int, char*, char (*)(unsigned char)) (Base64.cpp:183) ==1087539== by 0x267129: chip::Base64Encode32(unsigned char const*, unsigned int, char*) (Base64.cpp:200) ==1087539== by 0x170B6A: (anonymous namespace)::StringToBase64(std::__2::basic_string, std::__2::allocator > const&) (PersistentStorage.cpp:46) ==1087539== by 0x170A28: PersistentStorage::SyncSetKeyValue(char const*, void const*, unsigned short) (PersistentStorage.cpp:118) ==1087539== by 0x247320: chip::Controller::Device::Persist() (CHIPDevice.cpp:293) ==1087539== by 0x24A836: chip::Controller::DeviceController::PersistDevice(chip::Controller::Device*) (CHIPDeviceController.cpp:480) ==1087539== by 0x24E580: chip::Controller::DeviceCommissioner::OnOperationalCredentialsProvisioningCompletion(chip::Controller::Device*) (CHIPDeviceController.cpp:1354) ==1087539== by 0x24BA82: chip::Controller::DeviceCommissioner::OnOperationalCertificateAddResponse(void*, unsigned char, unsigned long, unsigned char*) (CHIPDeviceController.cpp:1261) ==1087539== by 0x21CC56: IMDefaultResponseCallback(chip::app::Command const*, EmberAfStatus) (CHIPClientCallbacks.cpp:303) ==1087539== by 0x24E8E0: chip::Controller::DeviceControllerInteractionModelDelegate::CommandResponseStatus(chip::app::CommandSender const*, chip::Protocols::SecureChannel::GeneralStatusCode, unsigned int, unsigned short, unsigned char, unsigned short, unsigned char, unsigned char) (CHIPDeviceController.cpp:1458) ==1087539== Tested via: `valgrind ./chip-tool pairing ble-wifi 112233 20202021 3840` fixes #7576 --- src/controller/CHIPDevice.cpp | 9 +++++---- src/credentials/CHIPCert.h | 6 +++--- src/credentials/CHIPOperationalCredentials.h | 20 ++++++++++---------- src/transport/raw/PeerAddress.h | 4 ++-- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 83ba264c74819e..d00c6b6bb88a82 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -163,6 +163,7 @@ CHIP_ERROR Device::Serialize(SerializedDevice & output) "Size of serializable should be <= size of output"); CHIP_ZERO_AT(serializable); + CHIP_ZERO_AT(output); serializable.mOpsCreds = mPairing; serializable.mDeviceId = Encoding::LittleEndian::HostSwap64(mDeviceId); @@ -203,9 +204,10 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input) { CHIP_ERROR error = CHIP_NO_ERROR; SerializableDevice serializable; - size_t maxlen = BASE64_ENCODED_LEN(sizeof(serializable)); - size_t len = strnlen(Uint8::to_const_char(&input.inner[0]), maxlen); - uint16_t deserializedLen = 0; + size_t maxlen = BASE64_ENCODED_LEN(sizeof(serializable)); + size_t len = strnlen(Uint8::to_const_char(&input.inner[0]), maxlen); + uint16_t deserializedLen = 0; + Inet::IPAddress ipAddress = {}; VerifyOrExit(len < sizeof(SerializedDevice), error = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(CanCastTo(len), error = CHIP_ERROR_INVALID_ARGUMENT); @@ -217,7 +219,6 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input) VerifyOrExit(deserializedLen > 0, error = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(deserializedLen <= sizeof(serializable), error = CHIP_ERROR_INVALID_ARGUMENT); - Inet::IPAddress ipAddress; uint16_t port; Inet::InterfaceId interfaceId; diff --git a/src/credentials/CHIPCert.h b/src/credentials/CHIPCert.h index 4211373ebb05e1..449c2733808773 100755 --- a/src/credentials/CHIPCert.h +++ b/src/credentials/CHIPCert.h @@ -278,9 +278,9 @@ class ChipDN */ struct CertificateKeyId { - const uint8_t * mId; /**< Pointer to the key identifier. Encoded as Octet String and represented as the ASN.1 DER Integer (X.690 - standard). */ - uint8_t mLen; /**< Key identifier length. */ + const uint8_t * mId = nullptr; /**< Pointer to the key identifier. Encoded as Octet String and represented as the ASN.1 DER + Integer (X.690 standard). */ + uint8_t mLen = 0; /**< Key identifier length. */ bool IsEqual(const CertificateKeyId & other) const; bool IsEmpty() const { return mId == nullptr; } diff --git a/src/credentials/CHIPOperationalCredentials.h b/src/credentials/CHIPOperationalCredentials.h index e9abae03d66a23..143ed287b0f7e8 100644 --- a/src/credentials/CHIPOperationalCredentials.h +++ b/src/credentials/CHIPOperationalCredentials.h @@ -254,16 +254,16 @@ class DLL_EXPORT OperationalCredentialSet CHIP_ERROR SetDevOpCredKeypair(const CertificateKeyId & trustedRootId, P256Keypair * newKeypair); private: - ChipCertificateSet * mOpCreds; /**< Pointer to an array of certificate data. */ - uint8_t mOpCredCount; /**< Number of certificates in mOpCreds - array. We maintain the invariant that all - the slots at indices less than - mCertCount have been constructed and slots - at indices >= mCertCount have either never - had their constructor called, or have had - their destructor called since then. */ - uint8_t mMaxCerts; /**< Length of mOpCreds array. */ - bool mMemoryAllocInternal; /**< Indicates whether temporary memory buffers are allocated internally. */ + ChipCertificateSet * mOpCreds; /**< Pointer to an array of certificate data. */ + uint8_t mOpCredCount; /**< Number of certificates in mOpCreds + array. We maintain the invariant that all + the slots at indices less than + mCertCount have been constructed and slots + at indices >= mCertCount have either never + had their constructor called, or have had + their destructor called since then. */ + uint8_t mMaxCerts; /**< Length of mOpCreds array. */ + bool mMemoryAllocInternal = false; /**< Indicates whether temporary memory buffers are allocated internally. */ NodeCredentialMap mChipDeviceCredentials[kOperationalCredentialsMax]; uint8_t mChipDeviceCredentialsCount; NodeKeypairMap mDeviceOpCredKeypair[kOperationalCredentialsMax]; diff --git a/src/transport/raw/PeerAddress.h b/src/transport/raw/PeerAddress.h index b4632898d41f25..94a78eb476d8f3 100644 --- a/src/transport/raw/PeerAddress.h +++ b/src/transport/raw/PeerAddress.h @@ -176,8 +176,8 @@ class PeerAddress } private: - Inet::IPAddress mIPAddress; - Type mTransportType; + Inet::IPAddress mIPAddress = {}; + Type mTransportType = Type::kUndefined; uint16_t mPort = CHIP_PORT; ///< Relevant for UDP data sending. Inet::InterfaceId mInterface = INET_NULL_INTERFACEID; };