Skip to content

Commit

Permalink
Fix integer overflow in ExtractIdFromServiceName. (#8779)
Browse files Browse the repository at this point in the history 
In particular, we enforce that the ids we are trying to extract are
the right length (exactly 16 hex chars each).
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Sep 8, 2021
1 parent a0aa899 commit 9685f7f
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 67 deletions.
82 changes: 28 additions & 54 deletions src/lib/mdns/ServiceNaming.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,35 +17,16 @@

#include "ServiceNaming.h"

#include <support/CodeUtils.h>
#include <lib/core/CHIPEncoding.h>
#include <lib/support/BytesToHex.h>
#include <lib/support/CodeUtils.h>

#include <cstdio>
#include <inttypes.h>
#include <string.h>

namespace chip {
namespace Mdns {
namespace {

uint8_t HexToInt(char c)
{
if ('0' <= c && c <= '9')
{
return static_cast<uint8_t>(c - '0');
}
else if ('a' <= c && c <= 'f')
{
return static_cast<uint8_t>(0x0a + c - 'a');
}
else if ('A' <= c && c <= 'F')
{
return static_cast<uint8_t>(0x0a + c - 'A');
}

return UINT8_MAX;
}

} // namespace

CHIP_ERROR MakeInstanceName(char * buffer, size_t bufferLen, const PeerId & peerId)
{
Expand All @@ -65,43 +46,36 @@ CHIP_ERROR ExtractIdFromInstanceName(const char * name, PeerId * peerId)
ReturnErrorCodeIf(name == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
ReturnErrorCodeIf(peerId == nullptr, CHIP_ERROR_INVALID_ARGUMENT);

peerId->SetNodeId(0);
peerId->SetFabricId(0);
// Make sure the string is long enough.
static constexpr size_t fabricIdByteLength = 8;
static constexpr size_t fabricIdStringLength = fabricIdByteLength * 2;
static constexpr size_t nodeIdByteLength = 8;
static constexpr size_t nodeIdStringLength = nodeIdByteLength * 2;
static constexpr size_t totalLength = fabricIdStringLength + nodeIdStringLength + 1; // +1 for '-'

bool deliminatorFound = false;
bool hasFabricPart = false;
bool hasNodePart = false;
// Ensure we have at least totalLength chars.
size_t len = strnlen(name, totalLength);
ReturnErrorCodeIf(len < totalLength, CHIP_ERROR_INVALID_ARGUMENT);

for (; *name != '\0'; name++)
{
if (*name == '.')
{
break;
}
else if (*name == '-')
{
deliminatorFound = true;
continue;
}
// Check that we have a proper terminator.
ReturnErrorCodeIf(name[totalLength] != '\0' && name[totalLength] != '.', CHIP_ERROR_WRONG_NODE_ID);

uint8_t val = HexToInt(*name);
ReturnErrorCodeIf(val == UINT8_MAX, CHIP_ERROR_WRONG_NODE_ID);
// Check what we have a separator where we expect.
ReturnErrorCodeIf(name[fabricIdStringLength] != '-', CHIP_ERROR_WRONG_NODE_ID);

if (deliminatorFound)
{
hasNodePart = true;
peerId->SetNodeId(peerId->GetNodeId() * 16 + val);
}
else
{
hasFabricPart = true;
peerId->SetFabricId(peerId->GetFabricId() * 16 + val);
}
}
static constexpr size_t bufferSize = max(fabricIdByteLength, nodeIdByteLength);
uint8_t buf[bufferSize];

ReturnErrorCodeIf(Encoding::HexToBytes(name, fabricIdStringLength, buf, bufferSize) == 0, CHIP_ERROR_WRONG_NODE_ID);
// Buf now stores the fabric id, as big-endian bytes.
static_assert(fabricIdByteLength == sizeof(uint64_t), "Wrong number of bytes");
peerId->SetFabricId(Encoding::BigEndian::Get64(buf));

ReturnErrorCodeIf(!deliminatorFound, CHIP_ERROR_WRONG_NODE_ID);
ReturnErrorCodeIf(!hasNodePart, CHIP_ERROR_WRONG_NODE_ID);
ReturnErrorCodeIf(!hasFabricPart, CHIP_ERROR_WRONG_NODE_ID);
ReturnErrorCodeIf(Encoding::HexToBytes(name + fabricIdStringLength + 1, nodeIdStringLength, buf, bufferSize) == 0,
CHIP_ERROR_WRONG_NODE_ID);
// Buf now stores the node id id, as big-endian bytes.
static_assert(nodeIdByteLength == sizeof(uint64_t), "Wrong number of bytes");
peerId->SetNodeId(Encoding::BigEndian::Get64(buf));

return CHIP_NO_ERROR;
}
Expand Down
5 changes: 4 additions & 1 deletion src/lib/mdns/ServiceNaming.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ constexpr size_t kMaxOperationalServiceNameSize =
/// builds the MDNS advertising name for a given fabric + nodeid pair
CHIP_ERROR MakeInstanceName(char * buffer, size_t bufferLen, const PeerId & peerId);

/// Inverse of MakeInstanceName
/// Inverse of MakeInstanceName. Will return errors on non-spec-compliant ids,
/// _except_ for allowing lowercase hex, not just the spec-defined uppercase
/// hex. The part of "name" up to the first '.' (or end of string, whichever
/// comes first) is parsed as a FABRICID-NODEID.
CHIP_ERROR ExtractIdFromInstanceName(const char * name, PeerId * peerId);

/// Generates the host name that a CHIP device is to use for a given unique
Expand Down
38 changes: 26 additions & 12 deletions src/lib/mdns/tests/TestServiceNaming.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,28 +57,42 @@ void TestExtractIdFromInstanceName(nlTestSuite * inSuite, void * inContext)
PeerId peerId;

NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName(nullptr, nullptr) == CHIP_ERROR_INVALID_ARGUMENT);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("01234-5678", nullptr) == CHIP_ERROR_INVALID_ARGUMENT);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("ACDEF1234567890-1234567890ABCDEF", nullptr) == CHIP_ERROR_INVALID_ARGUMENT);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName(nullptr, &peerId) == CHIP_ERROR_INVALID_ARGUMENT);

