From 553870dc9c609978ab18a61a5f8bf767c3fd10cc Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Wed, 17 Nov 2021 17:06:31 -0500 Subject: [PATCH] Make device attestation data parser future proof DeviceAttestationDeconstructor was too stringent in expecting no unknown tags at all, preventing future tags from breaking backward compatibility when they are benign if ignored. - Fix DeviceAttestationDeconstructor to skip unexpected tags - Simplify logic for ensuring in-order tags - Fix a corner case of the vendor-specific data iterator not caught before - Clean-up extraction of profileNum in CHIP tags Fixes #11247 --- .../DeviceAttestationConstructor.cpp | 46 ++++---- .../DeviceAttestationVendorReserved.h | 44 ++++--- .../TestDeviceAttestationConstruction.cpp | 108 +++++++++++------- src/lib/core/CHIPTLVTags.h | 6 +- 4 files changed, 124 insertions(+), 80 deletions(-) diff --git a/src/credentials/DeviceAttestationConstructor.cpp b/src/credentials/DeviceAttestationConstructor.cpp index eb206f2f56aa43..4d301dc6852f82 100644 --- a/src/credentials/DeviceAttestationConstructor.cpp +++ b/src/credentials/DeviceAttestationConstructor.cpp @@ -67,9 +67,9 @@ CHIP_ERROR DeconstructAttestationElements(const ByteSpan & attestationElements, { bool certificationDeclarationExists = false; bool attestationNonceExists = false; - bool timestampExists = false; - bool firmwareInfoExists = false; - uint32_t lastContextTagId = UINT32_MAX; + bool gotFirstContextTag = false; + uint32_t lastContextTagId = 0; + TLV::ContiguousBufferTLVReader tlvReader; TLV::TLVType containerType = TLV::kTLVType_Structure; @@ -85,47 +85,53 @@ CHIP_ERROR DeconstructAttestationElements(const ByteSpan & attestationElements, while ((error = tlvReader.Next()) == CHIP_NO_ERROR) { TLV::Tag tag = tlvReader.GetTag(); - if (!TLV::IsContextTag(tag)) + { break; + } - switch (TLV::TagNumFromTag(tag)) + // Ensure tag-order and correct first expected tag + uint32_t contextTagId = TLV::TagNumFromTag(tag); + if (!gotFirstContextTag) + { + // First tag must always be Certification Declaration + VerifyOrReturnError(contextTagId == kCertificationDeclarationTagId, CHIP_ERROR_UNEXPECTED_TLV_ELEMENT); + gotFirstContextTag = true; + } + else + { + // Subsequent tags must always be in order + VerifyOrReturnError(contextTagId > lastContextTagId, CHIP_ERROR_UNEXPECTED_TLV_ELEMENT); + } + lastContextTagId = contextTagId; + + switch (contextTagId) { case kCertificationDeclarationTagId: - VerifyOrReturnError(lastContextTagId == UINT32_MAX, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); - VerifyOrReturnError(certificationDeclarationExists == false, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); ReturnErrorOnFailure(tlvReader.GetByteView(certificationDeclaration)); certificationDeclarationExists = true; break; case kAttestationNonceTagId: - VerifyOrReturnError(lastContextTagId == kCertificationDeclarationTagId, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); - VerifyOrReturnError(attestationNonceExists == false, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); ReturnErrorOnFailure(tlvReader.GetByteView(attestationNonce)); attestationNonceExists = true; break; case kTimestampTagId: - VerifyOrReturnError(lastContextTagId == kAttestationNonceTagId, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); - VerifyOrReturnError(timestampExists == false, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); ReturnErrorOnFailure(tlvReader.Get(timestamp)); - timestampExists = true; break; case kFirmwareInfoTagId: - VerifyOrReturnError(lastContextTagId == kTimestampTagId, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); - VerifyOrReturnError(firmwareInfoExists == false, CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); ReturnErrorOnFailure(tlvReader.GetByteView(firmwareInfo)); - firmwareInfoExists = true; break; default: - return CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT; + // It's OK to have future context tags before vendor specific tags. + // We already checked that the tags are in order. + break; } - - lastContextTagId = TLV::TagNumFromTag(tag); } VerifyOrReturnError(error == CHIP_NO_ERROR || error == CHIP_END_OF_TLV, error); - VerifyOrReturnError(lastContextTagId == kTimestampTagId || lastContextTagId == kFirmwareInfoTagId, - CHIP_ERROR_MISSING_TLV_ELEMENT); + const bool allTagsNeededPresent = certificationDeclarationExists && attestationNonceExists; + VerifyOrReturnError(allTagsNeededPresent, CHIP_ERROR_MISSING_TLV_ELEMENT); size_t count = 0; ReturnErrorOnFailure(CountVendorReservedElementsInDA(attestationElements, count)); diff --git a/src/credentials/DeviceAttestationVendorReserved.h b/src/credentials/DeviceAttestationVendorReserved.h index 576ff2dfff3e22..ead4a5893cb4ce 100644 --- a/src/credentials/DeviceAttestationVendorReserved.h +++ b/src/credentials/DeviceAttestationVendorReserved.h @@ -40,7 +40,7 @@ class DeviceAttestationVendorReservedDeconstructor public: DeviceAttestationVendorReservedDeconstructor() {} - // read TLV until first profile tag + // Read TLV until first profile tag CHIP_ERROR PrepareToReadVendorReservedElements(const ByteSpan & attestationElements, size_t count) { mIsInitialized = false; @@ -55,11 +55,14 @@ class DeviceAttestationVendorReservedDeconstructor while (true) { ReturnErrorOnFailure(mTlvReader.Next()); - if (!TLV::IsProfileTag(mTlvReader.GetTag())) + + TLV::Tag tag = mTlvReader.GetTag(); + if (!TLV::IsContextTag(tag)) break; } // positioned to first context tag (vendor reserved data) mIsInitialized = true; + mIsAtFirstToken = true; return CHIP_NO_ERROR; } @@ -72,36 +75,45 @@ class DeviceAttestationVendorReservedDeconstructor * * @returns CHIP_NO_ERROR on success * CHIP_ERROR_INCORRECT_STATE if PrepareToReadVendorReservedElements hasn't been called first + * CHIP_ERROR_UNEXPECTED_TLV_ELEMENT if we reach non-profile-specific tags or vendorId is zero * CHIP_END_OF_TLV if not further entries are present */ CHIP_ERROR GetNextVendorReservedElement(struct VendorReservedElement & element) { - CHIP_ERROR err; - VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE); - while ((err = mTlvReader.Next()) == CHIP_NO_ERROR) + if (mIsAtFirstToken) { - uint64_t tag = mTlvReader.GetTag(); - if (!TLV::IsProfileTag(tag)) - { - continue; - } - // tag is profile tag - element.vendorId = TLV::VendorIdFromTag(tag); - element.profileNum = TLV::ProfileNumFromTag(tag); - element.tagNum = TLV::TagNumFromTag(tag); + // Already had a Next() done for us by PrepareToReadVendorReservedElements + // so we don't Next() since we should be pointing at a vendor-reserved. + mIsAtFirstToken = false; + } + else + { + ReturnErrorOnFailure(mTlvReader.Next()); + } - return mTlvReader.GetByteView(element.vendorReservedData); + TLV::Tag tag = mTlvReader.GetTag(); + if (!TLV::IsProfileTag(tag)) + { + return CHIP_ERROR_UNEXPECTED_TLV_ELEMENT; } - return err; + // tag is profile tag + element.vendorId = TLV::VendorIdFromTag(tag); + element.profileNum = TLV::ProfileNumFromTag(tag); + element.tagNum = TLV::TagNumFromTag(tag); + + ReturnErrorOnFailure(mTlvReader.GetByteView(element.vendorReservedData)); + + return CHIP_NO_ERROR; } private: size_t mNumVendorReservedData; // number of VendorReserved entries (could be 0) ByteSpan mAttestationData; bool mIsInitialized = false; + bool mIsAtFirstToken = false; TLV::ContiguousBufferTLVReader mTlvReader; TLV::TLVType containerType = TLV::kTLVType_Structure; }; diff --git a/src/credentials/tests/TestDeviceAttestationConstruction.cpp b/src/credentials/tests/TestDeviceAttestationConstruction.cpp index f75a183ce316b1..ef67db0f215245 100644 --- a/src/credentials/tests/TestDeviceAttestationConstruction.cpp +++ b/src/credentials/tests/TestDeviceAttestationConstruction.cpp @@ -180,9 +180,8 @@ static void TestAttestationElements_Construction(nlTestSuite * inSuite, void * i static void TestAttestationElements_Deconstruction(nlTestSuite * inSuite, void * inContext) { - CHIP_ERROR err = CHIP_NO_ERROR; - - static constexpr uint8_t attestationElementsTestVector[] = { + // This is a test case with only the known TLV tags fields + constexpr uint8_t attestationElementsTestVectorOnlyKnownTags[] = { 0x15, 0x30, 0x01, 0x70, 0xd2, 0x84, 0x4b, 0xa2, 0x01, 0x26, 0x04, 0x46, 0x63, 0x73, 0x61, 0x63, 0x64, 0x30, 0xa0, 0x58, 0x1d, 0x15, 0x25, 0x01, 0x88, 0x99, 0x25, 0x02, 0xfe, 0xff, 0x25, 0x03, 0xd2, 0x04, 0x25, 0x04, 0x2e, 0x16, 0x24, 0x05, 0xaa, 0x25, 0x06, 0xde, 0xc0, 0x25, 0x07, 0x94, 0x26, 0x18, 0x58, 0x40, 0x96, 0x57, 0x2d, 0xd6, 0x3c, @@ -196,7 +195,25 @@ static void TestAttestationElements_Deconstruction(nlTestSuite * inSuite, void * 0xff, 0x3e, 0x00, 0x03, 0x00, 0x18, 0x76, 0x65, 0x6e, 0x64, 0x6f, 0x72, 0x5f, 0x72, 0x65, 0x73, 0x65, 0x72, 0x76, 0x65, 0x64, 0x33, 0x5f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x18 }; - static constexpr uint8_t certificationDeclarationTestVector[] = { + + // The following case has an extra top-level context-specific tag (254) with value 0xDEADBEEF, in the right order, + // that should not cause a failure and merely be ignored. + constexpr uint8_t attestationElementsTestVectorWithOneUnknownTag[] = { + 0x15, 0x30, 0x01, 0x70, 0xd2, 0x84, 0x4b, 0xa2, 0x01, 0x26, 0x04, 0x46, 0x63, 0x73, 0x61, 0x63, 0x64, 0x30, 0xa0, + 0x58, 0x1d, 0x15, 0x25, 0x01, 0x88, 0x99, 0x25, 0x02, 0xfe, 0xff, 0x25, 0x03, 0xd2, 0x04, 0x25, 0x04, 0x2e, 0x16, + 0x24, 0x05, 0xaa, 0x25, 0x06, 0xde, 0xc0, 0x25, 0x07, 0x94, 0x26, 0x18, 0x58, 0x40, 0x96, 0x57, 0x2d, 0xd6, 0x3c, + 0x03, 0x64, 0x0b, 0x28, 0x67, 0x02, 0xbd, 0x6b, 0xba, 0x48, 0xac, 0x7c, 0x83, 0x54, 0x9b, 0x68, 0x73, 0x29, 0x47, + 0x48, 0xb9, 0x51, 0xd5, 0xab, 0x66, 0x62, 0x2e, 0x9d, 0x26, 0x10, 0x41, 0xf8, 0x0e, 0x97, 0x49, 0xfe, 0xff, 0x78, + 0x10, 0x02, 0x49, 0x67, 0xae, 0xdf, 0x41, 0x38, 0x36, 0x5b, 0x0a, 0x22, 0x57, 0x14, 0x9c, 0x9a, 0x12, 0x3e, 0x0d, + 0x30, 0xaa, 0x30, 0x02, 0x20, 0xe0, 0x42, 0x1b, 0x91, 0xc6, 0xfd, 0xcd, 0xb4, 0x0e, 0x2a, 0x4d, 0x2c, 0xf3, 0x1d, + 0xb2, 0xb4, 0xe1, 0x8b, 0x41, 0x1b, 0x1d, 0x3a, 0xd4, 0xd1, 0x2a, 0x9d, 0x90, 0xaa, 0x8e, 0x52, 0xfa, 0xe2, 0x26, + 0x03, 0xfd, 0xc6, 0x5b, 0x28, 0x26, 0xfe, 0xef, 0xbe, 0xad, 0xde, 0xd0, 0xf1, 0xff, 0x3e, 0x00, 0x01, 0x00, 0x17, + 0x73, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x5f, 0x76, 0x65, 0x6e, 0x64, 0x6f, 0x72, 0x5f, 0x72, 0x65, 0x73, 0x65, 0x72, + 0x76, 0x65, 0x64, 0x31, 0xd0, 0xf1, 0xff, 0x3e, 0x00, 0x03, 0x00, 0x18, 0x76, 0x65, 0x6e, 0x64, 0x6f, 0x72, 0x5f, + 0x72, 0x65, 0x73, 0x65, 0x72, 0x76, 0x65, 0x64, 0x33, 0x5f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x18, + }; + + constexpr uint8_t certificationDeclarationTestVector[] = { 0xd2, 0x84, 0x4b, 0xa2, 0x01, 0x26, 0x04, 0x46, 0x63, 0x73, 0x61, 0x63, 0x64, 0x30, 0xa0, 0x58, 0x1d, 0x15, 0x25, 0x01, 0x88, 0x99, 0x25, 0x02, 0xfe, 0xff, 0x25, 0x03, 0xd2, 0x04, 0x25, 0x04, 0x2e, 0x16, 0x24, 0x05, 0xaa, 0x25, 0x06, 0xde, 0xc0, 0x25, 0x07, 0x94, 0x26, 0x18, 0x58, 0x40, 0x96, 0x57, 0x2d, 0xd6, 0x3c, 0x03, 0x64, 0x0b, 0x28, @@ -204,54 +221,61 @@ static void TestAttestationElements_Deconstruction(nlTestSuite * inSuite, void * 0xab, 0x66, 0x62, 0x2e, 0x9d, 0x26, 0x10, 0x41, 0xf8, 0x0e, 0x97, 0x49, 0xfe, 0xff, 0x78, 0x10, 0x02, 0x49, 0x67, 0xae, 0xdf, 0x41, 0x38, 0x36, 0x5b, 0x0a, 0x22, 0x57, 0x14, 0x9c, 0x9a, 0x12, 0x3e, 0x0d, 0x30, 0xaa }; - static constexpr uint8_t attestationNonceTestVector[] = { 0xe0, 0x42, 0x1b, 0x91, 0xc6, 0xfd, 0xcd, 0xb4, 0x0e, 0x2a, 0x4d, + constexpr uint8_t attestationNonceTestVector[] = { 0xe0, 0x42, 0x1b, 0x91, 0xc6, 0xfd, 0xcd, 0xb4, 0x0e, 0x2a, 0x4d, 0x2c, 0xf3, 0x1d, 0xb2, 0xb4, 0xe1, 0x8b, 0x41, 0x1b, 0x1d, 0x3a, 0xd4, 0xd1, 0x2a, 0x9d, 0x90, 0xaa, 0x8e, 0x52, 0xfa, 0xe2 }; - static constexpr uint32_t timestampTestVector = 677103357; - static constexpr uint8_t vendorReserved1TestVector[] = { 0x73, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x5f, 0x76, 0x65, 0x6e, 0x64, 0x6f, + constexpr uint32_t timestampTestVector = 677103357; + constexpr uint8_t vendorReserved1TestVector[] = { 0x73, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x5f, 0x76, 0x65, 0x6e, 0x64, 0x6f, 0x72, 0x5f, 0x72, 0x65, 0x73, 0x65, 0x72, 0x76, 0x65, 0x64, 0x31 }; - static constexpr uint8_t vendorReserved3TestVector[] = { + constexpr uint8_t vendorReserved3TestVector[] = { 0x76, 0x65, 0x6e, 0x64, 0x6f, 0x72, 0x5f, 0x72, 0x65, 0x73, 0x65, 0x72, 0x76, 0x65, 0x64, 0x33, 0x5f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65 }; - static constexpr ByteSpan vendorReservedArrayTestVector[] = { ByteSpan(vendorReserved1TestVector), - ByteSpan(vendorReserved3TestVector) }; - static constexpr uint16_t vendorIdTestVector = 0xFFF1; - static constexpr uint16_t profileNumTestVector = 0x003E; + const ByteSpan vendorReservedArrayTestVector[] = { ByteSpan{vendorReserved1TestVector}, + ByteSpan{vendorReserved3TestVector} }; + constexpr uint16_t vendorIdTestVector = 0xFFF1; + constexpr uint16_t profileNumTestVector = 0x003E; - ByteSpan certificationDeclarationDeconstructed; - ByteSpan attestationNonceDeconstructed; - uint32_t timestampDeconstructed; - ByteSpan firmwareInfoDeconstructed; - DeviceAttestationVendorReservedDeconstructor vendorReserved; + const ByteSpan kTestCases[] = { ByteSpan{attestationElementsTestVectorOnlyKnownTags}, ByteSpan{attestationElementsTestVectorWithOneUnknownTag} }; - err = DeconstructAttestationElements(ByteSpan(attestationElementsTestVector), certificationDeclarationDeconstructed, - attestationNonceDeconstructed, timestampDeconstructed, firmwareInfoDeconstructed, - vendorReserved); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + // Try all test cases and see they are all OK + for (const ByteSpan & attestationElementsTestCase : kTestCases) + { + CHIP_ERROR err = CHIP_NO_ERROR; + ByteSpan certificationDeclarationDeconstructed; + ByteSpan attestationNonceDeconstructed; + uint32_t timestampDeconstructed; + ByteSpan firmwareInfoDeconstructed; + DeviceAttestationVendorReservedDeconstructor vendorReserved; + + err = DeconstructAttestationElements(attestationElementsTestCase, certificationDeclarationDeconstructed, + attestationNonceDeconstructed, timestampDeconstructed, firmwareInfoDeconstructed, + vendorReserved); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(inSuite, certificationDeclarationDeconstructed.data_equal(ByteSpan(certificationDeclarationTestVector))); - NL_TEST_ASSERT(inSuite, attestationNonceDeconstructed.data_equal(ByteSpan(attestationNonceTestVector))); - NL_TEST_ASSERT(inSuite, timestampTestVector == timestampDeconstructed); - NL_TEST_ASSERT(inSuite, firmwareInfoDeconstructed.empty()); - NL_TEST_ASSERT(inSuite, ArraySize(vendorReservedArrayTestVector) == vendorReserved.GetNumberOfElements()); - struct VendorReservedElement element; + NL_TEST_ASSERT(inSuite, certificationDeclarationDeconstructed.data_equal(ByteSpan(certificationDeclarationTestVector))); + NL_TEST_ASSERT(inSuite, attestationNonceDeconstructed.data_equal(ByteSpan(attestationNonceTestVector))); + NL_TEST_ASSERT(inSuite, timestampTestVector == timestampDeconstructed); + NL_TEST_ASSERT(inSuite, firmwareInfoDeconstructed.empty()); + NL_TEST_ASSERT(inSuite, ArraySize(vendorReservedArrayTestVector) == vendorReserved.GetNumberOfElements()); + struct VendorReservedElement element; - while (vendorReserved.GetNextVendorReservedElement(element) == CHIP_NO_ERROR) - { - NL_TEST_ASSERT(inSuite, vendorIdTestVector == element.vendorId); - NL_TEST_ASSERT(inSuite, profileNumTestVector == element.profileNum); - switch (element.tagNum) + while (vendorReserved.GetNextVendorReservedElement(element) == CHIP_NO_ERROR) { - case 1: - NL_TEST_ASSERT(inSuite, element.vendorReservedData.data_equal(vendorReservedArrayTestVector[0])); - break; - case 3: - NL_TEST_ASSERT(inSuite, element.vendorReservedData.data_equal(vendorReservedArrayTestVector[1])); - break; - default: - NL_TEST_ASSERT(inSuite, 0); - break; + NL_TEST_ASSERT(inSuite, vendorIdTestVector == element.vendorId); + NL_TEST_ASSERT(inSuite, profileNumTestVector == element.profileNum); + switch (element.tagNum) + { + case 1: + NL_TEST_ASSERT(inSuite, element.vendorReservedData.data_equal(vendorReservedArrayTestVector[0])); + break; + case 3: + NL_TEST_ASSERT(inSuite, element.vendorReservedData.data_equal(vendorReservedArrayTestVector[1])); + break; + default: + NL_TEST_ASSERT(inSuite, 0); + break; + } } } } @@ -424,7 +448,7 @@ static void TestAttestationElements_DeconstructionUnordered(nlTestSuite * inSuit err = DeconstructAttestationElements(ByteSpan(attestationElementsUnorderedTestVector), certificationDeclarationDeconstructed, attestationNonceDeconstructed, timestampDeconstructed, firmwareInfoDeconstructed, vendorReserved); - NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_ELEMENT); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_UNEXPECTED_TLV_ELEMENT); } /** diff --git a/src/lib/core/CHIPTLVTags.h b/src/lib/core/CHIPTLVTags.h index 7b49cea44ca4ea..984d54845d99c2 100644 --- a/src/lib/core/CHIPTLVTags.h +++ b/src/lib/core/CHIPTLVTags.h @@ -47,6 +47,8 @@ enum TLVCommonProfiles enum TLVTagFields { kProfileIdMask = 0xFFFFFFFF00000000ULL, + kProfileNumMask = 0x0000FFFF00000000ULL, + kVendorIdMask = 0xFFFF000000000000ULL, kProfileIdShift = 32, kVendorIdShift = 48, kProfileNumShift = 32, @@ -165,7 +167,7 @@ inline uint32_t ProfileIdFromTag(Tag tag) */ inline uint16_t ProfileNumFromTag(Tag tag) { - return static_cast((tag & kProfileIdMask) >> kProfileIdShift); + return static_cast((tag & kProfileNumMask) >> kProfileIdShift); } /** @@ -194,7 +196,7 @@ inline uint32_t TagNumFromTag(Tag tag) */ inline uint16_t VendorIdFromTag(Tag tag) { - return static_cast((tag & kProfileIdMask) >> kVendorIdShift); + return static_cast((tag & kVendorIdMask) >> kVendorIdShift); } /**