Skip to content

Commit

Permalink
Fix uninitialized memory during chip-tool pairing (#7577)
Browse files Browse the repository at this point in the history 
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<chip::Transport::PeerAddress> 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>(chip::Messaging::ExchangeContext*) const (ExchangeMgr.cpp:223)
==1087539==    by 0x26C4F3: bool chip::BitMapObjectPool<chip::Messaging::ExchangeContext, 8ul>::ForEachActiveObject<chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SecureSessionHandle, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&, chip::SecureSessionMgr*)::$_1>(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<char, std::__2::char_traits<char>, std::__2::allocator<char> > 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 <ssid> <password> 112233 20202021 3840`

fixes #7576
  • Loading branch information
@mspang @pull
mspang authored and pull[bot] committed Jun 23, 2021
1 parent fc6f353 commit 3340562
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 19 deletions.
9 changes: 5 additions & 4 deletions src/controller/CHIPDevice.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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<uint16_t>(len), error = CHIP_ERROR_INVALID_ARGUMENT);
Expand All @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions src/credentials/CHIPCert.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
20 changes: 10 additions & 10 deletions src/credentials/CHIPOperationalCredentials.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
4 changes: 2 additions & 2 deletions src/transport/raw/PeerAddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down

0 comments on commit 3340562

Please sign in to comment.