Skip to content

Commit 70cbc02

Browse files
tcarmelveilleuxtennessee-googlerestyled-commits
authored
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 CHIP-Specifications/connectedhomeip-spec#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>
1 parent 454d7d9 commit 70cbc02

File tree

9 files changed

+182
-62
lines changed

9 files changed

+182
-62
lines changed

‎credentials/development/commissioner_dut/struct_pai_vidpid_fallback_encoding_08/test_case_vector.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"description": "PAI Test Vector: Fallback VID and PID encoding example from spec: invalid, since substring following Mpid: is not exactly 4 uppercase hexadecimal digits",
3-
"is_success_case": "true",
3+
"is_success_case": "false",
44
"dac_cert": "308201d93082017fa003020102020846cb41af3a7fc9c3300a06082a8648ce3d040302303b3139303706035504030c3041434d45204d617474657220446576656c20504149203543444139383939204d7669643a46464631204d7069643a42313020170d3232303932333030303030305a180f39393939313233313233353935395a30463118301606035504030c0f4d617474657220546573742044414331143012060a2b0601040182a27c02010c044646463131143012060a2b0601040182a27c02020c04303042313059301306072a8648ce3d020106082a8648ce3d03010703420004d3e1c5422ab213f118f959a33d2204342cf598dffcf5cec0474a1bba6e4c1a7263a59c605cf892038a704fd266149b5c89fe35e17e8dda279bb03cb1c7bf8ca6a360305e300c0603551d130101ff04023000300e0603551d0f0101ff040403020780301d0603551d0e0416041430616dcabe874ac65e3dce88de7c5cc5defd86f9301f0603551d230418301680148fa036268e8eedbc73e12a0eeadcb2fbec8faaac300a06082a8648ce3d040302034800304502204ef134d60bb150def30f7c2df8f88c46755e36f48a12831aaedf17c4944cd70d022100f61555085107e4e783a5ad6abc64e1ee1f0d719b7d83a5f8bace41949f85c284",
55
"pai_cert": "308201c93082016fa003020102020821b17d362fe9afae300a06082a8648ce3d04030230303118301606035504030c0f4d617474657220546573742050414131143012060a2b0601040182a27c02010c04464646313020170d3232303932333030303030305a180f39393939313233313233353935395a303b3139303706035504030c3041434d45204d617474657220446576656c20504149203543444139383939204d7669643a46464631204d7069643a42313059301306072a8648ce3d020106082a8648ce3d03010703420004e72dbe2a0f600358978022ea70fb7ef3ef14de92a0dfb9cfb83b542fbccb7b8a7b13d3f37a26dfd2400224ae4ab82e1c13ca57a45e4ee54e862a192a6d4f1f3aa366306430120603551d130101ff040830060101ff020100300e0603551d0f0101ff040403020106301d0603551d0e041604148fa036268e8eedbc73e12a0eeadcb2fbec8faaac301f0603551d230418301680146afd22771f511fecbf1641976710dcdc31a1717e300a06082a8648ce3d040302034800304502210093a63c6f397d39dfa5b60c06c2839448ab5db0bf0728c7449c66bdc55e3ff6c302207b0c997ff95f1795b86828d1137b34f3f2aef1ffb2183a6e4c02836c1c735171",
66
"certification_declaration": "3081e906092a864886f70d010702a081db3081d8020103310d300b0609608648016503040201304406092a864886f70d010701a0370435152400012501f1ff360204b118250334122c04135a494732303134315a423333303030312d32342405002406002507769824080018317e307c020103801462fa823359acfaa9963e1cfa140addf504f37160300b0609608648016503040201300a06082a8648ce3d04030204483046022100e1d4967878513c474101db44c68061689be47443e21f6675c34b872edc5a319c022100a963e07bcd8e4abeb8b4fcb19f6a59c92f1a3bad4d7153f333684f93a836e3f9",

