Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzzing updates: all-clusters-app #27858

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions config/python/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@
//
#define CHIP_CONFIG_SECURITY_TEST_MODE 0

// For convenience, Chip Security Fuzz Mode can be enabled to
// create deterministic behaviour to better facilitate fuzzing.
//
// WARNING: This option circumvents basic Chip security functionality,
// including message encryption. Because of this it MUST NEVER BE ENABLED IN PRODUCTION BUILDS.
//
// To build with this flag, pass 'treat_warnings_as_errors=false' to gn/ninja.
//
#define CHIP_CONFIG_SECURITY_FUZZ_MODE 0

#define CHIP_CONFIG_ENABLE_UPDATE 1

#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 0
Expand Down
10 changes: 10 additions & 0 deletions config/standalone/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@
//
#define CHIP_CONFIG_SECURITY_TEST_MODE 0

// For convenience, Chip Security Fuzz Mode can be enabled to
// create deterministic behaviour to better facilitate fuzzing.
//
// WARNING: This option circumvents basic Chip security functionality,
// including message encryption. Because of this it MUST NEVER BE ENABLED IN PRODUCTION BUILDS.
//
// To build with this flag, pass 'treat_warnings_as_errors=false' to gn/ninja.
//
#define CHIP_CONFIG_SECURITY_FUZZ_MODE 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the default value, though? And if so, why are we adding it to all the project configs?


#define CHIP_CONFIG_ENABLE_UPDATE 1

#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 0
Expand Down
25 changes: 25 additions & 0 deletions examples/all-clusters-app/linux/fuzzing-main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@

#include "AppMain.h"
#include <app/server/Server.h>
#include <transport/SessionManager.h>
#include <transport/TransportMgr.h>
#include <transport/raw/PeerAddress.h>

#include <CommissionableInit.h>

Expand All @@ -29,6 +32,22 @@ LinuxCommissionableDataProvider gCommissionableDataProvider;

}

Transport::PeerAddress AddressFromString(const char * str)
{
Inet::IPAddress addr;

VerifyOrDie(Inet::IPAddress::FromString(str, addr));

return Transport::PeerAddress::UDP(addr);
}

uint16_t kLocalSessionId = 1;
uint16_t kPeerSessionId = 2;
const NodeId kLocalNodeId = 123;
const NodeId kPeerNodeId = 123;
const FabricIndex kFabricIndex = 1;
const Transport::PeerAddress kPeerAddress = AddressFromString("fe80::1");

void CleanShutdown()
{
Server::GetInstance().Shutdown();
Expand Down Expand Up @@ -64,6 +83,12 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t * aData, size_t aSize)

matterStackInitialized = true;

// And add a test session
SessionHolder testSessionHolder;
Server::GetInstance().GetSecureSessionManager().InjectCaseSessionWithTestKey(
testSessionHolder, kLocalSessionId, kPeerSessionId, kLocalNodeId, kPeerNodeId, kFabricIndex, kPeerAddress,
CryptoContext::SessionRole::kResponder);

// The fuzzer does not have a way to tell us when it's done, so just
// shut down things on exit.
atexit(CleanShutdown);
Expand Down
10 changes: 10 additions & 0 deletions examples/chip-tool/include/CHIPProjectAppConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@
//
#define CHIP_CONFIG_SECURITY_TEST_MODE 0

// For convenience, Chip Security Fuzz Mode can be enabled to
// create deterministic behaviour to better facilitate fuzzing.
//
// WARNING: This option circumvents basic Chip security functionality,
// including message encryption. Because of this it MUST NEVER BE ENABLED IN PRODUCTION BUILDS.
//
// To build with this flag, pass 'treat_warnings_as_errors=false' to gn/ninja.
//
#define CHIP_CONFIG_SECURITY_FUZZ_MODE 0

#define CHIP_CONFIG_ENABLE_UPDATE 1

#define CHIP_SYSTEM_CONFIG_PACKETBUFFER_POOL_SIZE 0
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
16 changes: 16 additions & 0 deletions src/lib/core/CHIPConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,22 @@
#define CHIP_CONFIG_SECURITY_TEST_MODE 0
#endif // CHIP_CONFIG_SECURITY_TEST_MODE

