Skip to content

Commit

Permalink
Remove empty state from FixedSpan (#29240)
Browse files Browse the repository at this point in the history
* Remove empty state from FixedSpan

Prior to this change, a default-constructed FixedSpan would be in a state
with empty() = true but size() > 0, but nevertheless compare as not equal
to an empty non-fixed Span.

After this change, a FixedSpan always contains a valid sequence of elements
of the relevant size. This aligns the behavior with std::span.

The IsSpanUsable() function and the empty() method have been removed
because they would always return true, and hence invite programmer errors.

Since we still rely on default-constructing certain FixedByteSpan types in
a number of places, such instances are now initialized to point to an array
of zeroes (the arrays for all sizes of span are shared and limited in size
to avoid inadvertently blowing out the constant data size in the binary).

* Disallow Spans pointing to null if size is not 0

This is validated using VerifyOrDie, but only in code paths that take a raw
pointer and size; code paths that take a T[] assume the array is valid and
remain unchecked and constexpr.

* Deprecate IsSpanUsable(Span) in favor of !empty()

* Fix test cases that use invalid ByteSpans

* Use ByteSpan() for empty span instead of ByteSpan(nullptr, 0)

* Address review comments

- Fix buffer size check in ConvertECDSASignatureRawToDER
- Add deleted nullptr_t overloads to Span and FixedSpan
- Comments around constexpr / VerifyOrDie

* Fix #29326 as per review

---------

Co-authored-by: Andrei Litvin <[email protected]>
  • Loading branch information
ksperling-apple and andy31415 authored Sep 22, 2023
1 parent bc64cb0 commit 59d2b7e
Show file tree
Hide file tree
Showing 49 changed files with 167 additions and 165 deletions.
8 changes: 5 additions & 3 deletions examples/chip-tool/commands/pairing/IssueNOCChainCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,12 @@ class IssueNOCChainCommand : public CHIPCommand
VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err));
ChipLogProgress(chipTool, "RCAC: %s", rcacStr.c_str());

auto ipkValue = ipk.ValueOr(chip::Crypto::IdentityProtectionKeySpan());
std::string ipkStr;
err = ToBase64(ipkValue, ipkStr);
VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err));
if (ipk.HasValue())
{
err = ToBase64(ipk.Value(), ipkStr);
VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err));
}
ChipLogProgress(chipTool, "IPK: %s", ipkStr.c_str());