‎credentials/development/commissioner_dut/struct_pai_vidpid_fallback_encoding_09/test_case_vector.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"description": "PAI Test Vector: Fallback VID and PID encoding example from spec: invalid, since substring following Mpid: is not exactly 4 uppercase hexadecimal digits",
3-
"is_success_case": "true",
3+
"is_success_case": "false",
44
"dac_cert": "308201d83082017da0030201020208697ea481e17f0f9c300a06082a8648ce3d04030230393137303506035504030c2e41434d45204d617474657220446576656c20504149203543444139383939204d7069643a204d7669643a464646313020170d3232303932333030303030305a180f39393939313233313233353935395a30463118301606035504030c0f4d617474657220546573742044414331143012060a2b0601040182a27c02010c044646463131143012060a2b0601040182a27c02020c04303042313059301306072a8648ce3d020106082a8648ce3d030107034200046eb8078776faf91061011baca81d4577b5b6b64e6f3009ddbc05aea20eaa044dbde2f70716de1d0ea2221a57ba5ca2ab23995fff18e63c6c675e403f7bf7ed53a360305e300c0603551d130101ff04023000300e0603551d0f0101ff040403020780301d0603551d0e041604141c1bdaa7b3c18076e81187da63295ee068a7fe29301f0603551d23041830168014bd781517ec524b8b752e7e529063de96d6c2deb1300a06082a8648ce3d0403020349003046022100ca59fb5cc9fd777641ba761c9fbf16dc24f044fcfca5dead17ed7cae02a3f64d022100bb501e52bef2e0ff3d9b6c9bc8dac1c6adf94a19d66f38cf840b89e0bbd7dc12",
55
"pai_cert": "308201c63082016da00302010202082dbfc034ee2ebe32300a06082a8648ce3d04030230303118301606035504030c0f4d617474657220546573742050414131143012060a2b0601040182a27c02010c04464646313020170d3232303932333030303030305a180f39393939313233313233353935395a30393137303506035504030c2e41434d45204d617474657220446576656c20504149203543444139383939204d7069643a204d7669643a464646313059301306072a8648ce3d020106082a8648ce3d030107034200041a872e39c4f8337945c51b1a5dfa009e0f8402a59d996da057aa370acb18450bc3316babcf7c6eed0555300d7e9e4ac42c0e9556857835e8e8de2d91a1bbd27da366306430120603551d130101ff040830060101ff020100300e0603551d0f0101ff040403020106301d0603551d0e04160414bd781517ec524b8b752e7e529063de96d6c2deb1301f0603551d230418301680146afd22771f511fecbf1641976710dcdc31a1717e300a06082a8648ce3d040302034700304402205c38ff93c407961a98e61bc23ea65381a760f97ac0481ece8ee21bcd451b8c2002201d6c7ee510aec10ea02402f5f6d80bc74489fb8793d64a77e06b8fece69cd4b8",
66
"certification_declaration": "3081e706092a864886f70d010702a081d93081d6020103310d300b0609608648016503040201304406092a864886f70d010701a0370435152400012501f1ff360204b118250334122c04135a494732303134315a423333303030312d32342405002406002507769824080018317c307a020103801462fa823359acfaa9963e1cfa140addf504f37160300b0609608648016503040201300a06082a8648ce3d0403020446304402201f38b426df25ab4f552ded014845afb1a353e808e9250fc19dcfcab3d717679f0220575a610c996f0221628ff9a76123d3462922f91b6011ba727e6d42884ae88c08",

‎src/crypto/CHIPCryptoPAL.cpp

+100-33
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <lib/support/BytesToHex.h>
3030
#include <lib/support/CodeUtils.h>
3131
#include <lib/support/Span.h>
32+
#include <stdint.h>
3233
#include <string.h>
3334

3435
using chip::ByteSpan;
@@ -38,6 +39,8 @@ using chip::Encoding::LittleEndian::Reader;
3839

3940
using namespace chip::ASN1;
4041