/**
* @def CHIP_CONFIG_SECURITY_FUZZ_MODE
*
* @brief
* Enable various features that facilitate better fuzzing.
*
* @note
* WARNING: This option makes it possible to circumvent basic chip security functionality,
* including message encryption. Because of this it SHOULD NEVER BE ENABLED IN PRODUCTION BUILDS.
*
* To build with this flag, pass 'treat_warnings_as_errors=false' to gn/ninja.
*/
#ifndef CHIP_CONFIG_SECURITY_FUZZ_MODE
#define CHIP_CONFIG_SECURITY_FUZZ_MODE 0
#endif // CHIP_CONFIG_SECURITY_FUZZ_MODE

/**
* @def CHIP_CONFIG_TEST_SHARED_SECRET_VALUE
*
Expand Down
4 changes: 2 additions & 2 deletions src/platform/OpenThread/OpenThreadUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,15 @@ void LogOpenThreadStateChange(otInstance * otInst, uint32_t flags)
meshPrefix.ToString(strBuf);
ChipLogDetail(DeviceLayer, " Mesh Prefix: %s/64", strBuf);
}
#if CHIP_CONFIG_SECURITY_TEST_MODE
#if CHIP_CONFIG_SECURITY_TEST_MODE || CHIP_CONFIG_SECURITY_FUZZ_MODE
{
otNetworkKey otKey;
otThreadGetNetworkKey(otInst, &otKey);
for (int i = 0; i < OT_NETWORK_KEY_SIZE; i++)
snprintf(&strBuf[i * 2], 3, "%02X", otKey.m8[i]);
ChipLogDetail(DeviceLayer, " Network Key: %s", strBuf);
}
#endif // CHIP_CONFIG_SECURITY_TEST_MODE
#endif // CHIP_CONFIG_SECURITY_TEST_MODE || CHIP_CONFIG_SECURITY_FUZZ_MODE
}
if ((flags & OT_CHANGED_THREAD_PARTITION_ID) != 0)
{
Expand Down
4 changes: 2 additions & 2 deletions src/transport/CryptoContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ CHIP_ERROR CryptoContext::InitFromSecret(SessionKeystore & keystore, const ByteS
auto & i2rKey = (role == SessionRole::kInitiator) ? mEncryptionKey : mDecryptionKey;
auto & r2iKey = (role == SessionRole::kInitiator) ? mDecryptionKey : mEncryptionKey;

#if CHIP_CONFIG_SECURITY_TEST_MODE
#if CHIP_CONFIG_SECURITY_TEST_MODE || CHIP_CONFIG_SECURITY_FUZZ_MODE

// If enabled, override the generated session key with a known key pair
// to allow man-in-the-middle session key recovery for testing purposes.
Expand Down Expand Up @@ -133,7 +133,7 @@ CHIP_ERROR CryptoContext::BuildNonce(NonceView nonce, uint8_t securityFlags, uin

bbuf.Put8(securityFlags);
bbuf.Put32(messageCounter);
#if CHIP_CONFIG_SECURITY_TEST_MODE
#if CHIP_CONFIG_SECURITY_TEST_MODE || CHIP_CONFIG_SECURITY_FUZZ_MODE
bbuf.Put64(0); // Simplifies decryption of CASE sessions when in TEST_MODE.
#else
bbuf.Put64(nodeId);
Expand Down
21 changes: 21 additions & 0 deletions src/transport/SecureMessageCodec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ CHIP_ERROR Encrypt(const CryptoContext & context, CryptoContext::ConstNonceView

ReturnErrorOnFailure(payloadHeader.EncodeBeforeData(msgBuf));

// Skip encryption and message integrity!
#if CHIP_CONFIG_SECURITY_FUZZ_MODE
#warning \
"Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing encryption! Node can only communicate with other nodes built with this flag set. Requires build flag 'treat_warnings_as_errors=false'."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "requires build flag" bit is:

  1. Not really in need of being documented here.
  2. Seems to not be needed at all, if this code just has #else around the bits we want to turn off....

Same for Decrypt.

ChipLogError(SecureChannel,
"Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing encryption... "
"Node can only communicate with other nodes built with this flag set.");
return CHIP_NO_ERROR;
#endif

uint8_t * data = msgBuf->Start();
uint16_t totalLen = msgBuf->TotalLength();

Expand All @@ -68,6 +78,17 @@ CHIP_ERROR Decrypt(const CryptoContext & context, CryptoContext::ConstNonceView
{
ReturnErrorCodeIf(msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);

#if CHIP_CONFIG_SECURITY_FUZZ_MODE
#warning \
"Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing decryption! Node can only communicate with other nodes built with this flag set. Requires build flag 'treat_warnings_as_errors=false'."
ChipLogError(SecureChannel,
"Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing decryption... "
"Node can only communicate with other nodes built with this flag set.");

ReturnErrorOnFailure(payloadHeader.DecodeAndConsume(msg));
return CHIP_NO_ERROR;
#endif

uint8_t * data = msg->Start();
uint16_t len = msg->DataLength();

Expand Down
12 changes: 11 additions & 1 deletion src/transport/SecureSessionTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,17 @@ class SecureSessionTable
public:
~SecureSessionTable() { mEntries.ReleaseAll(); }

void Init() { mNextSessionId = chip::Crypto::GetRandU16(); }
void Init()
{
#if CHIP_CONFIG_SECURITY_FUZZ_MODE
#warning "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing random sessionId!"
ChipLogError(SecureChannel, "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing random sessionId... ");
static uint16_t manualSessionId = 1;
mNextSessionId = ++manualSessionId;
#else
mNextSessionId = chip::Crypto::GetRandU16();
#endif
}

/**
* Allocate a new secure session out of the internal resource pool.
Expand Down
28 changes: 23 additions & 5 deletions src/transport/SessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -685,8 +685,6 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & partialPa

CHIP_ERROR err = CHIP_NO_ERROR;

Optional<SessionHandle> session = mSecureSessions.FindSecureSessionByLocalKey(partialPacketHeader.GetSessionId());

PayloadHeader payloadHeader;

// Drop secure unicast messages with privacy enabled.
Expand All @@ -699,14 +697,27 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & partialPa
PacketHeader packetHeader;
ReturnOnFailure(packetHeader.DecodeAndConsume(msg));

SessionMessageDelegate::DuplicateMessage isDuplicate = SessionMessageDelegate::DuplicateMessage::No;

if (msg.IsNull())
{
ChipLogError(Inet, "Secure transport received Unicast NULL packet, discarding");
return;
}

SessionMessageDelegate::DuplicateMessage isDuplicate = SessionMessageDelegate::DuplicateMessage::No;

Optional<SessionHandle> session = mSecureSessions.FindSecureSessionByLocalKey(partialPacketHeader.GetSessionId());

#if CHIP_CONFIG_SECURITY_FUZZ_MODE
// If no valid existing session was found - try to use test session instead.
if (!session.HasValue())
{
#warning "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 using default session!"
ChipLogError(SecureChannel, "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 using default session... ");
uint16_t kLocalSessionId = 1;
session = mSecureSessions.FindSecureSessionByLocalKey(kLocalSessionId);
}
#endif

if (!session.HasValue())
{
ChipLogError(Inet, "Data received on an unknown session (LSID=%d). Dropping it!", packetHeader.GetSessionId());
Expand Down Expand Up @@ -749,8 +760,15 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & partialPa
"Received a duplicate message with MessageCounter:" ChipLogFormatMessageCounter
" on exchange " ChipLogFormatExchangeId,
packetHeader.GetMessageCounter(), ChipLogValueExchangeIdFromReceivedHeader(payloadHeader));

#if CHIP_CONFIG_SECURITY_FUZZ_MODE
#warning "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing duplicate message check!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use some documentation explaining why we would want to do that.

ChipLogError(SecureChannel, "Warning: CHIP_CONFIG_SECURITY_FUZZ_MODE=1 bypassing duplicate message check... ");
#else
isDuplicate = SessionMessageDelegate::DuplicateMessage::Yes;
err = CHIP_NO_ERROR;
#endif

err = CHIP_NO_ERROR;
}
if (err != CHIP_NO_ERROR)
{
Expand Down
4 changes: 2 additions & 2 deletions src/transport/tests/TestSessionManagerDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ struct MessageTestEntry theMessageTestVector[] = {

.expectedMessageCount = 0,
},
#if !CHIP_CONFIG_SECURITY_TEST_MODE
#if !CHIP_CONFIG_SECURITY_TEST_MODE && !CHIP_CONFIG_SECURITY_FUZZ_MODE
// =======================================
// GROUP positive test cases
// =======================================
Expand Down Expand Up @@ -405,7 +405,7 @@ struct MessageTestEntry theMessageTestVector[] = {
},
#endif // CHIP_CONFIG_PRIVACY_ACCEPT_NONSPEC_SVE2

#endif // !CHIP_CONFIG_SECURITY_TEST_MODE
#endif // !CHIP_CONFIG_SECURITY_TEST_MODE && !CHIP_CONFIG_SECURITY_FUZZ_MODE
};

const uint16_t theMessageTestVectorLength = sizeof(theMessageTestVector) / sizeof(theMessageTestVector[0]);
Expand Down