From 2e7fae51e174d9119c79e5f61cf040be2f1f1f24 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Wed, 1 May 2024 17:18:19 +1200 Subject: [PATCH 1/5] Fix handling of short discriminator in QRCodeSetupPayloadGenerator If a SetupPayload contains a short discriminator then - isValidQRCodePayload() should return false - trying to generate a QR Code should return INVALID_ARGUMENT - generating with SetAllowInvalidPayload(true) should work (not die) --- .../QRCodeSetupPayloadGenerator.cpp | 9 +++++++-- src/setup_payload/SetupPayload.cpp | 6 +++++- src/setup_payload/tests/TestQRCode.cpp | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index 6ffa2319dadb35..3ee35d2b57c514 100644 --- a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp +++ b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp @@ -173,8 +173,13 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi VerifyOrReturnError(payload.rendezvousInformation.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.rendezvousInformation.Value().Raw(), kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); - ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.discriminator.GetLongValue(), - kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); + + // If the payload is valid then discriminator will always be long, but we shouldn't die if we're + // encoding a short one due to mAllowInvalidPayload being set. Just use the short value in that case. + auto const & pd = payload.discriminator; + uint16_t discriminator = (pd.IsShortDiscriminator()) ? pd.GetShortValue() : pd.GetLongValue(); + ReturnErrorOnFailure( + populateBits(bits.data(), offset, discriminator, kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( populateBits(bits.data(), offset, payload.setUpPINCode, kSetupPINCodeFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure(populateBits(bits.data(), offset, 0, kPaddingFieldLengthInBits, kTotalPayloadDataSizeInBits)); diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp index 3d312cd5846e8d..681de164c28223 100644 --- a/src/setup_payload/SetupPayload.cpp +++ b/src/setup_payload/SetupPayload.cpp @@ -79,7 +79,11 @@ bool PayloadContents::isValidQRCodePayload() const return false; } - // Discriminator validity is enforced by the SetupDiscriminator class. + // General discriminator validity is enforced by the SetupDiscriminator class, but it can't be short for QR a code. + if (discriminator.IsShortDiscriminator()) + { + return false; + } if (setUpPINCode >= 1 << kSetupPINCodeFieldLengthInBits) { diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp index 1ec508578dec30..02071f71351466 100644 --- a/src/setup_payload/tests/TestQRCode.cpp +++ b/src/setup_payload/tests/TestQRCode.cpp @@ -381,6 +381,25 @@ TEST(TestQRCode, TestQRCodeToPayloadGeneration) EXPECT_EQ(result, true); } +TEST(TestQRCode, TestGenerateWithShortDiscriminatorInvalid) +{ + SetupPayload payload = GetDefaultPayload(); + EXPECT_TRUE(payload.isValidQRCodePayload()); + + // A short discriminator isn't valid for a QR Code + payload.discriminator.SetShortValue(1); + EXPECT_FALSE(payload.isValidQRCodePayload()); + + // QRCodeSetupPayloadGenerator should therefore return an error + string base38Rep; + QRCodeSetupPayloadGenerator generator(payload); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT); + + // If we allow invalid payloads we should be able to encode + generator.SetAllowInvalidPayload(true); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR); +} + TEST(TestQRCode, TestExtractPayload) { EXPECT_EQ(QRCodeSetupPayloadParser::ExtractPayload(string("MT:ABC")), string("ABC")); From 5f567918a65c1c06ab4ed6d34185b232d736a344 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Wed, 1 May 2024 17:21:01 +1200 Subject: [PATCH 2/5] SetupPayload tweaks Make IsCommonTag, IsVendorTag, and getOptionalVendorData(tag, &info) public. The first two are just encoding spec rules that are useful for clients, and the latter allows clients to read vendor data by tag instead of having to read the whole list. Also use default values instead of explicit constructors for OptionalQRCodeInfo and fix up some doc comments to correctly reference the vendor tag range. --- src/setup_payload/SetupPayload.cpp | 12 -------- src/setup_payload/SetupPayload.h | 49 ++++++++++++------------------ 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp index 681de164c28223..063456327f6a62 100644 --- a/src/setup_payload/SetupPayload.cpp +++ b/src/setup_payload/SetupPayload.cpp @@ -33,18 +33,6 @@ namespace chip { -// Spec 5.1.4.2 CHIPCommon tag numbers are in the range [0x00, 0x7F] -bool SetupPayload::IsCommonTag(uint8_t tag) -{ - return tag < 0x80; -} - -// Spec 5.1.4.1 Manufacture-specific tag numbers are in the range [0x80, 0xFF] -bool SetupPayload::IsVendorTag(uint8_t tag) -{ - return !IsCommonTag(tag); -} - // Check the Setup Payload for validity // // `vendor_id` and `product_id` are allowed all of uint16_t diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h index 9b18574480ba6e..6e81e0e0e48ddb 100644 --- a/src/setup_payload/SetupPayload.h +++ b/src/setup_payload/SetupPayload.h @@ -153,29 +153,19 @@ enum optionalQRCodeInfoType */ struct OptionalQRCodeInfo { - OptionalQRCodeInfo() { int32 = 0; } - /*@{*/ uint8_t tag; /**< the tag number of the optional info */ enum optionalQRCodeInfoType type; /**< the type (String or Int) of the optional info */ std::string data; /**< the string value if type is optionalQRCodeInfoTypeString, otherwise should not be set */ - int32_t int32; /**< the integer value if type is optionalQRCodeInfoTypeInt, otherwise should not be set */ + int32_t int32 = 0; /**< the integer value if type is optionalQRCodeInfoTypeInt32, otherwise should not be set */ /*@}*/ }; struct OptionalQRCodeInfoExtension : OptionalQRCodeInfo { - OptionalQRCodeInfoExtension() - { - int32 = 0; - int64 = 0; - uint32 = 0; - uint64 = 0; - } - - int64_t int64; - uint64_t uint32; - uint64_t uint64; + int64_t int64 = 0; /**< the integer value if type is optionalQRCodeInfoTypeInt64, otherwise should not be set */ + uint64_t uint32 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt32, otherwise should not be set */ + uint64_t uint64 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt64, otherwise should not be set */ }; class SetupPayload : public PayloadContents @@ -193,17 +183,25 @@ class SetupPayload : public PayloadContents CHIP_ERROR addOptionalVendorData(uint8_t tag, std::string data); /** @brief A function to add an optional vendor data - * @param tag 7 bit [0-127] tag number + * @param tag tag number in the [0x80-0xFF] range * @param data Integer representation of data to add * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise **/ CHIP_ERROR addOptionalVendorData(uint8_t tag, int32_t data); /** @brief A function to remove an optional vendor data - * @param tag 7 bit [0-127] tag number + * @param tag tag number in the [0x80-0xFF] range * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise **/ CHIP_ERROR removeOptionalVendorData(uint8_t tag); + + /** @brief A function to retrieve an optional QR Code info vendor object + * @param tag tag number in the [0x80-0xFF] range + * @param info retrieved OptionalQRCodeInfo object + * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise + **/ + CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const; + /** * @brief A function to retrieve the vector of OptionalQRCodeInfo infos * @return Returns a vector of optionalQRCodeInfos @@ -235,21 +233,21 @@ class SetupPayload : public PayloadContents bool operator==(const SetupPayload & input) const; -private: - std::map optionalVendorData; - std::map optionalExtensionData; - /** @brief Checks if the tag is CHIP Common type * @param tag Tag to be checked * @return Returns True if the tag is of Common type **/ - static bool IsCommonTag(uint8_t tag); + static bool IsCommonTag(uint8_t tag) { return tag < 0x80; } /** @brief Checks if the tag is vendor-specific * @param tag Tag to be checked * @return Returns True if the tag is Vendor-specific **/ - static bool IsVendorTag(uint8_t tag); + static bool IsVendorTag(uint8_t tag) { return !IsCommonTag(tag); } + +private: + std::map optionalVendorData; + std::map optionalExtensionData; /** @brief A function to add an optional QR Code info vendor object * @param info Optional QR code info object to add @@ -269,13 +267,6 @@ class SetupPayload : public PayloadContents **/ std::vector getAllOptionalExtensionData() const; - /** @brief A function to retrieve an optional QR Code info vendor object - * @param tag 7 bit [0-127] tag number - * @param info retrieved OptionalQRCodeInfo object - * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise - **/ - CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const; - /** @brief A function to retrieve an optional QR Code info extended object * @param tag 8 bit [128-255] tag number * @param info retrieved OptionalQRCodeInfoExtension object From 61a43e798037570fdfcffae41006e6cf1834a1a1 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 2 May 2024 11:15:21 +1200 Subject: [PATCH 3/5] Address review comments Elaborate on the use case for AllowInvalidPayload in the comments, and encode a missing long discriminator as 0, to avoid a client encoding invalid payloads from relying on round-tripping a short discriminator through a QR code. --- src/setup_payload/QRCodeSetupPayloadGenerator.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index 3ee35d2b57c514..128113d3f40a4b 100644 --- a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp +++ b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp @@ -174,10 +174,12 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.rendezvousInformation.Value().Raw(), kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); - // If the payload is valid then discriminator will always be long, but we shouldn't die if we're - // encoding a short one due to mAllowInvalidPayload being set. Just use the short value in that case. + // isValidQRCodePayload() ensures that we have a long discriminator, but if AllowInvalidPayload is set + // we might have only a short one, and calling GetLongValue() would die. So check for this case here + // and use a placeholder value of 0. Encoding an invalid (or partially valid) payload is useful for + // clients that need to be able to serialize and deserialize partially populated or invalid payloads. auto const & pd = payload.discriminator; - uint16_t discriminator = (pd.IsShortDiscriminator()) ? pd.GetShortValue() : pd.GetLongValue(); + uint16_t discriminator = !pd.IsShortDiscriminator() ? pd.GetLongValue() : 0; ReturnErrorOnFailure( populateBits(bits.data(), offset, discriminator, kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( From a383e3900c42a2af15f458547e12a53a99a4c0e7 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 2 May 2024 12:13:22 +1200 Subject: [PATCH 4/5] Handle rendezvousInformation and discriminator the same way --- .../QRCodeSetupPayloadGenerator.cpp | 23 +++++++++---------- src/setup_payload/tests/TestQRCode.cpp | 19 +++++++++++++++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index 128113d3f40a4b..b0d00e8e9adf7b 100644 --- a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp +++ b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp @@ -162,6 +162,11 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi size_t totalPayloadSizeInBits = kTotalPayloadDataSizeInBits + (tlvDataLengthInBytes * 8); VerifyOrReturnError(bits.size() * 8 >= totalPayloadSizeInBits, CHIP_ERROR_BUFFER_TOO_SMALL); + // isValidQRCodePayload() has already performed all relevant checks (including that we have a + // long discriminator and rendevouz information). But if AllowInvalidPayload is set these + // requirements might be violated; in that case simply encode 0 for the relevant fields. + // Encoding an invalid (or partially valid) payload is useful for clients that need to be able + // to serialize and deserialize partially populated or invalid payloads. ReturnErrorOnFailure( populateBits(bits.data(), offset, payload.version, kVersionFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( @@ -170,18 +175,12 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi populateBits(bits.data(), offset, payload.productID, kProductIDFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure(populateBits(bits.data(), offset, static_cast(payload.commissioningFlow), kCommissioningFlowFieldLengthInBits, kTotalPayloadDataSizeInBits)); - VerifyOrReturnError(payload.rendezvousInformation.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.rendezvousInformation.Value().Raw(), - kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); - - // isValidQRCodePayload() ensures that we have a long discriminator, but if AllowInvalidPayload is set - // we might have only a short one, and calling GetLongValue() would die. So check for this case here - // and use a placeholder value of 0. Encoding an invalid (or partially valid) payload is useful for - // clients that need to be able to serialize and deserialize partially populated or invalid payloads. - auto const & pd = payload.discriminator; - uint16_t discriminator = !pd.IsShortDiscriminator() ? pd.GetLongValue() : 0; - ReturnErrorOnFailure( - populateBits(bits.data(), offset, discriminator, kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); + auto const & ri = payload.rendezvousInformation; + ReturnErrorOnFailure(populateBits(bits.data(), offset, (ri.HasValue() ? ri.Value().Raw() : 0), kRendezvousInfoFieldLengthInBits, + kTotalPayloadDataSizeInBits)); + auto const & pd = payload.discriminator; + ReturnErrorOnFailure(populateBits(bits.data(), offset, (!pd.IsShortDiscriminator() ? pd.GetLongValue() : 0), + kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure( populateBits(bits.data(), offset, payload.setUpPINCode, kSetupPINCodeFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure(populateBits(bits.data(), offset, 0, kPaddingFieldLengthInBits, kTotalPayloadDataSizeInBits)); diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp index 02071f71351466..6ca349c2884341 100644 --- a/src/setup_payload/tests/TestQRCode.cpp +++ b/src/setup_payload/tests/TestQRCode.cpp @@ -400,6 +400,25 @@ TEST(TestQRCode, TestGenerateWithShortDiscriminatorInvalid) EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR); } +TEST(TestQRCode, TestGenerateWithoutRendezvousInformation) +{ + SetupPayload payload = GetDefaultPayload(); + EXPECT_TRUE(payload.isValidQRCodePayload()); + + // Rendezvouz Information is required for a QR code + payload.rendezvousInformation.ClearValue(); + EXPECT_FALSE(payload.isValidQRCodePayload()); + + // QRCodeSetupPayloadGenerator should therefore return an error + string base38Rep; + QRCodeSetupPayloadGenerator generator(payload); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT); + + // If we allow invalid payloads we should be able to encode + generator.SetAllowInvalidPayload(true); + EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR); +} + TEST(TestQRCode, TestExtractPayload) { EXPECT_EQ(QRCodeSetupPayloadParser::ExtractPayload(string("MT:ABC")), string("ABC")); From 0dcae2fa822c8a59bddac82f3b59a75e60a5393e Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Fri, 3 May 2024 10:21:32 +1200 Subject: [PATCH 5/5] Address review comment: use ValueOr --- src/setup_payload/QRCodeSetupPayloadGenerator.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index b0d00e8e9adf7b..962a0393736683 100644 --- a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp +++ b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp @@ -175,9 +175,9 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi populateBits(bits.data(), offset, payload.productID, kProductIDFieldLengthInBits, kTotalPayloadDataSizeInBits)); ReturnErrorOnFailure(populateBits(bits.data(), offset, static_cast(payload.commissioningFlow), kCommissioningFlowFieldLengthInBits, kTotalPayloadDataSizeInBits)); - auto const & ri = payload.rendezvousInformation; - ReturnErrorOnFailure(populateBits(bits.data(), offset, (ri.HasValue() ? ri.Value().Raw() : 0), kRendezvousInfoFieldLengthInBits, - kTotalPayloadDataSizeInBits)); + ReturnErrorOnFailure(populateBits(bits.data(), offset, + payload.rendezvousInformation.ValueOr(RendezvousInformationFlag::kNone).Raw(), + kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); auto const & pd = payload.discriminator; ReturnErrorOnFailure(populateBits(bits.data(), offset, (!pd.IsShortDiscriminator() ? pd.GetLongValue() : 0), kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits));