Skip to content

Commit 4db45bf

Browse files
vijsvijs
committed
Addressed review comments
1 parent a11e4b3 commit 4db45bf

File tree

4 files changed

+61
-82
lines changed

4 files changed

+61
-82
lines changed

‎scripts/tools/check_includes_config.py

+1
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@
134134

135135
'src/credentials/attestation_verifier/FileAttestationTrustStore.h': {'vector'},
136136
'src/credentials/attestation_verifier/FileAttestationTrustStore.cpp': {'string'},
137+
'src/credentials/attestation_verifier/JSONFileRevocationSet.cpp': {'fstream'},
137138

138139
'src/setup_payload/AdditionalDataPayload.h': {'string'},
139140
'src/setup_payload/AdditionalDataPayloadParser.cpp': {'vector'},

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp

+24-34
Original file line numberDiff line numberDiff line change
@@ -459,45 +459,15 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
459459
}
460460

461461
{
462-
uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 };
463-
MutableByteSpan paaIssuer(issuerBuf);
464-
MutableByteSpan paiIssuer(issuerBuf);
465-
MutableByteSpan dacIssuer(issuerBuf);
466-
uint8_t akidBuf[kAuthorityKeyIdentifierLength];
467-
MutableByteSpan akid(akidBuf);
468-
uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength];
469-
MutableByteSpan serialNumber(serialNumberBuf);
470-
471-
VerifyOrExit(ExtractIssuerFromX509Cert(paaDerBuffer, paaIssuer) == CHIP_NO_ERROR,
472-
attestationError = AttestationVerificationResult::kPaaFormatInvalid);
473-
VerifyOrExit(ExtractAKIDFromX509Cert(paaDerBuffer, akid) == CHIP_NO_ERROR,
474-
attestationError = AttestationVerificationResult::kPaaFormatInvalid);
475-
VerifyOrExit(ExtractSerialNumberFromX509Cert(paaDerBuffer, serialNumber) == CHIP_NO_ERROR,
476-
attestationError = AttestationVerificationResult::kPaaFormatInvalid);
477-
478-
attestationError = IsCertificateRevoked(true, paaVidPid, paaIssuer, akid, serialNumber);
462+
attestationError = IsCertificateRevoked(true, paaVidPid, paaDerBuffer);
479463
VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
480464
attestationError = AttestationVerificationResult::kPaaRevoked);
481465

482-
VerifyOrExit(ExtractIssuerFromX509Cert(info.paiDerBuffer, paiIssuer) == CHIP_NO_ERROR,
483-
attestationError = AttestationVerificationResult::kPaiFormatInvalid);
484-
VerifyOrExit(ExtractAKIDFromX509Cert(info.paiDerBuffer, akid) == CHIP_NO_ERROR,
485-
attestationError = AttestationVerificationResult::kPaiFormatInvalid);
486-
VerifyOrExit(ExtractSerialNumberFromX509Cert(info.paiDerBuffer, serialNumber) == CHIP_NO_ERROR,
487-
attestationError = AttestationVerificationResult::kPaiFormatInvalid);
488-
489-
attestationError = IsCertificateRevoked(false, paiVidPid, paiIssuer, akid, serialNumber);
466+
attestationError = IsCertificateRevoked(false, paiVidPid, info.paiDerBuffer);
490467
VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
491468
attestationError = AttestationVerificationResult::kPaiRevoked);
492469

493-
VerifyOrExit(ExtractIssuerFromX509Cert(info.dacDerBuffer, dacIssuer) == CHIP_NO_ERROR,
494-
attestationError = AttestationVerificationResult::kDacFormatInvalid);
495-
VerifyOrExit(ExtractAKIDFromX509Cert(info.dacDerBuffer, akid) == CHIP_NO_ERROR,
496-
attestationError = AttestationVerificationResult::kDacFormatInvalid);
497-
VerifyOrExit(ExtractSerialNumberFromX509Cert(info.dacDerBuffer, serialNumber) == CHIP_NO_ERROR,
498-
attestationError = AttestationVerificationResult::kDacFormatInvalid);
499-
500-
attestationError = IsCertificateRevoked(false, dacVidPid, dacIssuer, akid, serialNumber);
470+
attestationError = IsCertificateRevoked(false, dacVidPid, info.dacDerBuffer);
501471
VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess,
502472
attestationError = AttestationVerificationResult::kDacRevoked);
503473
}
@@ -651,11 +621,31 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa
651621
return CHIP_NO_ERROR;
652622
}
653623

