From e87c25d6c46e2533ff3deb6bdebd9b7d9a6c67a0 Mon Sep 17 00:00:00 2001 From: Evgeny Margolis Date: Tue, 1 Mar 2022 10:36:43 -0800 Subject: [PATCH] Check for Valid Node Id, Fabric Id, and CATs Values When Processing CHIP Certs. (#15475) -- Node Id should be in a range [1, 0xFFFFFFEFFFFFFFFF] -- Fabric Id should be different from 0 -- CAT should be different from 0 --- .../operational-credentials-server.cpp | 4 +- src/credentials/CHIPCert.cpp | 40 ++++++++++++------- src/credentials/CHIPCertFromX509.cpp | 20 +++++++--- src/credentials/tests/TestChipCert.cpp | 2 +- src/lib/core/CASEAuthTag.h | 5 +++ src/lib/core/CHIPError.cpp | 6 +-- src/lib/core/CHIPError.h | 11 ++--- src/lib/core/PeerId.h | 5 +++ src/lib/core/tests/TestCHIPErrorStr.cpp | 5 +-- src/tools/chip-cert/Cmd_GenCert.cpp | 22 ++++++---- 10 files changed, 78 insertions(+), 42 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 768abd33f88d29..be8d55e65414f8 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2021 Project CHIP Authors + * Copyright (c) 2021-2022 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -500,7 +500,7 @@ OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err) err == CHIP_ERROR_CERT_PATH_TOO_LONG || err == CHIP_ERROR_CERT_USAGE_NOT_ALLOWED || err == CHIP_ERROR_CERT_EXPIRED || err == CHIP_ERROR_CERT_NOT_VALID_YET || err == CHIP_ERROR_UNSUPPORTED_CERT_FORMAT || err == CHIP_ERROR_UNSUPPORTED_ELLIPTIC_CURVE || err == CHIP_ERROR_CERT_LOAD_FAILED || - err == CHIP_ERROR_CERT_NOT_TRUSTED || err == CHIP_ERROR_WRONG_CERT_SUBJECT) + err == CHIP_ERROR_CERT_NOT_TRUSTED || err == CHIP_ERROR_WRONG_CERT_DN) { return OperationalCertStatus::kInvalidNOC; } diff --git a/src/credentials/CHIPCert.cpp b/src/credentials/CHIPCert.cpp index 546915729179d1..a741edcd1a50e8 100644 --- a/src/credentials/CHIPCert.cpp +++ b/src/credentials/CHIPCert.cpp @@ -605,40 +605,40 @@ CHIP_ERROR ChipDN::GetCertType(uint8_t & certType) const { if (rdn[i].mAttrOID == kOID_AttributeType_ChipRootId) { - VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_DN); lCertType = kCertType_Root; } else if (rdn[i].mAttrOID == kOID_AttributeType_ChipICAId) { - VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_DN); lCertType = kCertType_ICA; } else if (rdn[i].mAttrOID == kOID_AttributeType_ChipNodeId) { - VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_TYPE); - + VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_DN); + VerifyOrReturnError(IsOperationalNodeId(rdn[i].mChipVal), CHIP_ERROR_WRONG_CERT_DN); lCertType = kCertType_Node; } else if (rdn[i].mAttrOID == kOID_AttributeType_ChipFirmwareSigningId) { - VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrExit(lCertType == kCertType_NotSpecified, err = CHIP_ERROR_WRONG_CERT_DN); lCertType = kCertType_FirmwareSigning; } else if (rdn[i].mAttrOID == kOID_AttributeType_ChipFabricId) { // Only one fabricId attribute is allowed per DN. - VerifyOrExit(!fabricIdPresent, err = CHIP_ERROR_WRONG_CERT_TYPE); - + VerifyOrExit(!fabricIdPresent, err = CHIP_ERROR_WRONG_CERT_DN); + VerifyOrReturnError(IsValidFabricId(rdn[i].mChipVal), CHIP_ERROR_WRONG_CERT_DN); fabricIdPresent = true; } } if (lCertType == kCertType_Node) { - VerifyOrExit(fabricIdPresent, err = CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrExit(fabricIdPresent, err = CHIP_ERROR_WRONG_CERT_DN); } certType = lCertType; @@ -661,7 +661,7 @@ CHIP_ERROR ChipDN::GetCertChipId(uint64_t & chipId) const case kOID_AttributeType_ChipICAId: case kOID_AttributeType_ChipNodeId: case kOID_AttributeType_ChipFirmwareSigningId: - VerifyOrReturnError(chipId == 0, CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrReturnError(chipId == 0, CHIP_ERROR_WRONG_CERT_DN); chipId = rdn[i].mChipVal; break; @@ -677,16 +677,16 @@ CHIP_ERROR ChipDN::GetCertFabricId(uint64_t & fabricId) const { uint8_t rdnCount = RDNCount(); - fabricId = UINT64_MAX; + fabricId = kUndefinedFabricId; for (uint8_t i = 0; i < rdnCount; i++) { switch (rdn[i].mAttrOID) { case kOID_AttributeType_ChipFabricId: - // Ensure only one FabricID RDN present, since start value is UINT64_MAX, which is reserved and never seen. - VerifyOrReturnError(fabricId == UINT64_MAX, CHIP_ERROR_WRONG_CERT_TYPE); - + // Ensure only one FabricID RDN present, since start value is kUndefinedFabricId, which is reserved and never seen. + VerifyOrReturnError(fabricId == kUndefinedFabricId, CHIP_ERROR_WRONG_CERT_DN); + VerifyOrReturnError(IsValidFabricId(rdn[i].mChipVal), CHIP_ERROR_WRONG_CERT_DN); fabricId = rdn[i].mChipVal; break; default: @@ -694,7 +694,7 @@ CHIP_ERROR ChipDN::GetCertFabricId(uint64_t & fabricId) const } } - VerifyOrReturnError(fabricId != UINT64_MAX, CHIP_ERROR_WRONG_CERT_TYPE); + VerifyOrReturnError(IsValidFabricId(fabricId), CHIP_ERROR_WRONG_CERT_DN); return CHIP_NO_ERROR; } @@ -740,6 +740,14 @@ CHIP_ERROR ChipDN::DecodeFromTLV(TLVReader & reader) uint64_t chipAttr; VerifyOrReturnError(attrIsPrintableString == false, CHIP_ERROR_INVALID_TLV_TAG); ReturnErrorOnFailure(reader.Get(chipAttr)); + if (attrOID == chip::ASN1::kOID_AttributeType_ChipNodeId) + { + VerifyOrReturnError(IsOperationalNodeId(attrOID), CHIP_ERROR_INVALID_ARGUMENT); + } + else if (attrOID == chip::ASN1::kOID_AttributeType_ChipFabricId) + { + VerifyOrReturnError(IsValidFabricId(attrOID), CHIP_ERROR_INVALID_ARGUMENT); + } ReturnErrorOnFailure(AddAttribute(attrOID, chipAttr)); } // For 32-bit CHIP-defined DN attributes. @@ -748,6 +756,10 @@ CHIP_ERROR ChipDN::DecodeFromTLV(TLVReader & reader) uint32_t chipAttr; VerifyOrReturnError(attrIsPrintableString == false, CHIP_ERROR_INVALID_TLV_TAG); ReturnErrorOnFailure(reader.Get(chipAttr)); + if (attrOID == chip::ASN1::kOID_AttributeType_ChipCASEAuthenticatedTag) + { + VerifyOrReturnError(IsValidCASEAuthTag(chipAttr), CHIP_ERROR_INVALID_ARGUMENT); + } ReturnErrorOnFailure(AddAttribute(attrOID, chipAttr)); } // Otherwise the attribute is one of the supported X.509 attributes diff --git a/src/credentials/CHIPCertFromX509.cpp b/src/credentials/CHIPCertFromX509.cpp index f0ef8d503e681c..0ddacb40e2cfc5 100644 --- a/src/credentials/CHIPCertFromX509.cpp +++ b/src/credentials/CHIPCertFromX509.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020-2021 Project CHIP Authors + * Copyright (c) 2020-2022 Project CHIP Authors * Copyright (c) 2013-2017 Nest Labs, Inc. * All rights reserved. * @@ -141,10 +141,6 @@ static CHIP_ERROR ConvertDistinguishedName(ASN1Reader & reader, TLVWriter & writ err = ParseChipAttribute(reader, chipAttr); SuccessOrExit(err); - // Write the CHIP attribute value into the TLV. - err = writer.Put(ContextTag(tlvTagNum), chipAttr); - SuccessOrExit(err); - // Certificates use a combination of OIDs for Issuer and Subject. // NOC: Issuer = kOID_AttributeType_ChipRootId or kOID_AttributeType_ChipICAId // Subject = kOID_AttributeType_ChipNodeId @@ -160,12 +156,26 @@ static CHIP_ERROR ConvertDistinguishedName(ASN1Reader & reader, TLVWriter & writ attrOID == chip::ASN1::kOID_AttributeType_ChipICAId || attrOID == chip::ASN1::kOID_AttributeType_ChipRootId) { + if (attrOID == chip::ASN1::kOID_AttributeType_ChipNodeId) + { + VerifyOrReturnError(IsOperationalNodeId(chipAttr), CHIP_ERROR_WRONG_CERT_DN); + } subjectOrIssuer = chipAttr; } else if (attrOID == chip::ASN1::kOID_AttributeType_ChipFabricId) { + VerifyOrReturnError(IsValidFabricId(chipAttr), CHIP_ERROR_WRONG_CERT_DN); fabric.SetValue(chipAttr); } + else if (attrOID == chip::ASN1::kOID_AttributeType_ChipCASEAuthenticatedTag) + { + VerifyOrReturnError(CanCastTo(chipAttr), CHIP_ERROR_WRONG_CERT_DN); + VerifyOrReturnError(IsValidCASEAuthTag(static_cast(chipAttr)), CHIP_ERROR_WRONG_CERT_DN); + } + + // Write the CHIP attribute value into the TLV. + err = writer.Put(ContextTag(tlvTagNum), chipAttr); + SuccessOrExit(err); } // diff --git a/src/credentials/tests/TestChipCert.cpp b/src/credentials/tests/TestChipCert.cpp index 8dd132c1146b9f..dceb46c3cbd8d0 100644 --- a/src/credentials/tests/TestChipCert.cpp +++ b/src/credentials/tests/TestChipCert.cpp @@ -892,7 +892,7 @@ static void TestChipCert_GenerateNOCRoot(nlTestSuite * inSuite, void * inContext NL_TEST_ASSERT(inSuite, NewNodeOperationalX509Cert(noc_params, noc_keypair.Pubkey(), keypair, signed_cert_span1) == - CHIP_ERROR_WRONG_CERT_TYPE); + CHIP_ERROR_WRONG_CERT_DN); // Test error case: issuer cert DN type is Node certificate noc_params.SubjectDN.Clear(); diff --git a/src/lib/core/CASEAuthTag.h b/src/lib/core/CASEAuthTag.h index 7397d19dfcd2f1..fcc9c9b536c5e0 100644 --- a/src/lib/core/CASEAuthTag.h +++ b/src/lib/core/CASEAuthTag.h @@ -70,4 +70,9 @@ constexpr CASEAuthTag CASEAuthTagFromNodeId(NodeId aNodeId) return aNodeId & kMaskCASEAuthTag; } +constexpr CASEAuthTag IsValidCASEAuthTag(CASEAuthTag aCAT) +{ + return (aCAT & kTagVersionMask) > 0; +} + } // namespace chip diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index f3dd0284467673..fb630d73e4578a 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2022 Project CHIP Authors * Copyright (c) 2019 Google LLC. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -350,8 +350,8 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_INVALID_ACCESS_TOKEN.AsInteger(): desc = "Invalid access token"; break; - case CHIP_ERROR_WRONG_CERT_SUBJECT.AsInteger(): - desc = "Wrong certificate subject"; + case CHIP_ERROR_WRONG_CERT_DN.AsInteger(): + desc = "Wrong certificate distinguished name"; break; case CHIP_ERROR_INVALID_PROVISIONING_BUNDLE.AsInteger(): desc = "Invalid provisioning bundle"; diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 7420d851e48467..c55fd3935527e5 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020-2021 Project CHIP Authors + * Copyright (c) 2020-2022 Project CHIP Authors * Copyright (c) 2013-2017 Nest Labs, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -1232,16 +1232,13 @@ using CHIP_ERROR = ::chip::ChipError; #define CHIP_ERROR_INVALID_ACCESS_TOKEN CHIP_CORE_ERROR(0x58) /** - * @def CHIP_ERROR_WRONG_CERT_SUBJECT + * @def CHIP_ERROR_WRONG_CERT_DN * * @brief - * A certificate subject is wrong. + * A certificate subject/issuer distinguished name is wrong. * */ -#define CHIP_ERROR_WRONG_CERT_SUBJECT CHIP_CORE_ERROR(0x59) - -// deprecated alias -#define CHIP_ERROR_WRONG_CERTIFICATE_SUBJECT CHIP_ERROR_WRONG_CERT_SUBJECT +#define CHIP_ERROR_WRONG_CERT_DN CHIP_CORE_ERROR(0x59) /** * @def CHIP_ERROR_INVALID_PROVISIONING_BUNDLE diff --git a/src/lib/core/PeerId.h b/src/lib/core/PeerId.h index 7155ee32f62d42..4586ee11cda288 100644 --- a/src/lib/core/PeerId.h +++ b/src/lib/core/PeerId.h @@ -28,6 +28,11 @@ constexpr CompressedFabricId kUndefinedCompressedFabricId = 0ULL; constexpr FabricId kUndefinedFabricId = 0ULL; +constexpr bool IsValidFabricId(FabricId aFabricId) +{ + return aFabricId != kUndefinedFabricId; +} + /// A peer is identified by a node id within a compressed fabric ID class PeerId { diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp index 1f8e5e78fba931..9e5b165763e915 100644 --- a/src/lib/core/tests/TestCHIPErrorStr.cpp +++ b/src/lib/core/tests/TestCHIPErrorStr.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2022 Project CHIP Authors * Copyright (c) 2016-2017 Nest Labs, Inc. * All rights reserved. * @@ -137,8 +137,7 @@ static const CHIP_ERROR kTestElements[] = CHIP_ERROR_CERT_LOAD_FAILED, CHIP_ERROR_CERT_NOT_TRUSTED, CHIP_ERROR_INVALID_ACCESS_TOKEN, - CHIP_ERROR_WRONG_CERT_SUBJECT, - CHIP_ERROR_WRONG_CERTIFICATE_SUBJECT, + CHIP_ERROR_WRONG_CERT_DN, CHIP_ERROR_INVALID_PROVISIONING_BUNDLE, CHIP_ERROR_PROVISIONING_BUNDLE_DECRYPTION_ERROR, CHIP_ERROR_WRONG_NODE_ID, diff --git a/src/tools/chip-cert/Cmd_GenCert.cpp b/src/tools/chip-cert/Cmd_GenCert.cpp index 89c776f6dbb715..9c085846663a1f 100644 --- a/src/tools/chip-cert/Cmd_GenCert.cpp +++ b/src/tools/chip-cert/Cmd_GenCert.cpp @@ -73,19 +73,21 @@ const char * const gCmdOptionHelp = "\n" " -i, --subject-chip-id \n" "\n" - " Subject DN CHIP Id attribute (in hex). For Node Certificate it is CHIP Node Id attribute.\n" + " Subject DN CHIP Id attribute in hexadecimal format with upto 8 octets with or without '0x' prefix.\n" " - for Root certificate it is ChipRootId\n" " - for intermediate CA certificate it is ChipICAId\n" - " - for Node certificate it is ChipNodeId\n" + " - for Node certificate it is ChipNodeId. The value should be in a range [1, 0xFFFFFFEFFFFFFFFF]\n" " - for Firmware Signing certificate it is ChipFirmwareSigningId\n" "\n" " -f, --subject-fab-id \n" "\n" - " Subject DN Fabric Id attribute (in hex).\n" + " Subject DN Fabric Id attribute in hexadecimal format with upto 8 octets with or without '0x' prefix.\n" + " The value should be different from 0.\n" "\n" " -a, --subject-cat \n" "\n" - " Subject DN CHIP CASE Authentication Tag (in hex).\n" + " Subject DN CHIP CASE Authentication Tag in hexadecimal format with upto 4 octets with or without '0x' prefix.\n" + " The version subfield (lower 16 bits) should be different from 0.\n" "\n" " -c, --subject-cn-u \n" "\n" @@ -236,6 +238,11 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char switch (gCertType) { case kCertType_Node: + if (!chip::IsOperationalNodeId(chip64bitAttr)) + { + PrintArgError("%s: Invalid value specified for chip node-id attribute: %s\n", progName, arg); + return false; + } attrOID = kOID_AttributeType_ChipNodeId; break; case kCertType_FirmwareSigning: @@ -261,9 +268,10 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char break; case 'a': - if (!ParseInt(arg, chip32bitAttr, 16)) + if (!ParseInt(arg, chip32bitAttr, 16) || chip::IsValidCASEAuthTag(chip32bitAttr)) { - PrintArgError("%s: Invalid value specified for the subject authentication tag attribute: %s\n", progName, arg); + PrintArgError("%s: Invalid value specified for the subject CASE Authenticated Tag (CAT) attribute: %s\n", progName, + arg); return false; } attrOID = kOID_AttributeType_ChipCASEAuthenticatedTag; @@ -284,7 +292,7 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char } break; case 'f': - if (!ParseInt(arg, chip64bitAttr, 16)) + if (!ParseInt(arg, chip64bitAttr, 16) || !chip::IsValidFabricId(chip64bitAttr)) { PrintArgError("%s: Invalid value specified for subject fabric id attribute: %s\n", progName, arg); return false;