42+
namespace chip {
43+
namespace Crypto {
4144
namespace {
4245

4346
constexpr uint8_t kIntegerTag = 0x02u;
@@ -159,10 +162,76 @@ CHIP_ERROR ConvertIntegerRawToDerInternal(const ByteSpan & raw_integer, MutableB
159162
return CHIP_NO_ERROR;
160163
}
161164

162-
} // namespace
165+
/**
166+
* @brief Find a 4 uppercase hex digit hex value after a prefix string. Used to implement
167+
* fallback CN VID/PID encoding for PAA/PAI/DAC.
168+
*
169+
* @param[in] buffer - buffer in which to find the substring.
170+
* @param[in] prefix - prefix to match, which must be followed by 4 uppercase hex characters
171+
* @param[out] out_hex_value - on CHIP_NO_ERROR return, this will be the 16-bit hex value decoded.
172+
* @return CHIP_NO_ERROR on success, CHIP_ERROR_NOT_FOUND if not detected and
173+
* CHIP_ERROR_WRONG_CERT_DN if we saw the prefix but no valid hex string.
174+
*/
175+
CHIP_ERROR Find16BitUpperCaseHexAfterPrefix(const ByteSpan & buffer, const char * prefix, uint16_t & out_hex_value)
176+
{
177+
chip::CharSpan prefix_span = chip::CharSpan::fromCharString(prefix);
163178

164-
namespace chip {
165-
namespace Crypto {
179+
bool found_prefix_at_least_once = false;
180+
181+
// Scan string from left to right, to find the desired full matching substring.
182+
//
183+
// IMPORTANT NOTE: We are trying to find the equivalent of prefix + [0-9A-F]{4}.
184+
// The appearance of the full prefix, but not followed by the hex value, must
185+
// be detected, as it is illegal if there isn't a valid prefix within the string.
186+
// This is why we first check for the prefix and then maybe check for the hex
187+
// value, rather than doing a single check of making sure there is enough space
188+
// for both.
189+
for (size_t start_idx = 0; start_idx < buffer.size(); start_idx++)
190+
{
191+
const uint8_t * cursor = buffer.data() + start_idx;
192+
size_t remaining = buffer.size() - start_idx;
193+
194+
if (remaining < prefix_span.size())
195+
{
196+
// We can't possibly match prefix if not enough bytes left.
197+
break;
198+
}
199+
200+
// Try to match the prefix at current position.
201+
if (memcmp(cursor, prefix_span.data(), prefix_span.size()) != 0)
202+
{
203+
// Did not find prefix, move to next position.
204+
continue;
205+
}
206+
207+
// Otherwise, found prefix, skip to possible hex value.
208+
found_prefix_at_least_once = true;
209+
cursor += prefix_span.size();
210+
remaining -= prefix_span.size();
211+
212+
constexpr size_t expected_hex_len = HEX_ENCODED_LENGTH(sizeof(uint16_t));
213+
if (remaining < expected_hex_len)
214+
{
215+
// We can't possibly match the hex values if not enough bytes left.
216+
break;
217+
}
218+
219+
char hex_buf[expected_hex_len];
220+
memcpy(&hex_buf[0], cursor, sizeof(hex_buf));
221+
222+
if (Encoding::UppercaseHexToUint16(&hex_buf[0], sizeof(hex_buf), out_hex_value) != 0)
223+
{
224+
// Found first full valid match, return success, out_hex_value already updated.
225+
return CHIP_NO_ERROR;
226+
}
227+
228+
// Otherwise, did not find what we were looking for, try next position until exhausted.
229+
}
230+
231+
return found_prefix_at_least_once ? CHIP_ERROR_WRONG_CERT_DN : CHIP_ERROR_NOT_FOUND;
232+
}
233+
234+
} // namespace
166235

167236
using HKDF_sha_crypto = HKDF_sha;
168237

@@ -887,41 +956,39 @@ CHIP_ERROR ExtractVIDPIDFromAttributeString(DNAttrType attrType, const ByteSpan
887956
// Otherwise, it is a CommonName attribute.
888957
else if (!vidpidFromCNAttr.Initialized())
889958
{
890-
char cnAttr[kMax_CommonNameAttr_Length + 1];
891-
if (attr.size() <= chip::Crypto::kMax_CommonNameAttr_Length)
959+
ByteSpan attr_source_span{ attr };
960+
if (attr_source_span.size() > chip::Crypto::kMax_CommonNameAttr_Length)
892961
{
893-
memcpy(cnAttr, attr.data(), attr.size());
894-
cnAttr[attr.size()] = 0;
962+
attr_source_span.reduce_size(chip::Crypto::kMax_CommonNameAttr_Length);
963+
}
895964

896-
char * vid = strstr(cnAttr, kVIDPrefixForCNEncoding);
897-
if (vid != nullptr)
898-
{
899-
vid += strlen(kVIDPrefixForCNEncoding);
900-
if (cnAttr + attr.size() >= vid + kVIDandPIDHexLength)
901-
{
902-
uint16_t matterAttr;
903-
if (Encoding::UppercaseHexToUint16(vid, kVIDandPIDHexLength, matterAttr) == sizeof(matterAttr))
904-
{
905-
vidpidFromCNAttr.mVendorId.SetValue(static_cast<VendorId>(matterAttr));
906-
}
907-
}
908-
}
965+
// Try to find a valid Vendor ID encoded in fallback method.
966+
uint16_t vid = 0;
967+
CHIP_ERROR err = Find16BitUpperCaseHexAfterPrefix(attr_source_span, kVIDPrefixForCNEncoding, vid);
968+
if (err == CHIP_NO_ERROR)
969+
{
970+
vidpidFromCNAttr.mVendorId.SetValue(static_cast<VendorId>(vid));
971+
}
972+
else if (err != CHIP_ERROR_NOT_FOUND)
973+
{
974+
// This indicates a bad/ambiguous format.
975+
return err;
976+
}
909977

910-
char * pid = strstr(cnAttr, kPIDPrefixForCNEncoding);
911-
if (pid != nullptr)
912-
{
913-
pid += strlen(kPIDPrefixForCNEncoding);
914-
if (cnAttr + attr.size() >= pid + kVIDandPIDHexLength)
915-
{
916-
uint16_t matterAttr;
917-
if (Encoding::UppercaseHexToUint16(pid, kVIDandPIDHexLength, matterAttr) == sizeof(matterAttr))
918-
{
919-
vidpidFromCNAttr.mProductId.SetValue(matterAttr);
920-
}
921-
}
922-
}
978+
// Try to find a valid Product ID encoded in fallback method.
979+
uint16_t pid = 0;
980+
err = Find16BitUpperCaseHexAfterPrefix(attr_source_span, kPIDPrefixForCNEncoding, pid);
981+
if (err == CHIP_NO_ERROR)
982+
{
983+
vidpidFromCNAttr.mProductId.SetValue(pid);
984+
}
985+
else if (err != CHIP_ERROR_NOT_FOUND)
986+
{
987+
// This indicates a bad/ambiguous format.
988+
return err;
923989
}
924990
}
991+
925992
return CHIP_NO_ERROR;
926993
}
927994

‎src/crypto/CHIPCryptoPAL.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,7 @@ CHIP_ERROR AES_CTR_crypt(const uint8_t * input, size_t input_length, const Aes12
755755
* be configured to ignore CSR requested subject.
756756
*
757757
* @param keypair The key pair for which a CSR should be generated. Must not be null.
758-
* @param csr_span Span to hold the resulting CSR. Must be at least kMIN_CSR_Buffer_Size.
758+
* @param csr_span Span to hold the resulting CSR. Must have size at least kMIN_CSR_Buffer_Size.
759759
* Otherwise returns CHIP_ERROR_BUFFER_TOO_SMALL. It will get resized to
760760
* actual size needed on success.
761761

‎src/crypto/OperationalKeystore.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class OperationalKeystore
6767
* Only one pending operational keypair is supported at a time.
6868
*
6969
* @param fabricIndex - FabricIndex for which a new keypair must be made available
70-
* @param outCertificateSigningRequest - Buffer to contain the CSR. Must be at least `kMIN_CSR_Buffer_Size` large.
70+
* @param outCertificateSigningRequest - Buffer to contain the CSR. Must have size at least `kMIN_CSR_Buffer_Size`.
7171
*
7272
* @retval CHIP_NO_ERROR on success
7373
* @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCertificateSigningRequest` buffer is too small

0 commit comments

Comments
 (0)