Skip to content

Commit

Permalink
Fix in-place save for DefaultSessionResumptionStorage (#23062)
Browse files Browse the repository at this point in the history 
Nodes that already exist in the DefaultSessionResumptionStorage index
are consuming additional entries in the index on save, and aren't
cleaned from the link and state tables first.  This has the following
consequences:

1. If session resumption storage is full, this causes unnecessary
   eviction of records for other nodes.
2. This pulls the index out of sync from the backing state table
   because duplicate entries in the index for a given node only
   map to a single node-id-keyed entry in the state table.
3. An entry in the link table is leaked on each occurrence, as the 
   resumption-id-keyed link entry will be orphaned once its associated
   state table entry is overwritten.

A symptom of this can be seen in the DeleteAll(FabricIndex fabricIndex)
method, which will often log errors due to the index being out of sync
form the state table.

This commit fixes the problem by first deleting the link table entry
before saving a record for a node that already exists in the table.
Save is also simplified in this case by not rewriting the index, as
the node is already in the index.

To prevent future problems, unit test code has been added for 
DefaultSessionResumptionStorage to verify that DeleteAll is completing
without errors and that no link or state table entries are leaked.
  • Loading branch information
@msandstedt @pull
msandstedt authored and pull[bot] committed Feb 17, 2023
1 parent 23be3fa commit 32bd976
Show file tree
Hide file tree
Showing 3 changed files with 256 additions and 22 deletions.
38 changes: 38 additions & 0 deletions src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,44 @@ CHIP_ERROR DefaultSessionResumptionStorage::Save(const ScopedNodeId & node, Cons
SessionIndex index;
ReturnErrorOnFailure(LoadIndex(index));

for (size_t i = 0; i < index.mSize; ++i)
{
if (index.mNodes[i] == node)
{
// Node already exists in the index. Save in place.
CHIP_ERROR err = CHIP_NO_ERROR;
ResumptionIdStorage oldResumptionId;
Crypto::P256ECDHDerivedSecret oldSharedSecret;
CATValues oldPeerCATs;
// This follows the approach in Delete. Removal of the old
// resumption-id-keyed link is best effort. If we cannot load
// state to lookup the resumption ID for the key, the entry in
// the link table will be leaked.
err = LoadState(node, oldResumptionId, oldSharedSecret, oldPeerCATs);
if (err != CHIP_NO_ERROR)
{
ChipLogError(SecureChannel,
"LoadState failed; unable to fully delete session resumption record for node " ChipLogFormatX64
": %" CHIP_ERROR_FORMAT,
ChipLogValueX64(node.GetNodeId()), err.Format());
}
else
{
err = DeleteLink(oldResumptionId);
if (err != CHIP_NO_ERROR)
{
ChipLogError(SecureChannel,
"DeleteLink failed; unable to fully delete session resumption record for node " ChipLogFormatX64
": %" CHIP_ERROR_FORMAT,
ChipLogValueX64(node.GetNodeId()), err.Format());
}
}
ReturnErrorOnFailure(SaveState(node, resumptionId, sharedSecret, peerCATs));
ReturnErrorOnFailure(SaveLink(resumptionId, node));
return CHIP_NO_ERROR;
}
}

if (index.mSize == CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE)
{
// TODO: implement LRU for resumption
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/secure_channel/SimpleSessionResumptionStorage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ class SimpleSessionResumptionStorage : public DefaultSessionResumptionStorage
Crypto::P256ECDHDerivedSecret & sharedSecret, CATValues & peerCATs) override;
CHIP_ERROR DeleteState(const ScopedNodeId & node) override;

private:
static const char * StorageKey(DefaultStorageKeyAllocator & keyAlloc, const ScopedNodeId & node);
static const char * StorageKey(DefaultStorageKeyAllocator & keyAlloc, ConstResumptionIdView resumptionId);

private:
static constexpr size_t MaxScopedNodeIdSize() { return TLV::EstimateStructOverhead(sizeof(NodeId), sizeof(FabricIndex)); }

static constexpr size_t MaxIndexSize()
Expand Down
238 changes: 217 additions & 21 deletions src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@
* limitations under the License.
*/

#include <lib/support/DefaultStorageKeyAllocator.h>
#include <lib/support/TestPersistentStorageDelegate.h>
#include <lib/support/UnitTestRegistration.h>
#include <nlunit-test.h>

#include <lib/support/TestPersistentStorageDelegate.h>

// DefaultSessionResumptionStorage is a partial implementation.
// Use SimpleSessionResumptionStorage, which extends it, to test.
#include <protocols/secure_channel/SimpleSessionResumptionStorage.h>

#define ARRAY_SIZE(_array) ((sizeof(_array) / sizeof(_array[0])))

void TestSave(nlTestSuite * inSuite, void * inContext)
{
chip::SimpleSessionResumptionStorage sessionStorage;
Expand All @@ -38,7 +40,7 @@ void TestSave(nlTestSuite * inSuite, void * inContext)
} vectors[CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE + 1];

// Populate test vectors.
for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i)
for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i)
{
NL_TEST_ASSERT(
inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size()));
Expand Down Expand Up @@ -68,7 +70,7 @@ void TestSave(nlTestSuite * inSuite, void * inContext)
// If more sophisticated LRU behavior is implemented, this test
// case should be modified to match.
{
size_t last = sizeof(vectors) / sizeof(vectors[0]) - 1;
size_t last = ARRAY_SIZE(vectors) - 1;
NL_TEST_ASSERT(inSuite,
sessionStorage.Save(vectors[last].node, vectors[last].resumptionId, vectors[last].sharedSecret,
vectors[last].cats) == CHIP_NO_ERROR);
Expand All @@ -81,7 +83,124 @@ void TestSave(nlTestSuite * inSuite, void * inContext)
}