624+
AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest,
625+
ByteSpan certificate)
626+
{
627+
uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 };
628+
MutableByteSpan issuer(issuerBuf);
629+
uint8_t akidBuf[kAuthorityKeyIdentifierLength];
630+
MutableByteSpan akid(akidBuf);
631+
uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength];
632+
MutableByteSpan serialNumber(serialNumberBuf);
633+
634+
VerifyOrReturnError(ExtractIssuerFromX509Cert(certificate, issuer) == CHIP_NO_ERROR,
635+
AttestationVerificationResult::kPaaFormatInvalid);
636+
VerifyOrReturnError(ExtractAKIDFromX509Cert(certificate, akid) == CHIP_NO_ERROR,
637+
AttestationVerificationResult::kPaaFormatInvalid);
638+
VerifyOrReturnError(ExtractSerialNumberFromX509Cert(certificate, serialNumber) == CHIP_NO_ERROR,
639+
AttestationVerificationResult::kPaaFormatInvalid);
640+
641+
return IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, akid, serialNumber);
642+
}
643+
654644
AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, AttestationCertVidPid vidPidUnderTest,
655645
ByteSpan issuer, ByteSpan authorityKeyId,
656646
ByteSpan serialNumber)
657647
{
658-
VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kNotImplemented);
648+
VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kSuccess);
659649

660650
return mRevocationSet->IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, authorityKeyId, serialNumber);
661651
}

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h

+5-1
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ class DefaultDACVerifier : public DeviceAttestationVerifier
8787
CsaCdKeysTrustStore mCdKeysTrustStore;
8888
const AttestationTrustStore * mAttestationTrustStore;
8989
const RevocationSet * mRevocationSet;
90+
91+
AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest,
92+
ByteSpan certificate);
9093
};
9194

9295
/**
@@ -115,7 +118,8 @@ const AttestationTrustStore * GetTestAttestationTrustStore();
115118
* process lifetime. In particular, after the first call it's not
116119
* possible to change which AttestationTrustStore is used by this verifier.
117120
*/
118-
DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet);
121+
DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore,
122+
const RevocationSet * revocationSet = nullptr);
119123

120124
} // namespace Credentials
121125
} // namespace chip

‎src/credentials/attestation_verifier/JSONFileRevocationSet.cpp

+31-47
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@
1616
*/
1717
#include "JSONFileRevocationSet.h"
1818

19+
#include <lib/core/CHIPSafeCasts.h>
1920
#include <lib/support/Base64.h>
2021
#include <lib/support/BytesToHex.h>
2122
#include <lib/support/ScopedBuffer.h>
2223

2324
#include <cstdlib>
2425
#include <fstream>
25-
#include <iostream>
2626