// Short format is acceptable
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("ABCD-1234", &peerId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, peerId == PeerId().SetFabricId(0xABCD).SetNodeId(0x1234));
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("ABCDEF1234567890-1234567890ABCDEF", &peerId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, peerId == PeerId().SetFabricId(0xABCDEF1234567890ULL).SetNodeId(0x1234567890ABCDEFULL));

// ending in period (partial name) is acceptable
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122-aabb.some.suffix.here", &peerId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, peerId == PeerId().SetFabricId(0x1122).SetNodeId(0xaabb));
NL_TEST_ASSERT(inSuite,
ExtractIdFromInstanceName("1122334455667788-AABBCCDDEEFF1122.some.suffix.here", &peerId) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, peerId == PeerId().SetFabricId(0x1122334455667788ULL).SetNodeId(0xaabbccddeeff1122ULL));

// Invalid: non hex character
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1x22-aabc", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1x22334455667788-AABBCCDDEEDD1122", &peerId) != CHIP_NO_ERROR);

// Invalid: missing node id part (no - separator)
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("11x22", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("11x22.12-33.4455", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("11x22.4455", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788x2233445566778899", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788x2233445566778899.12-33.4455", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788x2233445566778899.4455", &peerId) != CHIP_NO_ERROR);

// Invalid: missing part
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("-1234", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1234-", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("-1234567890ABCDEF", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1234567890ABCDEF-", &peerId) != CHIP_NO_ERROR);

// Invalid: separator in wrong place
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("112233445566778-8AABBCCDDEEFF1122", &peerId) != CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788A-ABBCCDDEEFF1122", &peerId) != CHIP_NO_ERROR);

// Invalid: fabric part too short
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("11223344556677-AABBCCDDEEFF1122", &peerId) != CHIP_NO_ERROR);
// Invalid: fabric part too long
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("112233445566778899-AABBCCDDEEFF1122", &peerId) != CHIP_NO_ERROR);

// Invalid: node part too short
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788-AABBCCDDEEFF11", &peerId) != CHIP_NO_ERROR);
// Invalid: node part too long
NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1122334455667788-AABBCCDDEEFF112233", &peerId) != CHIP_NO_ERROR);
}

void TestMakeServiceNameSubtype(nlTestSuite * inSuite, void * inContext)
Expand Down

0 comments on commit 9685f7f

Please sign in to comment.