// Read back and verify values.
for (size_t i = 0; i < CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE; ++i)
for (auto & vector : vectors)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
chip::Crypto::P256ECDHDerivedSecret outSharedSecret;
chip::CATValues outCats;

// Verify retrieval by node.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByScopedNodeId(vector.node, outResumptionId, outSharedSecret, outCats) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, memcmp(vector.resumptionId.data(), outResumptionId.data(), vector.resumptionId.size()) == 0);
NL_TEST_ASSERT(inSuite, memcmp(vector.sharedSecret.ConstBytes(), outSharedSecret, vector.sharedSecret.Length()) == 0);
NL_TEST_ASSERT(inSuite, vector.cats.values[0] == outCats.values[0]);
NL_TEST_ASSERT(inSuite, vector.cats.values[1] == outCats.values[1]);
NL_TEST_ASSERT(inSuite, vector.cats.values[2] == outCats.values[2]);

// Validate retreiveal by resumption ID.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByResumptionId(vector.resumptionId, outNode, outSharedSecret, outCats) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, vector.node == outNode);
NL_TEST_ASSERT(inSuite, memcmp(vector.sharedSecret.Bytes(), outSharedSecret, vector.sharedSecret.Length()) == 0);
NL_TEST_ASSERT(inSuite, vector.cats.values[0] == outCats.values[0]);
NL_TEST_ASSERT(inSuite, vector.cats.values[1] == outCats.values[1]);
NL_TEST_ASSERT(inSuite, vector.cats.values[2] == outCats.values[2]);
}
}

void TestInPlaceSave(nlTestSuite * inSuite, void * inContext)
{
chip::SimpleSessionResumptionStorage sessionStorage;
chip::TestPersistentStorageDelegate storage;
sessionStorage.Init(&storage);
struct
{
chip::SessionResumptionStorage::ResumptionIdStorage resumptionId;
chip::Crypto::P256ECDHDerivedSecret sharedSecret;
chip::ScopedNodeId node;
chip::CATValues cats;
} vectors[CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE + 10];

// Construct only a few unique node identities to simiulate talking to a
// couple peers.
chip::ScopedNodeId nodes[3];
static_assert(ARRAY_SIZE(nodes) < CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE,
"must have fewer nodes than slots in session resumption storage");
for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i)
{
do
{
nodes[i] = chip::ScopedNodeId(static_cast<chip::NodeId>(rand()), static_cast<chip::FabricIndex>(i + 1));
} while (!nodes[i].IsOperational());
}