2727
namespace chip {
2828
namespace Credentials {
@@ -53,17 +53,16 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i
5353
ByteSpan issuer, ByteSpan authorityKeyId,
5454
ByteSpan serialNumber) const
5555
{
56-
for (size_t revocation_set_idx = 0; revocation_set_idx < mRevocationSet.size(); ++revocation_set_idx)
56+
for (int revocation_set_idx = 0; revocation_set_idx < static_cast<int>(mRevocationSet.size()); ++revocation_set_idx)
5757
{
58-
Json::Value type = mRevocationSet[static_cast<int>(revocation_set_idx)]["type"];
58+
Json::Value revocationSetEntry = mRevocationSet[revocation_set_idx];
5959

6060
// 1.
61-
if (type.asString().compare("revocation_set") == 0)
61+
if (revocationSetEntry["type"].asString().compare("revocation_set") == 0)
6262
{
63-
Json::Value jsonCrlIssuerSubjectKeyId = mRevocationSet[static_cast<int>(revocation_set_idx)]["issuer_subject_key_id"];
64-
Json::Value jsonCrlIssuerName = mRevocationSet[static_cast<int>(revocation_set_idx)]["issuer_name"];
65-
Json::Value jsonCrlRevokedSerialNumbers =
66-
mRevocationSet[static_cast<int>(revocation_set_idx)]["revoked_serial_numbers"];
63+
Json::Value jsonCrlIssuerSubjectKeyId = revocationSetEntry["issuer_subject_key_id"];
64+
Json::Value jsonCrlIssuerName = revocationSetEntry["issuer_name"];
65+
Json::Value jsonCrlRevokedSerialNumbers = revocationSetEntry["revoked_serial_numbers"];
6766

6867
uint8_t crlIssuerSubjectKeyIdBuf[Crypto::kAuthorityKeyIdentifierLength] = { 0 };
6968
ByteSpan crlIssuerSubjectKeyId(crlIssuerSubjectKeyIdBuf);
@@ -81,62 +80,46 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i
8180

8281
// 2.
8382
size_t crlSignerCertificateLength =
84-
Base64Decode(jsonCrlIssuerName.asString().c_str(), jsonCrlIssuerName.asString().size(), crlSignerCertificate.Get());
83+
Base64Decode(jsonCrlIssuerName.asString().c_str(), static_cast<uint16_t>(jsonCrlIssuerName.asString().size()),
84+
crlSignerCertificate.Get());
8585
VerifyOrReturnError(crlSignerCertificateLength > 0 && crlSignerCertificateLength != UINT16_MAX,
8686
AttestationVerificationResult::kInternalError);
8787

88-
// 3.
89-
if (isPaa)
90-
{
91-
Crypto::AttestationCertVidPid vid;
88+
// 3. && 4.
89+
Crypto::AttestationCertVidPid vidPid;
9290

93-
Crypto::ExtractVIDPIDFromAttributeString(
94-
Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vid, vid);
91+
Crypto::ExtractVIDPIDFromAttributeString(
92+
Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid, vidPid);
9593

96-
if (vid.mVendorId.HasValue())
94+
if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value())
95+
{
96+
// VID does not match. Stop further processing and continue to next entry.
97+
continue;
98+
}
99+
100+
if (isPaa)
101+
{
102+
if (vidPid.mProductId.HasValue())
97103
{
98-
if (vid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value())
99-
{
100-
// VID does not match. Stop further processing and continue to next entry.
101-
continue;
102-
}
104+
// PAA must not contain PID entry. Format wrong. Continuing to next entry.
105+
continue;
103106
}
104107
}
105-
// 4.
106108
else
107109
{
108-
Crypto::AttestationCertVidPid vidPid;
109-
110-
Crypto::ExtractVIDPIDFromAttributeString(Crypto::DNAttrType::kCommonName,
111-
ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid,
112-
vidPid);
113-
114-
if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value())
110+
if (vidPid.mProductId.HasValue() && vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value())
115111
{
116-
// VID does not match. Stop further processing and continue to next entry.
112+
// PID does not match. Stop further processing and continue to next entry.
117113
continue;
118114
}
119-
120-
if (vidPid.mProductId.HasValue())
121-
{
122-
if (vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value())
123-
{
124-
// PID does not match. Stop further processing and continue to next entry.
125-
continue;
126-
}
127-
}
128115
}
129116

130-
// 7.a Perform CRLFile validation
117+
// 7. Perform CRLFile validation
131118
if (authorityKeyId.data_equal(crlIssuerSubjectKeyId) == false)
132119
{
133120
continue;
134121
}
135122

136-
// TODO: 7.b
137-
138-
// TODO: 8.
139-
140123
// 9. && 10.
141124
for (int serial_number_idx = 0; serial_number_idx < static_cast<int>(jsonCrlRevokedSerialNumbers.size());
142125
++serial_number_idx)
@@ -151,15 +134,16 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i
151134

152135
size_t revokedSerialNumberLength =
153136
Base64Decode(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().c_str(),
154-
jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size(), revokedSerialNumber.Get());
137+
static_cast<uint16_t>(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size()),
138+
revokedSerialNumber.Get());
155139
VerifyOrReturnError(revokedSerialNumberLength > 0 && revokedSerialNumberLength != UINT16_MAX,
156140
AttestationVerificationResult::kInternalError);
157141

158142
uint8_t crlRevokedSerialNumberBuf[Crypto::kMaxCertificateSerialNumberLength] = { 0 };
159143
ByteSpan crlSerialNumber(crlRevokedSerialNumberBuf);
160144

161-
VerifyOrReturnError(Encoding::HexToBytes(reinterpret_cast<char *>(revokedSerialNumber.Get()),
162-
revokedSerialNumberLength, crlRevokedSerialNumberBuf,
145+
VerifyOrReturnError(Encoding::HexToBytes(Uint8::to_char(revokedSerialNumber.Get()), revokedSerialNumberLength,
146+
crlRevokedSerialNumberBuf,
163147
sizeof(crlRevokedSerialNumberBuf)) == sizeof(crlRevokedSerialNumberBuf),
164148
AttestationVerificationResult::kInvalidArgument);
165149

0 commit comments

Comments
 (0)