From fcfc9bcc85afa4054cf84cbc1606b64eb305b94d Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Fri, 3 May 2024 12:13:52 +1200 Subject: [PATCH] Fix handling of short discriminator in QRCodeSetupPayloadGenerator (#33250) * 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) * 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. * 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. * Handle rendezvousInformation and discriminator the same way * Address review comment: use ValueOr --- .../QRCodeSetupPayloadGenerator.cpp | 12 +++-- src/setup_payload/SetupPayload.cpp | 18 ++----- src/setup_payload/SetupPayload.h | 49 ++++++++----------- src/setup_payload/tests/TestQRCode.cpp | 38 ++++++++++++++ 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp index 6ffa2319dadb35..962a0393736683 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,10 +175,11 @@ 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(), + ReturnErrorOnFailure(populateBits(bits.data(), offset, + payload.rendezvousInformation.ValueOr(RendezvousInformationFlag::kNone).Raw(), kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits)); - ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.discriminator.GetLongValue(), + 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)); diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp index 3d312cd5846e8d..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 @@ -79,7 +67,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/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 diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp index 1ec508578dec30..6ca349c2884341 100644 --- a/src/setup_payload/tests/TestQRCode.cpp +++ b/src/setup_payload/tests/TestQRCode.cpp @@ -381,6 +381,44 @@ 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, 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"));