err = RemoteDataModelLogger::LogIssueNOCChain(nocStr.c_str(), icacStr.c_str(), rcacStr.c_str(), ipkStr.c_str());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ CHIP_ERROR DeviceAttestationCredsCC13X2_26X2::SignWithDeviceAttestationKey(const
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// In a non-exemplary implementation, the public key is not needed here. It is used here merely because
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,8 @@ CHIP_ERROR DeviceAttestationCredsCC13X4_26X4::SignWithDeviceAttestationKey(const
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// In a non-exemplary implementation, the public key is not needed here. It is used here merely because
Expand Down
4 changes: 2 additions & 2 deletions examples/platform/cc32xx/CC32XXDeviceAttestationCreds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ CHIP_ERROR DeviceAttestationCredsCC32XX::SignWithDeviceAttestationKey(const Byte
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// In a non-exemplary implementation, the public key is not needed here. It is used here merely because
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ CHIP_ERROR ExampleSe05xDACProvider::SignWithDeviceAttestationKey(const ByteSpan

ChipLogDetail(Crypto, "Sign using DA key from se05x");

VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_signature_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// Add public key + reference private key (ref to key inside SE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ CHIP_ERROR ExampleSe05xDACProviderv2::SignWithDeviceAttestationKey(const ByteSpa
CHIP_ERROR err = CHIP_NO_ERROR;
uint8_t signature_se05x[Crypto::kMax_ECDSA_Signature_Length_Der] = { 0 };
size_t signature_se05x_len = sizeof(signature_se05x);
VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_signature_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);

ChipLogDetail(Crypto, "Sign using DA key from se05x (Using internal sign)");

Expand Down
4 changes: 2 additions & 2 deletions examples/virtual-device-app/android/java/JNIDACProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(digest_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_signature_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!digest_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

uint8_t privateKeyBuf[Crypto::kP256_PrivateKey_Length];
Expand Down
2 changes: 1 addition & 1 deletion src/app/clusters/ota-requestor/DefaultOTARequestor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ CHIP_ERROR DefaultOTARequestor::ExtractUpdateDescription(const QueryImageRespons

VerifyOrReturnError(response.imageURI.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
ReturnErrorOnFailure(bdx::ParseURI(response.imageURI.Value(), nodeId, fileDesignator));
VerifyOrReturnError(IsSpanUsable(fileDesignator), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!fileDesignator.empty(), CHIP_ERROR_INVALID_ARGUMENT);
update.nodeId = nodeId;
update.fileDesignator = fileDesignator;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ struct ReplacementProductStruct : private HepaFilterMonitoring::Structs::Replace
*/
CHIP_ERROR SetProductIdentifierValue(chip::CharSpan aProductIdentifierValue)
{
VerifyOrReturnError(IsSpanUsable(aProductIdentifierValue), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!aProductIdentifierValue.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(aProductIdentifierValue.size() <= sizeof(productIdentifierValueBuffer), CHIP_ERROR_INVALID_ARGUMENT);

memcpy(productIdentifierValueBuffer, aProductIdentifierValue.data(), aProductIdentifierValue.size());
Expand Down
4 changes: 2 additions & 2 deletions src/app/tests/suites/credentials/TestHarnessDACProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,8 @@ CHIP_ERROR TestHarnessDACProvider::SignWithDeviceAttestationKey(const ByteSpan &
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_signature_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// In a non-exemplary implementation, the public key is not needed here. It is used here merely because
Expand Down
7 changes: 2 additions & 5 deletions src/controller/python/ChipDeviceController-IssueNocChain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,6 @@ void pychip_DeviceController_IssueNOCChainCallback(void * context, CHIP_ERROR st
MutableByteSpan chipIcacSpan;
MutableByteSpan chipRcacSpan;

Crypto::IdentityProtectionKeySpan ipkData;
ipkData = ipk.ValueOr(Crypto::IdentityProtectionKeySpan());

CHIP_ERROR err = status;
if (err != CHIP_NO_ERROR)
{
Expand All @@ -91,8 +88,8 @@ void pychip_DeviceController_IssueNOCChainCallback(void * context, CHIP_ERROR st
{
pychip_DeviceController_IssueNOCChainCallbackPythonCallbackFunct(
context, ToPyChipError(err), chipNocSpan.data(), chipNocSpan.size(), chipIcacSpan.data(), chipIcacSpan.size(),
chipRcacSpan.data(), chipRcacSpan.size(), ipkData.data(), ipk.HasValue() ? ipkData.size() : 0,
adminSubject.ValueOr(kUndefinedNodeId));
chipRcacSpan.data(), chipRcacSpan.size(), ipk.HasValue() ? ipk.Value().data() : nullptr,
ipk.HasValue() ? ipk.Value().size() : 0, adminSubject.ValueOr(kUndefinedNodeId));
}
else
{
Expand Down
17 changes: 4 additions & 13 deletions src/credentials/CHIPCert.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -485,23 +485,17 @@ CHIP_ERROR ChipCertificateSet::FindValidCert(const ChipDN & subjectDN, const Cer
// Default error if we don't find any matching cert.
err = (depth > 0) ? CHIP_ERROR_CA_CERT_NOT_FOUND : CHIP_ERROR_CERT_NOT_FOUND;

// Fail immediately if neither of the input criteria are specified.
if (subjectDN.IsEmpty() && subjectKeyId.empty())
{
ExitNow();
}

// For each cert in the set...
for (uint8_t i = 0; i < mCertCount; i++)
{
ChipCertificateData * candidateCert = &mCerts[i];

// Skip the certificate if its subject DN and key id do not match the input criteria.
if (!subjectDN.IsEmpty() && !candidateCert->mSubjectDN.IsEqual(subjectDN))
if (!candidateCert->mSubjectDN.IsEqual(subjectDN))
{
continue;
}
if (!subjectKeyId.empty() && !candidateCert->mSubjectKeyId.data_equal(subjectKeyId))
if (!candidateCert->mSubjectKeyId.data_equal(subjectKeyId))
{
continue;
}
Expand Down Expand Up @@ -1205,12 +1199,11 @@ CHIP_ERROR ConvertIntegerDERToRaw(ByteSpan derInt, uint8_t * rawInt, const uint1

CHIP_ERROR ConvertECDSASignatureRawToDER(P256ECDSASignatureSpan rawSig, MutableByteSpan & derSig)
{
ASN1Writer writer;
VerifyOrReturnError(derSig.size() >= kMax_ECDSA_Signature_Length_Der, CHIP_ERROR_BUFFER_TOO_SMALL);

ASN1Writer writer;
writer.Init(derSig);

ReturnErrorOnFailure(ConvertECDSASignatureRawToDER(rawSig, writer));

derSig.reduce_size(writer.GetLengthWritten());

return CHIP_NO_ERROR;
Expand All @@ -1221,8 +1214,6 @@ CHIP_ERROR ConvertECDSASignatureRawToDER(P256ECDSASignatureSpan rawSig, ASN1Writ
CHIP_ERROR err = CHIP_NO_ERROR;
uint8_t derInt[kP256_FE_Length + kEmitDerIntegerWithoutTagOverhead];

VerifyOrReturnError(!rawSig.empty(), CHIP_ERROR_INVALID_ARGUMENT);

// Ecdsa-Sig-Value ::= SEQUENCE
ASN1_START_SEQUENCE
{
Expand Down
5 changes: 1 addition & 4 deletions src/credentials/TestOnlyLocalCertificateAuthority.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,7 @@ class TestOnlyLocalCertificateAuthority
bool IsSuccess() { return mCurrentStatus == CHIP_NO_ERROR; }

ByteSpan GetNoc() const { return ByteSpan{ mLastNoc.Get(), mLastNoc.AllocatedSize() }; }
ByteSpan GetIcac() const
{
return mIncludeIcac ? ByteSpan{ mLastIcac.Get(), mLastIcac.AllocatedSize() } : ByteSpan{ nullptr, 0 };
}
ByteSpan GetIcac() const { return mIncludeIcac ? ByteSpan{ mLastIcac.Get(), mLastIcac.AllocatedSize() } : ByteSpan{}; }
ByteSpan GetRcac() const { return ByteSpan{ mLastRcac.Get(), mLastRcac.AllocatedSize() }; }

TestOnlyLocalCertificateAuthority & GenerateNocChain(FabricId fabricId, NodeId nodeId,
Expand Down
4 changes: 2 additions & 2 deletions src/credentials/examples/DeviceAttestationCredsExample.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ CHIP_ERROR ExampleDACProvider::SignWithDeviceAttestationKey(const ByteSpan & mes
Crypto::P256ECDSASignature signature;
Crypto::P256Keypair keypair;

VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!out_signature_buffer.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!message_to_sign.empty(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);

// In a non-exemplary implementation, the public key is not needed here. It is used here merely because
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/tests/TestDeviceAttestationCredentials.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ static void TestAttestationTrustStore(nlTestSuite * inSuite, void * inContext)
ByteSpan kPaaFFF1BadSkidSpan1{ TestCerts::sTestCert_PAA_FFF1_Cert.data(), TestCerts::sTestCert_PAA_FFF1_Cert.size() - 1 };

// SKID to trigger CHIP_ERROR_INVALID_ARGUMENT
ByteSpan kPaaFFF1BadSkidSpan2{ nullptr, TestCerts::sTestCert_PAA_FFF1_Cert.size() };
ByteSpan kPaaFFF1BadSkidSpan2;

// SKID to trigger CHIP_ERROR_CA_CERT_NOT_FOUND
uint8_t kPaaGoodSkidNotPresent[] = { 0x6A, 0xFD, 0x22, 0x77, 0x1F, 0x51, 0x71, 0x1F, 0xEC, 0xBF,
Expand Down
4 changes: 2 additions & 2 deletions src/crypto/CHIPCryptoPAL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, MutableByteSpan raw_in
CHIP_ERROR ConvertIntegerRawToDerInternal(const ByteSpan & raw_integer, MutableByteSpan & out_der_integer,
bool include_tag_and_length)
{
if (!IsSpanUsable(raw_integer) || !IsSpanUsable(out_der_integer))
if (raw_integer.empty() || out_der_integer.empty())
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
Expand Down Expand Up @@ -906,7 +906,7 @@ CHIP_ERROR DeriveGroupPrivacyKey(const ByteSpan & encryption_key, MutableByteSpa
VerifyOrReturnError(Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES == encryption_key.size(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES <= out_key.size(), CHIP_ERROR_INVALID_ARGUMENT);

const ByteSpan null_span = ByteSpan(nullptr, 0);
constexpr ByteSpan null_span = ByteSpan();

Crypto::HKDF_sha crypto;
return crypto.HKDF_SHA256(encryption_key.data(), encryption_key.size(), null_span.data(), null_span.size(), kGroupPrivacyInfo,
Expand Down
17 changes: 5 additions & 12 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -574,14 +574,11 @@ static void TestRawIntegerToDerInvalidCases(nlTestSuite * inSuite, void * inCont
HeapChecker heapChecker(inSuite);
// Cover case of invalid buffers
uint8_t placeholder[10] = { 0 };
MutableByteSpan good_out_buffer(placeholder, sizeof(placeholder));
ByteSpan good_buffer(placeholder, sizeof(placeholder));
MutableByteSpan good_out_buffer(placeholder);
ByteSpan good_buffer(placeholder);

MutableByteSpan bad_out_buffer_nullptr(nullptr, sizeof(placeholder));
MutableByteSpan bad_out_buffer_empty(placeholder, 0);

ByteSpan bad_buffer_nullptr(nullptr, sizeof(placeholder));
ByteSpan bad_buffer_empty(placeholder, 0);
MutableByteSpan bad_out_buffer_empty;
ByteSpan bad_buffer_empty;

struct ErrorCase
{
Expand All @@ -591,9 +588,7 @@ static void TestRawIntegerToDerInvalidCases(nlTestSuite * inSuite, void * inCont
};

const ErrorCase error_cases[] = {
{ .input = good_buffer, .output = bad_out_buffer_nullptr, .expected_status = CHIP_ERROR_INVALID_ARGUMENT },
{ .input = good_buffer, .output = bad_out_buffer_empty, .expected_status = CHIP_ERROR_INVALID_ARGUMENT },
{ .input = bad_buffer_nullptr, .output = good_out_buffer, .expected_status = CHIP_ERROR_INVALID_ARGUMENT },
{ .input = bad_buffer_empty, .output = good_out_buffer, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }
};

Expand Down Expand Up @@ -681,7 +676,6 @@ static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContex
{
uint8_t placeholder[1];

ByteSpan bad_buffer_nullptr(nullptr, sizeof(placeholder));
ByteSpan bad_buffer_empty(placeholder, 0);

const uint8_t zero_multi_byte_length[] = { 0x80 };
Expand Down Expand Up @@ -716,7 +710,6 @@ static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContex
};

const ErrorCase error_cases[] = {
{ .input_buf = bad_buffer_nullptr, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL },
{ .input_buf = bad_buffer_empty, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL },
{ .input_buf = zero_multi_byte_length_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT },
{ .input_buf = single_byte_length_zero_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT },
Expand Down Expand Up @@ -2622,7 +2615,7 @@ static void TestVIDPID_StringExtraction(nlTestSuite * inSuite, void * inContext)
{ DNAttrType::kCommonName, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute16), strlen(sTestCNAttribute16)), true, true, chip::VendorId::TestVendor1, 0xFE67, CHIP_NO_ERROR },
// Other input combinations:
{ DNAttrType::kUnspecified, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute15), strlen(sTestCNAttribute15)), false, false, chip::VendorId::NotSpecified, 0, CHIP_NO_ERROR },
{ DNAttrType::kCommonName, ByteSpan(nullptr, 0), false, false, chip::VendorId::NotSpecified, 0, CHIP_ERROR_INVALID_ARGUMENT },
{ DNAttrType::kCommonName, ByteSpan(), false, false, chip::VendorId::NotSpecified, 0, CHIP_ERROR_INVALID_ARGUMENT },
};
// clang-format on

Expand Down
Loading

0 comments on commit 59d2b7e

Please sign in to comment.