// Populate test vectors.
for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i)
{
NL_TEST_ASSERT(
inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size()));
*vectors[i].resumptionId.data() =
static_cast<uint8_t>(i); // set first byte to our index to ensure uniqueness for the FindByResumptionId call
vectors[i].sharedSecret.SetLength(vectors[i].sharedSecret.Capacity());
NL_TEST_ASSERT(inSuite,
CHIP_NO_ERROR ==
chip::Crypto::DRBG_get_bytes(vectors[i].sharedSecret.Bytes(), vectors[i].sharedSecret.Length()));
vectors[i].node = nodes[i % ARRAY_SIZE(nodes)];
vectors[i].cats.values[0] = static_cast<chip::CASEAuthTag>(rand());
vectors[i].cats.values[1] = static_cast<chip::CASEAuthTag>(rand());
vectors[i].cats.values[2] = static_cast<chip::CASEAuthTag>(rand());
}

// Add one entry for each node.
for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i)
{
NL_TEST_ASSERT(inSuite,
sessionStorage.Save(vectors[i].node, vectors[i].resumptionId, vectors[i].sharedSecret, vectors[i].cats) ==
CHIP_NO_ERROR);
}

// Read back and verify values.
for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
chip::Crypto::P256ECDHDerivedSecret outSharedSecret;
chip::CATValues outCats;

// Verify retrieval by node.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByScopedNodeId(vectors[i].node, outResumptionId, outSharedSecret, outCats) ==
CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite,
memcmp(vectors[i].resumptionId.data(), outResumptionId.data(), vectors[i].resumptionId.size()) == 0);
NL_TEST_ASSERT(inSuite, memcmp(vectors[i].sharedSecret.Bytes(), outSharedSecret, vectors[i].sharedSecret.Length()) == 0);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[0] == outCats.values[0]);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]);

// Validate retreiveal by resumption ID.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByResumptionId(vectors[i].resumptionId, outNode, outSharedSecret, outCats) ==
CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, vectors[i].node == outNode);
NL_TEST_ASSERT(inSuite, memcmp(vectors[i].sharedSecret.Bytes(), outSharedSecret, vectors[i].sharedSecret.Length()) == 0);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[0] == outCats.values[0]);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]);
}

// Now add all test vectors. This should overwrite each node's record
// many times.
for (auto & vector : vectors)
{
NL_TEST_ASSERT(inSuite,
sessionStorage.Save(vector.node, vector.resumptionId, vector.sharedSecret, vector.cats) == CHIP_NO_ERROR);
}

// Read back and verify that only the last record for each node was retained.
for (size_t i = ARRAY_SIZE(vectors) - ARRAY_SIZE(nodes); i < ARRAY_SIZE(vectors); ++i)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
Expand Down Expand Up @@ -109,6 +228,49 @@ void TestSave(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]);
NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]);
}

// Remove all records for all fabrics. If all three tables of (index, state,
// links) are in sync, deleting for each fabric should clean error free.
for (const auto & node : nodes)
{
NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(node.GetFabricIndex()) == CHIP_NO_ERROR);
}

// Verify that no entries can be located any longer for any node or
// resumption ID.
for (auto & vector : vectors)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
chip::Crypto::P256ECDHDerivedSecret outSharedSecret;
chip::CATValues outCats;

// Verify all records for all nodes are gone.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByScopedNodeId(vector.node, outResumptionId, outSharedSecret, outCats) != CHIP_NO_ERROR);

// Verify all records for all resumption IDs are gone.
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByResumptionId(vector.resumptionId, outNode, outSharedSecret, outCats) != CHIP_NO_ERROR);
}

// Verify no state table persistent storage entries were leaked.
for (const auto & node : nodes)
{
uint16_t size = 0;
chip::DefaultStorageKeyAllocator keyAlloc;
auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node), nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
// Verify no link table persistent storage entries were leaked.
for (auto & vector : vectors)
{
uint16_t size = 0;
chip::DefaultStorageKeyAllocator keyAlloc;
auto rv =
storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.resumptionId), nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
}

