From afd208ef9fa6950a96b3753817f80fe175fdd5fe Mon Sep 17 00:00:00 2001 From: Evgeni Margolis Date: Sat, 5 Mar 2022 20:05:17 -0800 Subject: [PATCH] Updated Fabric ID Validation in Fabric Table -- Updated logic that checks optional fabric id attribute in ICAC and RCAC to match fabric id attribute in NOC. -- According to spec there is no need to require fabric id in ICAC if it is present in RCAC. -- NOTE: validity check for NodeId, FabricId and other DN attributes is performed when loading certificates for validation (LoadCert()). So there is no need to check validity again in FabricInfo::VerifyCredentials(). -- Changed return value of ExtractNodeIdFabricIdFromOpCert() and ExtractFabricIdFromCert() to CHIP_ERROR_NOT_FOUND when requested element is not found in the certificate. --- src/credentials/CHIPCert.cpp | 4 ++-- src/credentials/CHIPCert.h | 4 ++-- src/credentials/FabricTable.cpp | 23 +++++++++++++++-------- src/credentials/tests/TestChipCert.cpp | 4 ++-- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/credentials/CHIPCert.cpp b/src/credentials/CHIPCert.cpp index ba802d41273fd2..130ede10847afb 100644 --- a/src/credentials/CHIPCert.cpp +++ b/src/credentials/CHIPCert.cpp @@ -1013,7 +1013,7 @@ CHIP_ERROR ExtractNodeIdFabricIdFromOpCert(const ChipCertificateData & opcert, N } if (!foundNodeId || !foundFabricId) { - return CHIP_ERROR_INVALID_ARGUMENT; + return CHIP_ERROR_NOT_FOUND; } *outNodeId = nodeId; @@ -1054,7 +1054,7 @@ CHIP_ERROR ExtractFabricIdFromCert(const ChipCertificateData & cert, FabricId * } } - return CHIP_ERROR_INVALID_ARGUMENT; + return CHIP_ERROR_NOT_FOUND; } CHIP_ERROR ExtractCATsFromOpCert(const ByteSpan & opcert, CATValues & cats) diff --git a/src/credentials/CHIPCert.h b/src/credentials/CHIPCert.h index 3e3b576d07108e..660d9e9c449ce4 100644 --- a/src/credentials/CHIPCert.h +++ b/src/credentials/CHIPCert.h @@ -781,7 +781,7 @@ CHIP_ERROR ConvertECDSASignatureDERToRaw(ASN1::ASN1Reader & reader, chip::TLV::T * These certificates may not contain a NodeID, so ExtractNodeIdFabricIdFromOpCert() * cannot be used for such certificates. * - * @return CHIP_ERROR_INVALID_ARGUMENT if the passed-in cert does not have RDN + * @return CHIP_ERROR_NOT_FOUND if the passed-in cert does not have RDN * corresponding to FabricID. */ CHIP_ERROR ExtractFabricIdFromCert(const ChipCertificateData & cert, FabricId * fabricId); @@ -790,7 +790,7 @@ CHIP_ERROR ExtractFabricIdFromCert(const ChipCertificateData & cert, FabricId * * Extract Node ID and Fabric ID from an operational certificate that has already been * parsed. * - * @return CHIP_ERROR_INVALID_ARGUMENT if the passed-in cert does not have at + * @return CHIP_ERROR_NOT_FOUND if the passed-in cert does not have at * least one NodeId RDN and one FabricId RDN in the Subject DN. No other * validation (e.g. checkign that there is exactly one RDN of each type) is * performed. diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 6d1187b079c51c..8eece2a614b0b5 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -312,25 +312,32 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & NodeId nodeId; ReturnErrorOnFailure(ExtractNodeIdFabricIdFromOpCert(certificates.GetLastCert()[0], &nodeId, &fabricId)); + CHIP_ERROR err; FabricId icacFabricId = kUndefinedFabricId; if (!icac.empty()) { - if (ExtractFabricIdFromCert(certificates.GetCertSet()[1], &icacFabricId) == CHIP_NO_ERROR && - icacFabricId != kUndefinedFabricId) + err = ExtractFabricIdFromCert(certificates.GetCertSet()[1], &icacFabricId); + if (err == CHIP_NO_ERROR) { ReturnErrorCodeIf(icacFabricId != fabricId, CHIP_ERROR_FABRIC_MISMATCH_ON_ICA); } + // FabricId is optional field in ICAC and "not found" code is not treated as error. + else if (err != CHIP_ERROR_NOT_FOUND) + { + return err; + } } FabricId rcacFabricId = kUndefinedFabricId; - if (ExtractFabricIdFromCert(certificates.GetCertSet()[0], &rcacFabricId) == CHIP_NO_ERROR && rcacFabricId != kUndefinedFabricId) + err = ExtractFabricIdFromCert(certificates.GetCertSet()[0], &rcacFabricId); + if (err == CHIP_NO_ERROR) { ReturnErrorCodeIf(rcacFabricId != fabricId, CHIP_ERROR_WRONG_CERT_DN); - if (!icac.empty()) - { - // If FabricId attribute is present in RCAC then it SHOULD be present in ICAC as well. - ReturnErrorCodeIf(icacFabricId == kUndefinedFabricId, CHIP_ERROR_WRONG_CERT_DN); - } + } + // FabricId is optional field in RCAC and "not found" code is not treated as error. + else if (err != CHIP_ERROR_NOT_FOUND) + { + return err; } ReturnErrorOnFailure(GeneratePeerId(fabricId, nodeId, &nocPeerId)); diff --git a/src/credentials/tests/TestChipCert.cpp b/src/credentials/tests/TestChipCert.cpp index dceb46c3cbd8d0..ed14292b892706 100644 --- a/src/credentials/tests/TestChipCert.cpp +++ b/src/credentials/tests/TestChipCert.cpp @@ -1196,7 +1196,7 @@ static void TestChipCert_ExtractNodeIdFabricId(nlTestSuite * inSuite, void * inC FabricId fabricId; err = ExtractFabricIdFromCert(cert, &fabricId); - NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_NOT_FOUND); } // Test extraction from the parsed form of ICA Cert that doesn't have FabricId. @@ -1209,7 +1209,7 @@ static void TestChipCert_ExtractNodeIdFabricId(nlTestSuite * inSuite, void * inC FabricId fabricId; err = ExtractFabricIdFromCert(certSet.GetCertSet()[0], &fabricId); - NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT); + NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_NOT_FOUND); certSet.Release(); } }