Fix corner cases of handling of Common Name fallback encoding (#28911)

* Fix corner cases of handling of Common Name fallback encoding Problem: - Appearance of a Mpid:/Mvid: in a DAC/PAI/PAA DN was deemed OK by previous code, but this caused a critical ambiguity in PAIs which would possibly cause fall-back to non-PID-scoped PAI interpretation. - Related to https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/7470 - Fixes #28898 This PR: - Replaces the logic for fallback encodign conversion to take the first legitimate fully matching case for Mvid: and Mpid: and detect errors where either of these is present but without a following Mpid/Mvid. - Updates unit tests to improve coverage and to properly mark as invalid some cases marked invalid in spec which where deemed valid by prior code by mistake Testing done: - Integration tests still pass (relater to Commissioner DUT). - Test vectors updated. - New unit tests added. * Restyled by clang-format * Restyled by prettier-json * Address review comments by revamping algorithm * Fix leftover comment follow-ups from @bzbarsky-apple from #28899 * Restyled by clang-format * Add more comments and fix clang-tidy * Address more review comments --------- Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
project-chip · Aug 29, 2023 · d77c363 · d77c363
1 parent 5a77bc0
commit d77c363
Show file tree

Hide file tree

Showing 9 changed files with 182 additions and 62 deletions.
diff --git a/...development/commissioner_dut/struct_pai_vidpid_fallback_encoding_08/test_case_vector.json b/...development/commissioner_dut/struct_pai_vidpid_fallback_encoding_08/test_case_vector.json
@@ -1,6 +1,6 @@
 {
     "description": "PAI Test Vector: Fallback VID and PID encoding example from spec: invalid, since substring following Mpid: is not exactly 4 uppercase hexadecimal digits",
-    "is_success_case": "true",
+    "is_success_case": "false",
     "dac_cert": "308201d93082017fa003020102020846cb41af3a7fc9c3300a06082a8648ce3d040302303b3139303706035504030c3041434d45204d617474657220446576656c20504149203543444139383939204d7669643a46464631204d7069643a42313020170d3232303932333030303030305a180f39393939313233313233353935395a30463118301606035504030c0f4d617474657220546573742044414331143012060a2b0601040182a27c02010c044646463131143012060a2b0601040182a27c02020c04303042313059301306072a8648ce3d020106082a8648ce3d03010703420004d3e1c5422ab213f118f959a33d2204342cf598dffcf5cec0474a1bba6e4c1a7263a59c605cf892038a704fd266149b5c89fe35e17e8dda279bb03cb1c7bf8ca6a360305e300c0603551d130101ff04023000300e0603551d0f0101ff040403020780301d0603551d0e0416041430616dcabe874ac65e3dce88de7c5cc5defd86f9301f0603551d230418301680148fa036268e8eedbc73e12a0eeadcb2fbec8faaac300a06082a8648ce3d040302034800304502204ef134d60bb150def30f7c2df8f88c46755e36f48a12831aaedf17c4944cd70d022100f61555085107e4e783a5ad6abc64e1ee1f0d719b7d83a5f8bace41949f85c284",
     "pai_cert": "308201c93082016fa003020102020821b17d362fe9afae300a06082a8648ce3d04030230303118301606035504030c0f4d617474657220546573742050414131143012060a2b0601040182a27c02010c04464646313020170d3232303932333030303030305a180f39393939313233313233353935395a303b3139303706035504030c3041434d45204d617474657220446576656c20504149203543444139383939204d7669643a46464631204d7069643a42313059301306072a8648ce3d020106082a8648ce3d03010703420004e72dbe2a0f600358978022ea70fb7ef3ef14de92a0dfb9cfb83b542fbccb7b8a7b13d3f37a26dfd2400224ae4ab82e1c13ca57a45e4ee54e862a192a6d4f1f3aa366306430120603551d130101ff040830060101ff020100300e0603551d0f0101ff040403020106301d0603551d0e041604148fa036268e8eedbc73e12a0eeadcb2fbec8faaac301f0603551d230418301680146afd22771f511fecbf1641976710dcdc31a1717e300a06082a8648ce3d040302034800304502210093a63c6f397d39dfa5b60c06c2839448ab5db0bf0728c7449c66bdc55e3ff6c302207b0c997ff95f1795b86828d1137b34f3f2aef1ffb2183a6e4c02836c1c735171",
     "certification_declaration": "3081e906092a864886f70d010702a081db3081d8020103310d300b0609608648016503040201304406092a864886f70d010701a0370435152400012501f1ff360204b118250334122c04135a494732303134315a423333303030312d32342405002406002507769824080018317e307c020103801462fa823359acfaa9963e1cfa140addf504f37160300b0609608648016503040201300a06082a8648ce3d04030204483046022100e1d4967878513c474101db44c68061689be47443e21f6675c34b872edc5a319c022100a963e07bcd8e4abeb8b4fcb19f6a59c92f1a3bad4d7153f333684f93a836e3f9",

diff --git a/...development/commissioner_dut/struct_pai_vidpid_fallback_encoding_09/test_case_vector.json b/...development/commissioner_dut/struct_pai_vidpid_fallback_encoding_09/test_case_vector.json
@@ -1,6 +1,6 @@
 {
     "description": "PAI Test Vector: Fallback VID and PID encoding example from spec: invalid, since substring following Mpid: is not exactly 4 uppercase hexadecimal digits",
-    "is_success_case": "true",
+    "is_success_case": "false",
     "dac_cert": "308201d83082017da0030201020208697ea481e17f0f9c300a06082a8648ce3d04030230393137303506035504030c2e41434d45204d617474657220446576656c20504149203543444139383939204d7069643a204d7669643a464646313020170d3232303932333030303030305a180f39393939313233313233353935395a30463118301606035504030c0f4d617474657220546573742044414331143012060a2b0601040182a27c02010c044646463131143012060a2b0601040182a27c02020c04303042313059301306072a8648ce3d020106082a8648ce3d030107034200046eb8078776faf91061011baca81d4577b5b6b64e6f3009ddbc05aea20eaa044dbde2f70716de1d0ea2221a57ba5ca2ab23995fff18e63c6c675e403f7bf7ed53a360305e300c0603551d130101ff04023000300e0603551d0f0101ff040403020780301d0603551d0e041604141c1bdaa7b3c18076e81187da63295ee068a7fe29301f0603551d23041830168014bd781517ec524b8b752e7e529063de96d6c2deb1300a06082a8648ce3d0403020349003046022100ca59fb5cc9fd777641ba761c9fbf16dc24f044fcfca5dead17ed7cae02a3f64d022100bb501e52bef2e0ff3d9b6c9bc8dac1c6adf94a19d66f38cf840b89e0bbd7dc12",
     "pai_cert": "308201c63082016da00302010202082dbfc034ee2ebe32300a06082a8648ce3d04030230303118301606035504030c0f4d617474657220546573742050414131143012060a2b0601040182a27c02010c04464646313020170d3232303932333030303030305a180f39393939313233313233353935395a30393137303506035504030c2e41434d45204d617474657220446576656c20504149203543444139383939204d7069643a204d7669643a464646313059301306072a8648ce3d020106082a8648ce3d030107034200041a872e39c4f8337945c51b1a5dfa009e0f8402a59d996da057aa370acb18450bc3316babcf7c6eed0555300d7e9e4ac42c0e9556857835e8e8de2d91a1bbd27da366306430120603551d130101ff040830060101ff020100300e0603551d0f0101ff040403020106301d0603551d0e04160414bd781517ec524b8b752e7e529063de96d6c2deb1301f0603551d230418301680146afd22771f511fecbf1641976710dcdc31a1717e300a06082a8648ce3d040302034700304402205c38ff93c407961a98e61bc23ea65381a760f97ac0481ece8ee21bcd451b8c2002201d6c7ee510aec10ea02402f5f6d80bc74489fb8793d64a77e06b8fece69cd4b8",
     "certification_declaration": "3081e706092a864886f70d010702a081d93081d6020103310d300b0609608648016503040201304406092a864886f70d010701a0370435152400012501f1ff360204b118250334122c04135a494732303134315a423333303030312d32342405002406002507769824080018317c307a020103801462fa823359acfaa9963e1cfa140addf504f37160300b0609608648016503040201300a06082a8648ce3d0403020446304402201f38b426df25ab4f552ded014845afb1a353e808e9250fc19dcfcab3d717679f0220575a610c996f0221628ff9a76123d3462922f91b6011ba727e6d42884ae88c08",

diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp
@@ -29,6 +29,7 @@
 #include <lib/support/BytesToHex.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/Span.h>
+#include <stdint.h>
 #include <string.h>
 
 using chip::ByteSpan;
@@ -38,6 +39,8 @@ using chip::Encoding::LittleEndian::Reader;
 
 using namespace chip::ASN1;
 
+namespace chip {
+namespace Crypto {
 namespace {
 
 constexpr uint8_t kIntegerTag         = 0x02u;
@@ -159,10 +162,76 @@ CHIP_ERROR ConvertIntegerRawToDerInternal(const ByteSpan & raw_integer, MutableB
     return CHIP_NO_ERROR;
 }
 
-} // namespace
+/**
+ * @brief Find a 4 uppercase hex digit hex value after a prefix string. Used to implement
+ *        fallback CN VID/PID encoding for PAA/PAI/DAC.
+ *
+ * @param[in] buffer - buffer in which to find the substring.
+ * @param[in] prefix - prefix to match, which must be followed by 4 uppercase hex characters
+ * @param[out] out_hex_value - on CHIP_NO_ERROR return, this will be the 16-bit hex value decoded.
+ * @return CHIP_NO_ERROR on success, CHIP_ERROR_NOT_FOUND if not detected and
+ *         CHIP_ERROR_WRONG_CERT_DN if we saw the prefix but no valid hex string.
+ */
+CHIP_ERROR Find16BitUpperCaseHexAfterPrefix(const ByteSpan & buffer, const char * prefix, uint16_t & out_hex_value)
+{
+    chip::CharSpan prefix_span = chip::CharSpan::fromCharString(prefix);
 
-namespace chip {
-namespace Crypto {
+    bool found_prefix_at_least_once = false;
+
+    // Scan string from left to right, to find the desired full matching substring.
+    //
+    // IMPORTANT NOTE: We are trying to find the equivalent of prefix + [0-9A-F]{4}.
+    // The appearance of the full prefix, but not followed by the hex value, must
+    // be detected, as it is illegal if there isn't a valid prefix within the string.
+    // This is why we first check for the prefix and then maybe check for the hex
+    // value, rather than doing a single check of making sure there is enough space
+    // for both.
+    for (size_t start_idx = 0; start_idx < buffer.size(); start_idx++)
+    {
+        const uint8_t * cursor = buffer.data() + start_idx;
+        size_t remaining       = buffer.size() - start_idx;
+
+        if (remaining < prefix_span.size())
+        {
+            // We can't possibly match prefix if not enough bytes left.
+            break;
+        }
+
+        // Try to match the prefix at current position.
+        if (memcmp(cursor, prefix_span.data(), prefix_span.size()) != 0)
+        {
+            // Did not find prefix, move to next position.
+            continue;
+        }
+
+        // Otherwise, found prefix, skip to possible hex value.
+        found_prefix_at_least_once = true;
+        cursor += prefix_span.size();
+        remaining -= prefix_span.size();
+
+        constexpr size_t expected_hex_len = HEX_ENCODED_LENGTH(sizeof(uint16_t));
+        if (remaining < expected_hex_len)
+        {
+            // We can't possibly match the hex values if not enough bytes left.
+            break;
+        }
+
+        char hex_buf[expected_hex_len];
+        memcpy(&hex_buf[0], cursor, sizeof(hex_buf));
+
+        if (Encoding::UppercaseHexToUint16(&hex_buf[0], sizeof(hex_buf), out_hex_value) != 0)
+        {
+            // Found first full valid match, return success, out_hex_value already updated.
+            return CHIP_NO_ERROR;
+        }
+
+        // Otherwise, did not find what we were looking for, try next position until exhausted.
+    }
+
+    return found_prefix_at_least_once ? CHIP_ERROR_WRONG_CERT_DN : CHIP_ERROR_NOT_FOUND;
+}
+
+} // namespace
 
 using HKDF_sha_crypto = HKDF_sha;
 
@@ -887,41 +956,39 @@ CHIP_ERROR ExtractVIDPIDFromAttributeString(DNAttrType attrType, const ByteSpan
     // Otherwise, it is a CommonName attribute.
     else if (!vidpidFromCNAttr.Initialized())
     {
-        char cnAttr[kMax_CommonNameAttr_Length + 1];
-        if (attr.size() <= chip::Crypto::kMax_CommonNameAttr_Length)
+        ByteSpan attr_source_span{ attr };
+        if (attr_source_span.size() > chip::Crypto::kMax_CommonNameAttr_Length)
         {
-            memcpy(cnAttr, attr.data(), attr.size());
-            cnAttr[attr.size()] = 0;
+            attr_source_span.reduce_size(chip::Crypto::kMax_CommonNameAttr_Length);
+        }
 
-            char * vid = strstr(cnAttr, kVIDPrefixForCNEncoding);
-            if (vid != nullptr)
-            {
-                vid += strlen(kVIDPrefixForCNEncoding);
-                if (cnAttr + attr.size() >= vid + kVIDandPIDHexLength)
-                {
-                    uint16_t matterAttr;
-                    if (Encoding::UppercaseHexToUint16(vid, kVIDandPIDHexLength, matterAttr) == sizeof(matterAttr))
-                    {
-                        vidpidFromCNAttr.mVendorId.SetValue(static_cast<VendorId>(matterAttr));
-                    }
-                }
-            }
+        // Try to find a valid Vendor ID encoded in fallback method.
+        uint16_t vid   = 0;
+        CHIP_ERROR err = Find16BitUpperCaseHexAfterPrefix(attr_source_span, kVIDPrefixForCNEncoding, vid);
+        if (err == CHIP_NO_ERROR)
+        {
+            vidpidFromCNAttr.mVendorId.SetValue(static_cast<VendorId>(vid));
+        }
+        else if (err != CHIP_ERROR_NOT_FOUND)
+        {
+            // This indicates a bad/ambiguous format.
+            return err;
+        }
 
-            char * pid = strstr(cnAttr, kPIDPrefixForCNEncoding);
-            if (pid != nullptr)
-            {
-                pid += strlen(kPIDPrefixForCNEncoding);
-                if (cnAttr + attr.size() >= pid + kVIDandPIDHexLength)
-                {
-                    uint16_t matterAttr;
-                    if (Encoding::UppercaseHexToUint16(pid, kVIDandPIDHexLength, matterAttr) == sizeof(matterAttr))
-                    {
-                        vidpidFromCNAttr.mProductId.SetValue(matterAttr);
-                    }
-                }
-            }
+        // Try to find a valid Product ID encoded in fallback method.
+        uint16_t pid = 0;
+        err          = Find16BitUpperCaseHexAfterPrefix(attr_source_span, kPIDPrefixForCNEncoding, pid);
+        if (err == CHIP_NO_ERROR)
+        {
+            vidpidFromCNAttr.mProductId.SetValue(pid);
+        }
+        else if (err != CHIP_ERROR_NOT_FOUND)
+        {
+            // This indicates a bad/ambiguous format.
+            return err;
         }
     }
+
     return CHIP_NO_ERROR;
 }
 

diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h
@@ -755,7 +755,7 @@ CHIP_ERROR AES_CTR_crypt(const uint8_t * input, size_t input_length, const Aes12
  * be configured to ignore CSR requested subject.
  *
  * @param keypair The key pair for which a CSR should be generated. Must not be null.
- * @param csr_span Span to hold the resulting CSR. Must be at least kMIN_CSR_Buffer_Size.
+ * @param csr_span Span to hold the resulting CSR. Must have size at least kMIN_CSR_Buffer_Size.
  *                 Otherwise returns CHIP_ERROR_BUFFER_TOO_SMALL. It will get resized to
  *                 actual size needed on success.
 

diff --git a/src/crypto/OperationalKeystore.h b/src/crypto/OperationalKeystore.h
@@ -67,7 +67,7 @@ class OperationalKeystore
      *  Only one pending operational keypair is supported at a time.
      *
      * @param fabricIndex - FabricIndex for which a new keypair must be made available
-     * @param outCertificateSigningRequest - Buffer to contain the CSR. Must be at least `kMIN_CSR_Buffer_Size` large.
+     * @param outCertificateSigningRequest - Buffer to contain the CSR. Must have size at least `kMIN_CSR_Buffer_Size`.
      *
      * @retval CHIP_NO_ERROR on success
      * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCertificateSigningRequest` buffer is too small