void TestDelete(nlTestSuite * inSuite, void * inContext)
Expand All @@ -128,7 +290,7 @@ void TestDelete(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(sharedSecret.Bytes(), sharedSecret.Length()));

// Populate test vectors.
for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i)
for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i)
{
NL_TEST_ASSERT(
inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size()));
Expand All @@ -146,7 +308,7 @@ void TestDelete(nlTestSuite * inSuite, void * inContext)
}

// Delete values in turn from storage and verify they are removed.
for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i)
for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
Expand All @@ -160,6 +322,23 @@ void TestDelete(nlTestSuite * inSuite, void * inContext)
sessionStorage.FindByResumptionId(vectors[i].resumptionId, outNode, outSharedSecret, outCats) !=
CHIP_NO_ERROR);
}

// Verify no state or link table persistent storage entries were leaked.
for (auto & vector : vectors)
{
uint16_t size = 0;
chip::DefaultStorageKeyAllocator keyAlloc;
{
auto rv =
storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.node), nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
{
auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.resumptionId),
nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
}
}

void TestDeleteAll(nlTestSuite * inSuite, void * inContext)
Expand Down Expand Up @@ -197,38 +376,54 @@ void TestDeleteAll(nlTestSuite * inSuite, void * inContext)
}

// Fill storage.
for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i)
for (auto & vector : vectors)
{
for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j)
for (auto & node : vector.nodes)
{
NL_TEST_ASSERT(inSuite,
sessionStorage.Save(vectors[i].nodes[j].node, vectors[i].nodes[j].resumptionId, sharedSecret,
chip::CATValues{}) == CHIP_NO_ERROR);
sessionStorage.Save(node.node, node.resumptionId, sharedSecret, chip::CATValues{}) == CHIP_NO_ERROR);
}
}

// Validate Fabric deletion.
for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i)
for (const auto & vector : vectors)
{
chip::ScopedNodeId outNode;
chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId;
chip::Crypto::P256ECDHDerivedSecret outSharedSecret;
chip::CATValues outCats;
// Verify fabric node entries exist.
for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j)
for (const auto & node : vector.nodes)
{
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByScopedNodeId(vectors[i].nodes[j].node, outResumptionId, outSharedSecret, outCats) ==
CHIP_NO_ERROR);
NL_TEST_ASSERT(
inSuite, sessionStorage.FindByScopedNodeId(node.node, outResumptionId, outSharedSecret, outCats) == CHIP_NO_ERROR);
}
// Delete fabric.
NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(vectors[i].fabricIndex) == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(vector.fabricIndex) == CHIP_NO_ERROR);
// Verify fabric node entries no longer exist.
for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j)
for (const auto & node : vector.nodes)
{
NL_TEST_ASSERT(inSuite,
sessionStorage.FindByScopedNodeId(vectors[i].nodes[j].node, outResumptionId, outSharedSecret, outCats) !=
CHIP_NO_ERROR);
NL_TEST_ASSERT(
inSuite, sessionStorage.FindByScopedNodeId(node.node, outResumptionId, outSharedSecret, outCats) != CHIP_NO_ERROR);
}
}
// Verify no state or link table persistent storage entries were leaked.
for (auto & vector : vectors)
{
for (auto & node : vector.nodes)
{
uint16_t size = 0;
chip::DefaultStorageKeyAllocator keyAlloc;
{
auto rv =
storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node.node), nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
{
auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node.resumptionId),
nullptr, size);
NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
}
}
}
}
Expand All @@ -242,6 +437,7 @@ void TestDeleteAll(nlTestSuite * inSuite, void * inContext)
static const nlTest sTests[] =
{
NL_TEST_DEF("TestSave", TestSave),
NL_TEST_DEF("TestInPlaceSave", TestInPlaceSave),
NL_TEST_DEF("TestDelete", TestDelete),
NL_TEST_DEF("TestDeleteAll", TestDeleteAll),

Expand Down

0 comments on commit 32bd976

Please sign in to comment.